Repository navigation
Description
Summary
The namespace-approval workflow (added in #44085) posts a
> ⚠️ **Namespace changed** — approvals for ... have been reset.
warning that lists all languages, even when only one language's package name actually changed in tspconfig.yaml (and even when no approvals ever existed for the other languages).
This was observed in #44553, where only the @azure-tools/typespec-ts (TypeScript/JS) emitter section changed, yet the bot reported dotnet, python, java, and typescript as reset.
Evidence (#44553)
The tspconfig.yaml diff changed only the TypeScript emitter options:
"@azure-tools/typespec-ts":
- emitter-output-dir: "{output-dir}/{service-dir}/ai-translation-document-rest"
+ emitter-output-dir: "{output-dir}/{service-dir}/ai-translation-document"
generate-metadata: true
- is-modular-library: false
package-details:
- name: "@azure-rest/ai-translation-document"
+ name: "@azure/ai-translation-document"No dotnet / python / java emitter options were touched. Yet the bot posted:
⚠️ Namespace changed — approvals for dotnet, python, java, typescript have been reset.
(comment: #44553 (comment))
The PR timeline confirms this fired on the first-ever namespace detection for the PR (the workflow became active after the PR was originally opened, so the first run happened on a synchronize event). Nothing had ever been approved for any language at that point, so describing them as "reset" is doubly wrong.
Root cause
In .github/workflows/src/namespace-approval/post-results.js (main @ c178c557), the synchronize reset-detection loop:
azure-rest-api-specs/.github/workflows/src/namespace-approval/post-results.js
Lines 251 to 266 in c178c55
| const prev = previousTable.get(language); | |
| const newNs = results.namespacesFound[language]; | |
| if (!prev || prev.namespace !== newNs) { | |
| // Namespace changed or new language: reset approval | |
| const approvedLabel = `namespace-${language}-approved`; | |
| if (existingLabels.includes(approvedLabel)) { | |
| core.info( | |
| `Namespace changed for ${language}: "${prev?.namespace ?? "(new)"}" → "${newNs}", resetting approval`, | |
| ); | |
| await removeLabelIfPresent(github, owner, repo, issue_number, approvedLabel); | |
| existingLabels.splice(existingLabels.indexOf(approvedLabel), 1); | |
| } | |
| resetLanguages.push(language); | |
| } else if (prev.status && !prev.status.includes("Pending")) { | |
| // Namespace unchanged and previously approved: preserve status |
const prev = previousTable.get(language);
const newNs = results.namespacesFound[language];
if (!prev || prev.namespace !== newNs) {
const approvedLabel = `namespace-${language}-approved`;
if (existingLabels.includes(approvedLabel)) {
// ...actually removes the approval label (correctly guarded)
}
resetLanguages.push(language); // <-- unconditional; not guarded
}Two contributing issues:
-
namespacesFoundcontains every language in the file, not just the changed one.detect-namespaces.jsre-parses all emitter options from the whole changedtspconfig.yaml, so the loop iterates over every language even though only one language's section changed in the diff. There is no per-language "did this section change in the diff" gate. -
resetLanguages.push(language)is unconditional. It runs whenever!prevorprev.namespace !== newNs. When there is no previous bot comment (prev === undefined, e.g. first detection), every language satisfies!prevand is pushed intoresetLanguages. The user-facing warning (built fromresetLanguages) then claims approvals were "reset" for languages that (a) were never approved and (b) never changed.
The label removal itself is correctly guarded by existingLabels.includes(approvedLabel), but the resetLanguages list that drives the message is not — so the message overstates what happened.
Warning is emitted here:
azure-rest-api-specs/.github/workflows/src/namespace-approval/post-results.js
Lines 205 to 207 in c178c55
| if (resetLanguages && resetLanguages.length > 0) { | |
| body += `\n> ⚠️ **Namespace changed** — approvals for ${resetLanguages.join(", ")} have been reset.\n`; | |
| } |
Expected behavior
- The "approvals ... have been reset" warning should list only languages whose namespace actually changed from a previously-recorded value and that were previously approved (i.e. an approved label was genuinely removed).
- A first-time detection (
prev === undefined) should treat languages as newly pending, not "reset". - Changing one language's package name should not imply anything about the other languages' approval state.
Suggested fix
Only add a language to resetLanguages when a real reset occurred, and treat "no previous entry" as new/pending rather than reset:
if (prev && prev.namespace !== newNs) {
const approvedLabel = `namespace-${language}-approved`;
if (existingLabels.includes(approvedLabel)) {
// remove approval label + splice existingLabels
resetLanguages.push(language);
}
} else if (prev && prev.status && !prev.status.includes("Pending")) {
preservedApprovals.set(language, prev);
}
// languages with no `prev` fall through to normal pending handling belowReferences
- Introduced by Add automated namespace approval workflow for spec PRs #44085
- Observed in [Translation] Fix DocumentTranslation getSupportedFormats 'type' parameter and response enum casing #44553
- Code:
.github/workflows/src/namespace-approval/post-results.js(reset loop L251-L266, warning L205-L207; detection L238)
Activity
fix: namespace reset only triggers for changed+approved languages
fix: namespace reset only triggers for changed+approved languages