From 3b59a3dfa7d0879b09dc491672dbe3c62025d163 Mon Sep 17 00:00:00 2001 From: Sameeksha Vaity Date: Thu, 18 Jun 2026 16:19:26 -0700 Subject: [PATCH 01/23] Add automated namespace approval workflow for spec PRs GitHub Actions workflow that validates namespace changes on spec PRs: - Detects namespaces from tspconfig.yaml using @azure-tools/typespec-metadata emitter - Applies per-language namespace-pending labels for architect review - Blocks merge until authorized architects approve via namespace-approved labels - Supports namespace-approved-all shortcut for management plane - Resets approval when namespace values change - Uses repo conventions: ESM modules in src/, actions/checkout@v6, actions/github-script@v8, ubuntu-24.04, install-deps-github-script - Reuses existing Mgmt and data-plane labels Files added: - .github/workflows/namespace-approval.yml - .github/workflows/src/namespace-approval/detect-namespaces.js - .github/workflows/src/namespace-approval/validate-approval.js - .github/workflows/src/namespace-approval/status-check.js - .github/namespace-approvers.yml - .github/NAMESPACE-REVIEW-PROCESS.md Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- .github/NAMESPACE-REVIEW-PROCESS.md | 101 ++++++ .github/namespace-approvers.yml | 31 ++ .github/workflows/namespace-approval.yml | 102 ++++++ .../namespace-approval/detect-namespaces.js | 332 ++++++++++++++++++ .../src/namespace-approval/status-check.js | 29 ++ .../namespace-approval/validate-approval.js | 220 ++++++++++++ 6 files changed, 815 insertions(+) create mode 100644 .github/NAMESPACE-REVIEW-PROCESS.md create mode 100644 .github/namespace-approvers.yml create mode 100644 .github/workflows/namespace-approval.yml create mode 100644 .github/workflows/src/namespace-approval/detect-namespaces.js create mode 100644 .github/workflows/src/namespace-approval/status-check.js create mode 100644 .github/workflows/src/namespace-approval/validate-approval.js diff --git a/.github/NAMESPACE-REVIEW-PROCESS.md b/.github/NAMESPACE-REVIEW-PROCESS.md new file mode 100644 index 000000000000..989aa03f7ee1 --- /dev/null +++ b/.github/NAMESPACE-REVIEW-PROCESS.md @@ -0,0 +1,101 @@ +# Namespace Approval Process + +## Overview + +When a service team adds or modifies SDK namespace configuration in their `tspconfig.yaml`, the namespace must be approved by authorized architects before the spec PR can merge. + +This process is automated via GitHub Actions on this repository. + +## How It Works + +### Detection + +When a PR modifies any `tspconfig.yaml` file, the workflow: + +1. Identifies which languages have namespace/package-name configuration +2. Extracts namespaces using the `@azure-tools/typespec-metadata` emitter (primary) with YAML parsing fallback +3. Determines if the service is data-plane or management-plane +4. Applies `-namespace-pending` labels for each detected language + +### Approval + +- **Data plane:** Each language architect approves their language by applying `-namespace-approved` +- **Management plane:** ArthurMa1978 or m-nash can apply `namespace-approved-all` to approve all languages at once + +### Status Check + +A required status check (`namespace-approval`) blocks merge until all pending namespaces are approved. The check: + +- ✅ Passes when all `*-namespace-pending` labels have corresponding `*-namespace-approved` labels +- ❌ Fails when any pending namespace lacks approval +- ⏭️ Skipped when the PR doesn't modify tspconfig.yaml (no namespace changes) + +### Reset on Change + +If a service team pushes a commit that **changes** a namespace value (not just TypeSpec code), the approval for that language is reset: + +- The `-namespace-approved` label is removed +- The `-namespace-pending` label is re-applied +- The status check fails again until re-approved + +TypeSpec-only changes (no namespace modification) do **not** reset approval. + +## Labels + +| Label | Applied by | Meaning | +| --------------------------- | ---------------- | ---------------------------------------- | +| `-namespace-pending` | Bot | Namespace detected, awaiting approval | +| `-namespace-approved` | Architect | Namespace approved for this language | +| `namespace-approved-all` | Architect (mgmt) | Shortcut: approves all pending languages | +| `Mgmt` | Bot | PR is management plane | + +Where `` is one of: `dotnet`, `java`, `python`, `typescript`, `go`, `cpp`, `rust` + +## Authorized Approvers + +Defined in [`.github/namespace-approvers.yml`](../.github/namespace-approvers.yml): + + + +| Scope | Approvers | +| -------------------- | ----------------------------- | +| .NET | @jsquire, @m-nash | +| Java | @JonathanGiles, @alzimmermsft | +| Python | @xirzec, @kashifkhan | +| TypeScript | @xirzec, @maorleger | +| Go | @jhendrixMSFT | +| C++ | @LarryOsterman | +| Rust | @heaths, @LarryOsterman | +| Mgmt (all languages) | @ArthurMa1978, @m-nash | + + + +## For Service Teams + +No action needed beyond your normal workflow: + +1. Add/modify namespace in your `tspconfig.yaml` +2. Open a PR as usual +3. The bot will apply pending labels and post a status table +4. Wait for architects to approve — they're notified via labels +5. Once approved, the status check passes and the PR can merge + +## For Architects + +1. Watch for PRs with `-namespace-pending` labels +2. Review the proposed namespace in the PR diff +3. If acceptable, apply `-namespace-approved` label +4. For management plane PRs, you can use `namespace-approved-all` to approve all at once + +## Configuration + +To update approvers, submit a PR modifying `.github/namespace-approvers.yml`. Changes take effect immediately on merge. + +## Relationship to API Review + +Namespace approval is **separate from API review**: + +- **Namespace approval** (this repo): gates spec PR merge, ensures library naming is correct +- **API review** (azure-sdk repo): reviews API surface design (methods, models, parameters) + +A new service typically needs namespace approval first (on their spec PR), then API review later (when SDK is built). diff --git a/.github/namespace-approvers.yml b/.github/namespace-approvers.yml new file mode 100644 index 000000000000..3cf4eafd0854 --- /dev/null +++ b/.github/namespace-approvers.yml @@ -0,0 +1,31 @@ +# cspell:words jsquire alzimmermsft xirzec kashifkhan maorleger jhendrixMSFT LarryOsterman heaths ArthurMa1978 +# Namespace Approvers Configuration +# This file defines who can approve namespace changes per language. +# To update, submit a PR to this file. In the future, this may be +# synced automatically from ARCHITECTS files in each language repo. + +data-plane: + dotnet: + - jsquire + - m-nash + java: + - JonathanGiles + - alzimmermsft + python: + - xirzec + - kashifkhan + typescript: + - xirzec + - maorleger + go: + - jhendrixMSFT + cpp: + - LarryOsterman + rust: + - heaths + - LarryOsterman + +management-plane: + all: + - ArthurMa1978 + - m-nash diff --git a/.github/workflows/namespace-approval.yml b/.github/workflows/namespace-approval.yml new file mode 100644 index 000000000000..d133aa216c7c --- /dev/null +++ b/.github/workflows/namespace-approval.yml @@ -0,0 +1,102 @@ +name: Namespace Approval + +on: + pull_request: + types: + - opened + - synchronize + - labeled + - unlabeled + +permissions: + pull-requests: write + contents: read + +jobs: + detect-namespaces: + if: github.event.action == 'opened' || github.event.action == 'synchronize' + name: Detect Namespace Changes + + runs-on: ubuntu-24.04 + + outputs: + has_pending: ${{ steps.detect.outputs.has_pending }} + + steps: + - uses: actions/checkout@v6 + with: + fetch-depth: 0 + + - name: Setup Node and install dependencies + uses: ./.github/actions/setup-node-install-deps + + - name: Install dependencies for github-script actions + uses: ./.github/actions/install-deps-github-script + + - id: detect + name: Detect namespace changes + uses: actions/github-script@v8 + with: + script: | + const { default: detectNamespaces } = + await import('${{ github.workspace }}/.github/workflows/src/namespace-approval/detect-namespaces.js'); + await detectNamespaces({ github, context, core }); + + validate-approval: + if: github.event.action == 'labeled' + name: Validate Namespace Approval + + runs-on: ubuntu-24.04 + + steps: + - uses: actions/checkout@v6 + with: + ref: ${{ github.event.pull_request.base.ref }} + sparse-checkout: | + .github + + - name: Install dependencies for github-script actions + uses: ./.github/actions/install-deps-github-script + + - name: Validate namespace approval label + uses: actions/github-script@v8 + with: + script: | + const { default: validateApproval } = + await import('${{ github.workspace }}/.github/workflows/src/namespace-approval/validate-approval.js'); + await validateApproval({ github, context, core }); + + status-check: + name: Namespace Approval Status + needs: [detect-namespaces, validate-approval] + if: always() + + runs-on: ubuntu-24.04 + + steps: + - name: Verify namespace approval status + uses: actions/github-script@v8 + with: + script: | + const { data: pr } = await github.rest.pulls.get({ + owner: context.repo.owner, + repo: context.repo.repo, + pull_number: context.payload.pull_request.number, + }); + const labels = pr.labels.map((l) => l.name); + + if (!labels.includes("namespace-review-required")) { + core.info("No namespace review required, passing"); + return; + } + + const pendingLabels = labels.filter((l) => l.endsWith("-namespace-pending")); + if (pendingLabels.length > 0) { + core.setFailed( + `Namespace approval pending for: ${pendingLabels + .map((l) => l.replace("-namespace-pending", "")) + .join(", ")}`, + ); + } else if (labels.includes("namespace-approved")) { + core.info("All namespaces approved - merge allowed"); + } \ No newline at end of file diff --git a/.github/workflows/src/namespace-approval/detect-namespaces.js b/.github/workflows/src/namespace-approval/detect-namespaces.js new file mode 100644 index 000000000000..10c31c6d0eed --- /dev/null +++ b/.github/workflows/src/namespace-approval/detect-namespaces.js @@ -0,0 +1,332 @@ +import { execSync } from "child_process"; +import { existsSync, readFileSync, rmSync } from "fs"; +import yaml from "js-yaml"; +import { dirname, join } from "path"; + +/** + * @typedef {Object} ApproversConfig + * @property {Record} [data-plane] + * @property {{ all?: string[] }} [management-plane] + */ + +/** + * @typedef {Object} TypeSpecMetadataEntry + * @property {string} [namespace] + * @property {string} [packageName] + */ + +/** + * @typedef {Object} TypeSpecMetadata + * @property {{ type?: string }} [typespec] + * @property {Record} [languages] + */ + +/** + * @typedef {Object} TspConfigEmitterOptions + * @property {string} [namespace] + * @property {{ name?: string }} [package-details] + * @property {string} [module] + */ + +/** + * @typedef {Object} TspConfig + * @property {Record} [options] + */ + +/** @type {Record} */ +const EMITTER_TO_LANG = { + csharp: "dotnet", + "http-client-csharp": "dotnet", + "http-client-csharp-mgmt": "dotnet", + java: "java", + python: "python", + typescript: "typescript", + go: "go", +}; + +/** @type {Record} */ +const FALLBACK_EMITTER_MAP = { + "@azure-typespec/http-client-csharp": "dotnet", + "@azure-typespec/http-client-csharp-mgmt": "dotnet", + "@azure-tools/typespec-csharp": "dotnet", + "@azure-tools/typespec-java": "java", + "@azure-tools/typespec-python": "python", + "@azure-tools/typespec-ts": "typescript", + "@azure-tools/typespec-go": "go", +}; + +/** + * Load approvers configuration from YAML file. + * + * @param {import("@actions/core")} core + * @returns {ApproversConfig | null} + */ +function loadApproversConfig(core) { + try { + const content = readFileSync(".github/namespace-approvers.yml", "utf8"); + return /** @type {ApproversConfig} */ (yaml.load(content)); + } catch (e) { + core.setFailed("Failed to load .github/namespace-approvers.yml: " + String(e)); + return null; + } +} + +/** + * Extract namespaces from tspconfig.yaml using typespec-metadata emitter with YAML fallback. + * + * @param {string} file - Path to tspconfig.yaml + * @param {Record} namespacesFound - Map of language to namespace (mutated) + * @param {import("@actions/core")} core + * @returns {{ isMgmt: boolean, isDataPlane: boolean }} + */ +function extractNamespaces(file, namespacesFound, core) { + let isMgmt = false; + let isDataPlane = false; + + if (!existsSync(file)) return { isMgmt, isDataPlane }; + + const tspDir = dirname(file); + + if (file.includes(".Management/") || file.includes("/resource-manager/")) { + isMgmt = true; + } else { + isDataPlane = true; + } + + // Try typespec-metadata emitter first + try { + core.info(`Running typespec-metadata emitter on ${tspDir}`); + execSync( + `npx tsp compile "${tspDir}" --emit "@azure-tools/typespec-metadata" --output-dir "${tspDir}" --option "@azure-tools/typespec-metadata.format=json"`, + { stdio: "pipe", timeout: 120000 }, + ); + + const metadataPath = join( + tspDir, + "@azure-tools", + "typespec-metadata", + "typespec-metadata.json", + ); + if (!existsSync(metadataPath)) throw new Error("metadata file not found"); + + const metadataRaw = /** @type {unknown} */ (JSON.parse(readFileSync(metadataPath, "utf8"))); + const metadata = /** @type {TypeSpecMetadata} */ (metadataRaw); + + if (metadata.typespec?.type === "management") isMgmt = true; + if (metadata.typespec?.type === "data") isDataPlane = true; + + const languages = metadata.languages ?? {}; + for (const [langKey, entries] of Object.entries(languages)) { + const lang = EMITTER_TO_LANG[langKey] ?? langKey; + if (entries.length > 0) { + const firstEntry = entries[0]; + const ns = firstEntry.namespace ?? firstEntry.packageName; + if (ns) namespacesFound[lang] = ns; + } + } + + try { + rmSync(join(tspDir, "@azure-tools"), { recursive: true }); + } catch { + // ignore cleanup errors + } + } catch (e) { + core.warning(`typespec-metadata emitter failed: ${String(e)}. Falling back to YAML parse.`); + const content = readFileSync(file, "utf8"); + const parsed = /** @type {TspConfig} */ (yaml.load(content)); + if (!parsed?.options) return { isMgmt, isDataPlane }; + + for (const [emitter, lang] of Object.entries(FALLBACK_EMITTER_MAP)) { + const opts = parsed.options[emitter]; + if (!opts) continue; + if (opts.namespace) namespacesFound[lang] = opts.namespace; + else if (opts["package-details"]?.name) namespacesFound[lang] = opts["package-details"].name; + else if (opts.module) namespacesFound[lang] = opts.module; + } + } + + return { isMgmt, isDataPlane }; +} + +/** + * Detect namespace changes in a PR and apply pending labels. + * + * @param {import("@actions/github-script").AsyncFunctionArguments} args + */ +export default async function detectNamespaces({ github, context, core }) { + const approversConfig = loadApproversConfig(core); + if (!approversConfig) return; + + const payload = /** @type {import("@octokit/webhooks-types").PullRequestEvent} */ ( + context.payload + ); + + const baseSha = payload.pull_request.base.sha; + const headSha = payload.pull_request.head.sha; + const diffOutput = execSync(`git diff --name-only ${baseSha}...${headSha}`).toString().trim(); + const changedFiles = diffOutput + .split("\n") + .filter((f) => /specification\/.*\/tspconfig\.yaml$/.test(f)); + + if (changedFiles.length === 0) { + core.info("No tspconfig.yaml changes detected, skipping"); + return; + } + + core.info(`Found tspconfig.yaml changes: ${changedFiles.join(", ")}`); + + /** @type {Record} */ + const namespacesFound = {}; + let isMgmt = false; + let isDataPlane = false; + + for (const file of changedFiles) { + const result = extractNamespaces(file, namespacesFound, core); + if (result.isMgmt) isMgmt = true; + if (result.isDataPlane) isDataPlane = true; + } + + const prNumber = payload.pull_request.number; + + // On synchronize: reset approvals for changed namespaces + if (payload.action === "synchronize") { + const { data: currentPR } = await github.rest.pulls.get({ + owner: context.repo.owner, + repo: context.repo.repo, + pull_number: prNumber, + }); + /** @type {string[]} */ + const existingLabels = currentPR.labels.map((l) => l.name ?? ""); + + for (const lang of Object.keys(namespacesFound)) { + const approvedLabel = `${lang}-namespace-approved`; + if (existingLabels.includes(approvedLabel)) { + core.info(`Namespace changed for ${lang}, resetting approval`); + try { + await github.rest.issues.removeLabel({ + owner: context.repo.owner, + repo: context.repo.repo, + issue_number: prNumber, + name: approvedLabel, + }); + } catch { + // label may not exist + } + } + } + + for (const label of ["namespace-approved-all", "namespace-approved"]) { + if (existingLabels.includes(label)) { + try { + await github.rest.issues.removeLabel({ + owner: context.repo.owner, + repo: context.repo.repo, + issue_number: prNumber, + name: label, + }); + } catch { + // label may not exist + } + } + } + } + + const languages = Object.keys(namespacesFound); + if (languages.length === 0) { + core.info("No namespace changes detected"); + return; + } + + // Apply labels + /** @type {string[]} */ + const labelsToAdd = ["namespace-review-required"]; + if (isMgmt) labelsToAdd.push("Mgmt"); + if (isDataPlane) labelsToAdd.push("data-plane"); + for (const lang of languages) labelsToAdd.push(`${lang}-namespace-pending`); + + // cspell:words fbbf + for (const label of labelsToAdd) { + try { + await github.rest.issues.getLabel({ + owner: context.repo.owner, + repo: context.repo.repo, + name: label, + }); + } catch { + /** @type {Record} */ + const colors = { "namespace-review-required": "e11d48" }; + await github.rest.issues.createLabel({ + owner: context.repo.owner, + repo: context.repo.repo, + name: label, + color: colors[label] ?? (label.includes("pending") ? "fbbf24" : "94a3b8"), + }); + } + } + + await github.rest.issues.addLabels({ + owner: context.repo.owner, + repo: context.repo.repo, + issue_number: prNumber, + labels: labelsToAdd, + }); + + // Build bot comment + /** + * @param {string} lang + * @returns {string[]} + */ + const getApprovers = (lang) => { + if (isMgmt) { + const mgmtApprovers = approversConfig["management-plane"]?.all; + if (mgmtApprovers) return mgmtApprovers; + } + return approversConfig["data-plane"]?.[lang] ?? ["TBD"]; + }; + + const planeType = isMgmt ? "Management Plane" : "Data Plane"; + const baseRef = payload.pull_request.base.ref; + let body = `## Namespace Review Required\n\n**Plane:** ${planeType}\n\n`; + body += `| Language | Proposed Namespace | Status | Approvers |\n`; + body += `|----------|-------------------|--------|----------|\n`; + for (const [lang, ns] of Object.entries(namespacesFound)) { + body += `| ${lang} | \`${ns}\` | ⏳ Pending | ${getApprovers(lang).join(", ")} |\n`; + } + body += `\n**How to approve:**\n`; + body += `- Per language: apply \`-namespace-approved\` label\n`; + body += `- All at once: apply \`namespace-approved-all\` label (shortcut for mgmt plane)\n\n`; + body += `Merge is blocked until all languages are approved.\n`; + if (payload.action === "synchronize") { + body += `\n> ⚠️ **Namespace changed** — approvals for affected languages have been reset.\n`; + } + body += `\n_Approver list: [.github/namespace-approvers.yml](../blob/${baseRef}/.github/namespace-approvers.yml)_\n`; + body += `_Namespaces extracted via [@azure-tools/typespec-metadata](https://www.npmjs.com/package/@azure-tools/typespec-metadata) emitter_\n`; + body += ``; + + const comments = await github.rest.issues.listComments({ + owner: context.repo.owner, + repo: context.repo.repo, + issue_number: prNumber, + }); + const botComment = comments.data.find( + (c) => c.user?.type === "Bot" && c.body?.includes(""), + ); + + if (botComment) { + await github.rest.issues.updateComment({ + owner: context.repo.owner, + repo: context.repo.repo, + comment_id: botComment.id, + body, + }); + } else { + await github.rest.issues.createComment({ + owner: context.repo.owner, + repo: context.repo.repo, + issue_number: prNumber, + body, + }); + } + + core.setOutput("has_pending", "true"); +} diff --git a/.github/workflows/src/namespace-approval/status-check.js b/.github/workflows/src/namespace-approval/status-check.js new file mode 100644 index 000000000000..ef0f77f657d1 --- /dev/null +++ b/.github/workflows/src/namespace-approval/status-check.js @@ -0,0 +1,29 @@ +/** + * Verify namespace approval status and set check result. + * + * @param {import("@actions/github-script").AsyncFunctionArguments} args + */ +export default async function statusCheck({ github, context, core }) { + const { data: pr } = await github.rest.pulls.get({ + owner: context.repo.owner, + repo: context.repo.repo, + pull_number: /** @type {import("@octokit/webhooks-types").PullRequestEvent} */ (context.payload) + .pull_request.number, + }); + /** @type {string[]} */ + const labels = pr.labels.map((l) => l.name ?? ""); + + if (!labels.includes("namespace-review-required")) { + core.info("No namespace review required, passing"); + return; + } + + const pendingLabels = labels.filter((l) => l.endsWith("-namespace-pending")); + if (pendingLabels.length > 0) { + core.setFailed( + `Namespace approval pending for: ${pendingLabels.map((l) => l.replace("-namespace-pending", "")).join(", ")}`, + ); + } else if (labels.includes("namespace-approved")) { + core.info("All namespaces approved - merge allowed"); + } +} diff --git a/.github/workflows/src/namespace-approval/validate-approval.js b/.github/workflows/src/namespace-approval/validate-approval.js new file mode 100644 index 000000000000..621a39925d9c --- /dev/null +++ b/.github/workflows/src/namespace-approval/validate-approval.js @@ -0,0 +1,220 @@ +import { readFileSync } from "fs"; +import yaml from "js-yaml"; + +/** + * @typedef {Object} ApproversConfig + * @property {Record} [data-plane] + * @property {{ all?: string[] }} [management-plane] + */ + +/** + * Validate that a namespace approval label was added by an authorized approver. + * + * @param {import("@actions/github-script").AsyncFunctionArguments} args + */ +export default async function validateApproval({ github, context, core }) { + /** @type {ApproversConfig} */ + let approversConfig; + try { + const content = readFileSync(".github/namespace-approvers.yml", "utf8"); + approversConfig = /** @type {ApproversConfig} */ (yaml.load(content)); + } catch (e) { + core.setFailed("Failed to load .github/namespace-approvers.yml: " + String(e)); + return; + } + + const payload = /** @type {import("@octokit/webhooks-types").PullRequestLabeledEvent} */ ( + context.payload + ); + + const prNumber = payload.pull_request.number; + /** @type {string[]} */ + const labels = payload.pull_request.labels.map((l) => l.name); + + if (!labels.includes("namespace-review-required")) { + core.info("No namespace review, skipping"); + return; + } + + const addedLabel = payload.label.name; + const isMgmt = labels.includes("Mgmt"); + const actor = payload.sender.login; + + /** @type {string[]} */ + let langsToApprove; + + if (addedLabel === "namespace-approved-all") { + // Shortcut: approve all pending languages at once + const mgmtApprovers = approversConfig["management-plane"]?.all ?? []; + /** @type {string[]} */ + const allApprovers = [ + ...new Set([...mgmtApprovers, ...Object.values(approversConfig["data-plane"] ?? {}).flat()]), + ]; + + if (!allApprovers.includes(actor)) { + await github.rest.issues.removeLabel({ + owner: context.repo.owner, + repo: context.repo.repo, + issue_number: prNumber, + name: addedLabel, + }); + await github.rest.issues.createComment({ + owner: context.repo.owner, + repo: context.repo.repo, + issue_number: prNumber, + body: `⚠️ @${actor} is not authorized to use \`namespace-approved-all\`. This shortcut is available to authorized namespace approvers.\n\nLabel removed.`, + }); + return; + } + + langsToApprove = labels + .filter((l) => l.endsWith("-namespace-pending")) + .map((l) => l.replace("-namespace-pending", "")); + } else { + const match = addedLabel.match(/^(\w+)-namespace-approved$/); + if (!match) { + core.info(`${addedLabel} is not a namespace approval label`); + return; + } + + const lang = match[1]; + /** @type {string[]} */ + let authorizedList; + if (isMgmt && approversConfig["management-plane"]?.all) { + authorizedList = approversConfig["management-plane"].all; + } else { + authorizedList = approversConfig["data-plane"]?.[lang] ?? []; + } + + if (!authorizedList.includes(actor)) { + await github.rest.issues.removeLabel({ + owner: context.repo.owner, + repo: context.repo.repo, + issue_number: prNumber, + name: addedLabel, + }); + await github.rest.issues.createComment({ + owner: context.repo.owner, + repo: context.repo.repo, + issue_number: prNumber, + body: `⚠️ @${actor} is not authorized to approve **${lang}** namespace. Authorized approvers: ${authorizedList.join(", ")}.\n\nLabel \`${addedLabel}\` has been removed.`, + }); + return; + } + + langsToApprove = [lang]; + } + + core.info(`Approving languages: ${langsToApprove.join(", ")} by ${actor}`); + + // Apply per-language approved labels and remove pending labels + for (const lang of langsToApprove) { + try { + await github.rest.issues.removeLabel({ + owner: context.repo.owner, + repo: context.repo.repo, + issue_number: prNumber, + name: `${lang}-namespace-pending`, + }); + } catch { + // label may not exist + } + + if (addedLabel === "namespace-approved-all") { + const approvedLabel = `${lang}-namespace-approved`; + try { + await github.rest.issues.getLabel({ + owner: context.repo.owner, + repo: context.repo.repo, + name: approvedLabel, + }); + } catch { + await github.rest.issues.createLabel({ + owner: context.repo.owner, + repo: context.repo.repo, + name: approvedLabel, + color: "22c55e", + }); + } + await github.rest.issues.addLabels({ + owner: context.repo.owner, + repo: context.repo.repo, + issue_number: prNumber, + labels: [approvedLabel], + }); + } + } + + // Update bot comment + const { data: currentPR } = await github.rest.pulls.get({ + owner: context.repo.owner, + repo: context.repo.repo, + pull_number: prNumber, + }); + /** @type {string[]} */ + const currentLabels = currentPR.labels.map((l) => l.name ?? ""); + const pendingLabels = currentLabels.filter((l) => l.endsWith("-namespace-pending")); + + const comments = await github.rest.issues.listComments({ + owner: context.repo.owner, + repo: context.repo.repo, + issue_number: prNumber, + }); + const botComment = comments.data.find( + (c) => c.user?.type === "Bot" && c.body?.includes(""), + ); + + if (botComment?.body) { + let body = botComment.body; + for (const lang of langsToApprove) { + const rowRegex = new RegExp(`(\\| ${lang} \\|[^|]+\\|) ⏳ Pending (\\|)`, "i"); + body = body.replace(rowRegex, `$1 ✅ Approved by @${actor} $2`); + } + await github.rest.issues.updateComment({ + owner: context.repo.owner, + repo: context.repo.repo, + comment_id: botComment.id, + body, + }); + } + + if (pendingLabels.length === 0) { + core.info("All namespaces approved!"); + try { + await github.rest.issues.removeLabel({ + owner: context.repo.owner, + repo: context.repo.repo, + issue_number: prNumber, + name: "namespace-review-required", + }); + } catch { + // label may not exist + } + try { + await github.rest.issues.getLabel({ + owner: context.repo.owner, + repo: context.repo.repo, + name: "namespace-approved", + }); + } catch { + await github.rest.issues.createLabel({ + owner: context.repo.owner, + repo: context.repo.repo, + name: "namespace-approved", + color: "22c55e", + }); + } + await github.rest.issues.addLabels({ + owner: context.repo.owner, + repo: context.repo.repo, + issue_number: prNumber, + labels: ["namespace-approved"], + }); + await github.rest.issues.createComment({ + owner: context.repo.owner, + repo: context.repo.repo, + issue_number: prNumber, + body: "## ✅ Namespace Approved\n\nAll required namespace approvals received. This PR is clear to merge from a namespace perspective.", + }); + } +} From 4f28a24e7df6b9fe5df001cbdf762d3c2b2a0e28 Mon Sep 17 00:00:00 2001 From: Sameeksha Vaity Date: Mon, 22 Jun 2026 12:04:16 -0700 Subject: [PATCH 02/23] Address review feedback: unlabeled guard, remove C++/Rust - Add unlabeled handler: re-applies pending labels if removed by unauthorized user (non-approver, non-bot) - Allow authorized approvers and bots (github-actions, azure-sdk) to remove namespace labels - Remove C++/Rust from approvers config and process docs (no emitter config exists for these in tspconfig.yaml) - Update workflow to trigger on unlabeled events --- .github/NAMESPACE-REVIEW-PROCESS.md | 4 +- .github/namespace-approvers.yml | 7 +- .github/workflows/namespace-approval.yml | 4 +- .../namespace-approval/validate-approval.js | 186 ++++++++++++++---- 4 files changed, 157 insertions(+), 44 deletions(-) diff --git a/.github/NAMESPACE-REVIEW-PROCESS.md b/.github/NAMESPACE-REVIEW-PROCESS.md index 989aa03f7ee1..fcd84adb3838 100644 --- a/.github/NAMESPACE-REVIEW-PROCESS.md +++ b/.github/NAMESPACE-REVIEW-PROCESS.md @@ -49,7 +49,7 @@ TypeSpec-only changes (no namespace modification) do **not** reset approval. | `namespace-approved-all` | Architect (mgmt) | Shortcut: approves all pending languages | | `Mgmt` | Bot | PR is management plane | -Where `` is one of: `dotnet`, `java`, `python`, `typescript`, `go`, `cpp`, `rust` +Where `` is one of: `dotnet`, `java`, `python`, `typescript`, `go` ## Authorized Approvers @@ -64,8 +64,6 @@ Defined in [`.github/namespace-approvers.yml`](../.github/namespace-approvers.ym | Python | @xirzec, @kashifkhan | | TypeScript | @xirzec, @maorleger | | Go | @jhendrixMSFT | -| C++ | @LarryOsterman | -| Rust | @heaths, @LarryOsterman | | Mgmt (all languages) | @ArthurMa1978, @m-nash | diff --git a/.github/namespace-approvers.yml b/.github/namespace-approvers.yml index 3cf4eafd0854..7fba1d068b89 100644 --- a/.github/namespace-approvers.yml +++ b/.github/namespace-approvers.yml @@ -1,4 +1,4 @@ -# cspell:words jsquire alzimmermsft xirzec kashifkhan maorleger jhendrixMSFT LarryOsterman heaths ArthurMa1978 +# cspell:words jsquire alzimmermsft xirzec kashifkhan maorleger jhendrixMSFT ArthurMa1978 # Namespace Approvers Configuration # This file defines who can approve namespace changes per language. # To update, submit a PR to this file. In the future, this may be @@ -19,11 +19,6 @@ data-plane: - maorleger go: - jhendrixMSFT - cpp: - - LarryOsterman - rust: - - heaths - - LarryOsterman management-plane: all: diff --git a/.github/workflows/namespace-approval.yml b/.github/workflows/namespace-approval.yml index d133aa216c7c..bd377ee95ab3 100644 --- a/.github/workflows/namespace-approval.yml +++ b/.github/workflows/namespace-approval.yml @@ -43,7 +43,7 @@ jobs: await detectNamespaces({ github, context, core }); validate-approval: - if: github.event.action == 'labeled' + if: github.event.action == 'labeled' || github.event.action == 'unlabeled' name: Validate Namespace Approval runs-on: ubuntu-24.04 @@ -99,4 +99,4 @@ jobs: ); } else if (labels.includes("namespace-approved")) { core.info("All namespaces approved - merge allowed"); - } \ No newline at end of file + } diff --git a/.github/workflows/src/namespace-approval/validate-approval.js b/.github/workflows/src/namespace-approval/validate-approval.js index 621a39925d9c..c977525af9eb 100644 --- a/.github/workflows/src/namespace-approval/validate-approval.js +++ b/.github/workflows/src/namespace-approval/validate-approval.js @@ -8,55 +8,113 @@ import yaml from "js-yaml"; */ /** - * Validate that a namespace approval label was added by an authorized approver. + * @typedef {Object} ValidateContext + * @property {import("@actions/github-script").AsyncFunctionArguments["github"]} github + * @property {import("@actions/github-script").AsyncFunctionArguments["context"]} context + * @property {import("@actions/github-script").AsyncFunctionArguments["core"]} core + * @property {ApproversConfig} approversConfig + * @property {string} targetLabel + * @property {string} actor + * @property {number} prNumber + * @property {boolean} isMgmt + */ + +/** @type {string[]} */ +const ALLOWED_BOT_LOGINS = ["github-actions[bot]", "azure-sdk"]; + +/** + * Get all authorized approvers as a flat list. * - * @param {import("@actions/github-script").AsyncFunctionArguments} args + * @param {ApproversConfig} approversConfig + * @returns {string[]} */ -export default async function validateApproval({ github, context, core }) { - /** @type {ApproversConfig} */ - let approversConfig; - try { - const content = readFileSync(".github/namespace-approvers.yml", "utf8"); - approversConfig = /** @type {ApproversConfig} */ (yaml.load(content)); - } catch (e) { - core.setFailed("Failed to load .github/namespace-approvers.yml: " + String(e)); +function getAllApprovers(approversConfig) { + const mgmtApprovers = approversConfig["management-plane"]?.all ?? []; + return [ + ...new Set([...mgmtApprovers, ...Object.values(approversConfig["data-plane"] ?? {}).flat()]), + ]; +} + +/** + * Handle unlabeled event: re-apply pending labels if removed by unauthorized user. + * + * @param {ValidateContext} ctx + */ +async function handleUnlabeled({ + github, + context, + core, + approversConfig, + targetLabel, + actor, + prNumber, +}) { + // Only guard namespace-related labels + if (!targetLabel.endsWith("-namespace-pending") && targetLabel !== "namespace-review-required") { + core.info(`${targetLabel} is not a namespace label, skipping`); return; } - const payload = /** @type {import("@octokit/webhooks-types").PullRequestLabeledEvent} */ ( - context.payload - ); + // Allow bots and authorized approvers to remove labels + if (ALLOWED_BOT_LOGINS.includes(actor)) { + core.info(`${actor} is a trusted bot, allowing label removal`); + return; + } - const prNumber = payload.pull_request.number; /** @type {string[]} */ - const labels = payload.pull_request.labels.map((l) => l.name); - - if (!labels.includes("namespace-review-required")) { - core.info("No namespace review, skipping"); + const allApprovers = getAllApprovers(approversConfig); + if (allApprovers.includes(actor)) { + core.info(`${actor} is an authorized approver, allowing label removal`); return; } - const addedLabel = payload.label.name; - const isMgmt = labels.includes("Mgmt"); - const actor = payload.sender.login; + // Unauthorized removal — re-apply the label + core.warning(`${actor} is not authorized to remove ${targetLabel}, re-applying`); + + await github.rest.issues.addLabels({ + owner: context.repo.owner, + repo: context.repo.repo, + issue_number: prNumber, + labels: [targetLabel], + }); + + await github.rest.issues.createComment({ + owner: context.repo.owner, + repo: context.repo.repo, + issue_number: prNumber, + body: `⚠️ @${actor} is not authorized to remove \`${targetLabel}\`. Only authorized namespace approvers can remove namespace labels.\n\nLabel has been re-applied.`, + }); +} +/** + * Handle labeled event: validate approval labels from authorized approvers. + * + * @param {ValidateContext & { labels: string[] }} ctx + */ +async function handleLabeled({ + github, + context, + core, + approversConfig, + targetLabel, + actor, + prNumber, + isMgmt, + labels, +}) { /** @type {string[]} */ let langsToApprove; - if (addedLabel === "namespace-approved-all") { - // Shortcut: approve all pending languages at once - const mgmtApprovers = approversConfig["management-plane"]?.all ?? []; + if (targetLabel === "namespace-approved-all") { /** @type {string[]} */ - const allApprovers = [ - ...new Set([...mgmtApprovers, ...Object.values(approversConfig["data-plane"] ?? {}).flat()]), - ]; + const allApprovers = getAllApprovers(approversConfig); if (!allApprovers.includes(actor)) { await github.rest.issues.removeLabel({ owner: context.repo.owner, repo: context.repo.repo, issue_number: prNumber, - name: addedLabel, + name: targetLabel, }); await github.rest.issues.createComment({ owner: context.repo.owner, @@ -71,9 +129,9 @@ export default async function validateApproval({ github, context, core }) { .filter((l) => l.endsWith("-namespace-pending")) .map((l) => l.replace("-namespace-pending", "")); } else { - const match = addedLabel.match(/^(\w+)-namespace-approved$/); + const match = targetLabel.match(/^(\w+)-namespace-approved$/); if (!match) { - core.info(`${addedLabel} is not a namespace approval label`); + core.info(`${targetLabel} is not a namespace approval label`); return; } @@ -91,13 +149,13 @@ export default async function validateApproval({ github, context, core }) { owner: context.repo.owner, repo: context.repo.repo, issue_number: prNumber, - name: addedLabel, + name: targetLabel, }); await github.rest.issues.createComment({ owner: context.repo.owner, repo: context.repo.repo, issue_number: prNumber, - body: `⚠️ @${actor} is not authorized to approve **${lang}** namespace. Authorized approvers: ${authorizedList.join(", ")}.\n\nLabel \`${addedLabel}\` has been removed.`, + body: `⚠️ @${actor} is not authorized to approve **${lang}** namespace. Authorized approvers: ${authorizedList.join(", ")}.\n\nLabel \`${targetLabel}\` has been removed.`, }); return; } @@ -120,7 +178,7 @@ export default async function validateApproval({ github, context, core }) { // label may not exist } - if (addedLabel === "namespace-approved-all") { + if (targetLabel === "namespace-approved-all") { const approvedLabel = `${lang}-namespace-approved`; try { await github.rest.issues.getLabel({ @@ -218,3 +276,65 @@ export default async function validateApproval({ github, context, core }) { }); } } + +/** + * Validate namespace label changes by authorized approvers. + * Handles both labeled (approval) and unlabeled (guard against unauthorized removal). + * + * @param {import("@actions/github-script").AsyncFunctionArguments} args + */ +export default async function validateApproval({ github, context, core }) { + /** @type {ApproversConfig} */ + let approversConfig; + try { + const content = readFileSync(".github/namespace-approvers.yml", "utf8"); + approversConfig = /** @type {ApproversConfig} */ (yaml.load(content)); + } catch (e) { + core.setFailed("Failed to load .github/namespace-approvers.yml: " + String(e)); + return; + } + + const payload = /** @type {import("@octokit/webhooks-types").PullRequestLabeledEvent} */ ( + context.payload + ); + + const prNumber = payload.pull_request.number; + /** @type {string[]} */ + const labels = payload.pull_request.labels.map((l) => l.name); + const targetLabel = payload.label.name; + const actor = payload.sender.login; + const isMgmt = labels.includes("Mgmt"); + + // Handle unlabeled: guard against unauthorized removal of pending/required labels + if (payload.action === "unlabeled") { + return await handleUnlabeled({ + github, + context, + core, + approversConfig, + targetLabel, + actor, + prNumber, + isMgmt, + }); + } + + // Skip if no namespace review is required + if (!labels.includes("namespace-review-required")) { + core.info("No namespace review, skipping"); + return; + } + + // Handle labeled: validate approval labels + return await handleLabeled({ + github, + context, + core, + approversConfig, + targetLabel, + actor, + prNumber, + isMgmt, + labels, + }); +} From 37aa3d19a7c8476754fc634e42cb5db2e8cd8c40 Mon Sep 17 00:00:00 2001 From: Sameeksha Vaity Date: Mon, 22 Jun 2026 12:44:10 -0700 Subject: [PATCH 03/23] Add unit tests and namespace format validation - Add 33 unit tests for validate-approval, status-check, and validate-format - Add validate-format.js module with per-language regex validation - Add namespace-format-rules.yml config (Arthur's regex patterns) - Integrate format validation into detect-namespaces.js bot comment - All ESLint and Prettier checks passing Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- .github/namespace-format-rules.yml | 31 ++ .../namespace-approval/detect-namespaces.js | 29 +- .../namespace-approval/validate-approval.js | 7 +- .../src/namespace-approval/validate-format.js | 98 ++++++ .../namespace-approval/status-check.test.js | 75 +++++ .../validate-approval.test.js | 316 ++++++++++++++++++ .../validate-format.test.js | 161 +++++++++ 7 files changed, 711 insertions(+), 6 deletions(-) create mode 100644 .github/namespace-format-rules.yml create mode 100644 .github/workflows/src/namespace-approval/validate-format.js create mode 100644 .github/workflows/test/namespace-approval/status-check.test.js create mode 100644 .github/workflows/test/namespace-approval/validate-approval.test.js create mode 100644 .github/workflows/test/namespace-approval/validate-format.test.js diff --git a/.github/namespace-format-rules.yml b/.github/namespace-format-rules.yml new file mode 100644 index 000000000000..41432b698bf4 --- /dev/null +++ b/.github/namespace-format-rules.yml @@ -0,0 +1,31 @@ +# Namespace Format Rules +# These regex patterns validate namespace/package naming conventions per language. +# Namespaces that fail validation are flagged in the bot comment before manual review. +# +# To update: submit a PR modifying the patterns below. +# Each language can have one rule or multiple rules (array). +# Each rule has a `pattern` (regex) and optional `description`. +# +# Maintainer: @ArthurMa1978 + +dotnet: + pattern: '^Azure\.ResourceManager\.(?:[A-Z][a-zA-Z0-9]*)(?:\.[A-Z][a-zA-Z0-9]*)?$' + description: "Azure.ResourceManager.{Name} or Azure.ResourceManager.{SuperService}.{Name} (PascalCase)" + +java: + - pattern: "^azure-resourcemanager-[a-z0-9]+(?:-[a-z0-9]+)?$" + description: "Maven artifact: azure-resourcemanager-{name} (lowercase)" + - pattern: '^com\.azure\.resourcemanager\.[a-z0-9]+(?:\.[a-z0-9]+)?$' + description: "Java package: com.azure.resourcemanager.{name} (lowercase)" + +go: + pattern: "^sdk/resourcemanager/(?:[a-z0-9]+)/(?:arm[a-z0-9]+)$" + description: "sdk/resourcemanager/{name}/arm{name} (lowercase)" + +typescript: + pattern: "^@azure/arm-[a-z0-9]+$" + description: "@azure/arm-{name} (lowercase)" + +python: + pattern: "^azure-mgmt-[a-z0-9]+$" + description: "azure-mgmt-{name} (lowercase)" diff --git a/.github/workflows/src/namespace-approval/detect-namespaces.js b/.github/workflows/src/namespace-approval/detect-namespaces.js index 10c31c6d0eed..4d0d5a3a1ddc 100644 --- a/.github/workflows/src/namespace-approval/detect-namespaces.js +++ b/.github/workflows/src/namespace-approval/detect-namespaces.js @@ -2,6 +2,7 @@ import { execSync } from "child_process"; import { existsSync, readFileSync, rmSync } from "fs"; import yaml from "js-yaml"; import { dirname, join } from "path"; +import { loadFormatRules, validateNamespaceFormat } from "./validate-format.js"; /** * @typedef {Object} ApproversConfig @@ -284,13 +285,35 @@ export default async function detectNamespaces({ github, context, core }) { return approversConfig["data-plane"]?.[lang] ?? ["TBD"]; }; + // Validate namespace format against rules + const formatRules = loadFormatRules(core); + /** @type {Record} */ + const formatResults = {}; + if (formatRules) { + for (const [lang, ns] of Object.entries(namespacesFound)) { + formatResults[lang] = validateNamespaceFormat(lang, ns, formatRules); + } + } + const planeType = isMgmt ? "Management Plane" : "Data Plane"; const baseRef = payload.pull_request.base.ref; let body = `## Namespace Review Required\n\n**Plane:** ${planeType}\n\n`; - body += `| Language | Proposed Namespace | Status | Approvers |\n`; - body += `|----------|-------------------|--------|----------|\n`; + body += `| Language | Proposed Namespace | Format | Status | Approvers |\n`; + body += `|----------|-------------------|--------|--------|----------|\n`; for (const [lang, ns] of Object.entries(namespacesFound)) { - body += `| ${lang} | \`${ns}\` | ⏳ Pending | ${getApprovers(lang).join(", ")} |\n`; + const fmt = formatResults[lang]; + const formatStatus = !fmt ? "—" : fmt.valid ? "✅" : `⚠️ Invalid`; + body += `| ${lang} | \`${ns}\` | ${formatStatus} | ⏳ Pending | ${getApprovers(lang).join(", ")} |\n`; + } + + // Add format warnings if any + const formatErrors = Object.values(formatResults).filter((r) => !r.valid); + if (formatErrors.length > 0) { + body += `\n> **⚠️ Format issues detected:**\n`; + for (const err of formatErrors) { + body += `> - **${err.language}:** ${err.error}\n`; + } + body += `>\n> _Format validation does not block approval but should be reviewed._\n`; } body += `\n**How to approve:**\n`; body += `- Per language: apply \`-namespace-approved\` label\n`; diff --git a/.github/workflows/src/namespace-approval/validate-approval.js b/.github/workflows/src/namespace-approval/validate-approval.js index c977525af9eb..fd0ede0b9f81 100644 --- a/.github/workflows/src/namespace-approval/validate-approval.js +++ b/.github/workflows/src/namespace-approval/validate-approval.js @@ -294,9 +294,10 @@ export default async function validateApproval({ github, context, core }) { return; } - const payload = /** @type {import("@octokit/webhooks-types").PullRequestLabeledEvent} */ ( - context.payload - ); + const payload = + /** @type {import("@octokit/webhooks-types").PullRequestLabeledEvent | import("@octokit/webhooks-types").PullRequestUnlabeledEvent} */ ( + context.payload + ); const prNumber = payload.pull_request.number; /** @type {string[]} */ diff --git a/.github/workflows/src/namespace-approval/validate-format.js b/.github/workflows/src/namespace-approval/validate-format.js new file mode 100644 index 000000000000..0d9e199a0454 --- /dev/null +++ b/.github/workflows/src/namespace-approval/validate-format.js @@ -0,0 +1,98 @@ +import { readFileSync } from "fs"; +import yaml from "js-yaml"; + +/** + * @typedef {Object} FormatRule + * @property {string} pattern - Regex pattern string + * @property {string} [description] - Human-readable description + */ + +/** + * @typedef {Record} FormatRulesConfig + */ + +const DEFAULT_RULES_PATH = ".github/namespace-format-rules.yml"; + +/** + * Load format rules from YAML config. + * + * @param {import("@actions/core")} core + * @param {string} [rulesPath] + * @returns {FormatRulesConfig | null} + */ +export function loadFormatRules(core, rulesPath = DEFAULT_RULES_PATH) { + try { + const content = readFileSync(rulesPath, "utf8"); + return /** @type {FormatRulesConfig} */ (yaml.load(content)); + } catch (e) { + core.warning( + `Format rules not found at ${rulesPath}, skipping format validation: ${String(e)}`, + ); + return null; + } +} + +/** + * @typedef {Object} FormatValidationResult + * @property {boolean} valid + * @property {string} namespace + * @property {string} language + * @property {string} [matchedRule] - Description of the rule that matched + * @property {string} [error] - Error message if invalid + */ + +/** + * Validate a namespace string against the format rules for a language. + * + * @param {string} language - Language key (e.g., "dotnet", "java", "go") + * @param {string} namespace - The namespace string to validate + * @param {FormatRulesConfig} rules - The loaded format rules + * @returns {FormatValidationResult} + */ +export function validateNamespaceFormat(language, namespace, rules) { + const langRules = rules[language]; + + if (!langRules) { + // No rules defined for this language — pass by default + return { valid: true, namespace, language, matchedRule: "no rules defined" }; + } + + const ruleList = Array.isArray(langRules) ? langRules : [langRules]; + + for (const rule of ruleList) { + const regex = new RegExp(rule.pattern); + if (regex.test(namespace)) { + return { + valid: true, + namespace, + language, + matchedRule: rule.description ?? rule.pattern, + }; + } + } + + // None of the patterns matched + const expectedPatterns = ruleList.map((r) => r.description ?? r.pattern).join(" OR "); + return { + valid: false, + namespace, + language, + error: `"${namespace}" does not match expected format: ${expectedPatterns}`, + }; +} + +/** + * Validate all detected namespaces against format rules. + * + * @param {Record} namespacesFound - Map of language to namespace + * @param {FormatRulesConfig} rules - The loaded format rules + * @returns {FormatValidationResult[]} + */ +export function validateAllNamespaces(namespacesFound, rules) { + /** @type {FormatValidationResult[]} */ + const results = []; + for (const [lang, ns] of Object.entries(namespacesFound)) { + results.push(validateNamespaceFormat(lang, ns, rules)); + } + return results; +} diff --git a/.github/workflows/test/namespace-approval/status-check.test.js b/.github/workflows/test/namespace-approval/status-check.test.js new file mode 100644 index 000000000000..b28a4bcaccfd --- /dev/null +++ b/.github/workflows/test/namespace-approval/status-check.test.js @@ -0,0 +1,75 @@ +import { beforeEach, describe, expect, it, vi } from "vitest"; +import statusCheck from "../../src/namespace-approval/status-check.js"; +import { createMockContext, createMockCore, createMockGithub } from "../mocks.js"; + +describe("status-check", () => { + /** @type {ReturnType} */ + let github; + /** @type {ReturnType} */ + let context; + /** @type {ReturnType} */ + let core; + + /** @returns {import("@actions/github-script").AsyncFunctionArguments} */ + function args() { + return /** @type {import("@actions/github-script").AsyncFunctionArguments} */ ( + /** @type {unknown} */ ({ github, context, core }) + ); + } + + beforeEach(() => { + vi.clearAllMocks(); + github = createMockGithub(); + context = createMockContext(); + core = createMockCore(); + }); + + it("should pass when no namespace-review-required label", async () => { + context.payload = { pull_request: { number: 42 } }; + github.rest.pulls.get.mockResolvedValue({ + data: { labels: [{ name: "other-label" }] }, + }); + + await statusCheck(args()); + + expect(core.info).toHaveBeenCalledWith("No namespace review required, passing"); + expect(core.setFailed).not.toHaveBeenCalled(); + }); + + it("should fail when pending labels remain", async () => { + context.payload = { pull_request: { number: 42 } }; + github.rest.pulls.get.mockResolvedValue({ + data: { + labels: [ + { name: "namespace-review-required" }, + { name: "java-namespace-pending" }, + { name: "dotnet-namespace-pending" }, + ], + }, + }); + + await statusCheck(args()); + + const failMsg = String(core.setFailed.mock.calls[0][0]); + expect(failMsg).toContain("java"); + expect(failMsg).toContain("dotnet"); + }); + + it("should pass when all namespaces approved", async () => { + context.payload = { pull_request: { number: 42 } }; + github.rest.pulls.get.mockResolvedValue({ + data: { + labels: [ + { name: "namespace-review-required" }, + { name: "namespace-approved" }, + { name: "java-namespace-approved" }, + ], + }, + }); + + await statusCheck(args()); + + expect(core.info).toHaveBeenCalledWith("All namespaces approved - merge allowed"); + expect(core.setFailed).not.toHaveBeenCalled(); + }); +}); diff --git a/.github/workflows/test/namespace-approval/validate-approval.test.js b/.github/workflows/test/namespace-approval/validate-approval.test.js new file mode 100644 index 000000000000..77992c067b4b --- /dev/null +++ b/.github/workflows/test/namespace-approval/validate-approval.test.js @@ -0,0 +1,316 @@ +import { beforeEach, describe, expect, it, vi } from "vitest"; +import { createMockContext, createMockCore, createMockGithub } from "../mocks.js"; + +// cspell:words jsquire xirzec + +// Mock fs and js-yaml so validate-approval.js doesn't hit disk +vi.mock("fs", () => ({ + readFileSync: vi.fn(), +})); +vi.mock("js-yaml", () => ({ + default: { load: vi.fn() }, +})); + +import { readFileSync } from "fs"; +import yaml from "js-yaml"; +import validateApproval from "../../src/namespace-approval/validate-approval.js"; + +/** @type {import("../../src/namespace-approval/validate-approval.js").ApproversConfig} */ +const approversConfig = { + "data-plane": { + dotnet: ["jsquire", "m-nash"], + java: ["JonathanGiles"], + python: ["xirzec"], + }, + "management-plane": { + all: ["ArthurMa1978", "m-nash"], + }, +}; + +function setupMocks() { + /** @type {ReturnType} */ (readFileSync).mockReturnValue("yaml-content"); + /** @type {ReturnType} */ (yaml.load).mockReturnValue(approversConfig); +} + +/** + * @param {Object} opts + * @param {string} opts.action + * @param {string} opts.labelName + * @param {string} opts.actor + * @param {string[]} [opts.labels] + * @param {boolean} [opts.isMgmt] + */ +function createPRLabeledPayload({ action, labelName, actor, labels, isMgmt = false }) { + /** @type {string[]} */ + const labelNames = [...(labels ?? []), ...(isMgmt ? ["Mgmt"] : [])]; + const allLabels = labelNames.map((name) => ({ name })); + return { + action, + label: { name: labelName }, + sender: { login: actor, type: "User" }, + pull_request: { + number: 42, + labels: allLabels, + }, + }; +} + +describe("validate-approval", () => { + /** @type {ReturnType} */ + let github; + /** @type {ReturnType} */ + let context; + /** @type {ReturnType} */ + let core; + + /** @returns {import("@actions/github-script").AsyncFunctionArguments} */ + function args() { + return /** @type {import("@actions/github-script").AsyncFunctionArguments} */ ( + /** @type {unknown} */ ({ github, context, core }) + ); + } + + beforeEach(() => { + vi.clearAllMocks(); + setupMocks(); + github = createMockGithub(); + context = createMockContext(); + core = createMockCore(); + + // Extend mock with methods used by validate-approval + /** @type {any} */ (github.rest.issues).createComment = vi.fn(); + /** @type {any} */ (github.rest.issues).getLabel = vi.fn(); + /** @type {any} */ (github.rest.issues).createLabel = vi.fn(); + /** @type {any} */ (github.rest.issues).listComments = vi.fn().mockResolvedValue({ data: [] }); + }); + + describe("labeled - per-language approval", () => { + it("should skip when namespace-review-required label is absent", async () => { + context.payload = createPRLabeledPayload({ + action: "labeled", + labelName: "java-namespace-approved", + actor: "JonathanGiles", + labels: ["java-namespace-pending"], + }); + + await validateApproval(args()); + + expect(core.info).toHaveBeenCalledWith("No namespace review, skipping"); + expect(github.rest.issues.removeLabel).not.toHaveBeenCalled(); + }); + + it("should skip non-namespace labels", async () => { + context.payload = createPRLabeledPayload({ + action: "labeled", + labelName: "some-other-label", + actor: "anyone", + labels: ["namespace-review-required"], + }); + + await validateApproval(args()); + + expect(core.info).toHaveBeenCalledWith( + expect.stringContaining("is not a namespace approval label"), + ); + }); + + it("should allow authorized approver to approve their language", async () => { + context.payload = createPRLabeledPayload({ + action: "labeled", + labelName: "java-namespace-approved", + actor: "JonathanGiles", + labels: ["namespace-review-required", "java-namespace-pending"], + }); + + github.rest.pulls.get.mockResolvedValue({ + data: { labels: [{ name: "namespace-review-required" }] }, + }); + /** @type {any} */ (github.rest.issues).listComments = vi + .fn() + .mockResolvedValue({ data: [] }); + + await validateApproval(args()); + + expect(github.rest.issues.removeLabel).toHaveBeenCalledWith( + expect.objectContaining({ name: "java-namespace-pending" }), + ); + }); + + it("should reject unauthorized approver and remove label", async () => { + context.payload = createPRLabeledPayload({ + action: "labeled", + labelName: "java-namespace-approved", + actor: "random-user", + labels: ["namespace-review-required", "java-namespace-pending"], + }); + + await validateApproval(args()); + + expect(github.rest.issues.removeLabel).toHaveBeenCalledWith( + expect.objectContaining({ name: "java-namespace-approved" }), + ); + expect(github.rest.issues.createComment).toHaveBeenCalled(); + const calls = vi.mocked(github.rest.issues.createComment).mock.calls; + const commentBody = String(/** @type {Record} */ (calls[0][0]).body); + expect(commentBody).toContain("not authorized"); + }); + + it("should allow mgmt-plane approver for mgmt PR", async () => { + context.payload = createPRLabeledPayload({ + action: "labeled", + labelName: "dotnet-namespace-approved", + actor: "ArthurMa1978", + labels: ["namespace-review-required", "dotnet-namespace-pending"], + isMgmt: true, + }); + + github.rest.pulls.get.mockResolvedValue({ + data: { labels: [{ name: "namespace-review-required" }] }, + }); + /** @type {any} */ (github.rest.issues).listComments = vi + .fn() + .mockResolvedValue({ data: [] }); + + await validateApproval(args()); + + expect(github.rest.issues.removeLabel).toHaveBeenCalledWith( + expect.objectContaining({ name: "dotnet-namespace-pending" }), + ); + }); + }); + + describe("labeled - namespace-approved-all shortcut", () => { + it("should approve all pending languages for authorized user", async () => { + context.payload = createPRLabeledPayload({ + action: "labeled", + labelName: "namespace-approved-all", + actor: "ArthurMa1978", + labels: ["namespace-review-required", "dotnet-namespace-pending", "java-namespace-pending"], + }); + + github.rest.pulls.get.mockResolvedValue({ + data: { labels: [] }, + }); + /** @type {any} */ (github.rest.issues).listComments = vi + .fn() + .mockResolvedValue({ data: [] }); + + await validateApproval(args()); + + expect(github.rest.issues.removeLabel).toHaveBeenCalledWith( + expect.objectContaining({ name: "dotnet-namespace-pending" }), + ); + expect(github.rest.issues.removeLabel).toHaveBeenCalledWith( + expect.objectContaining({ name: "java-namespace-pending" }), + ); + }); + + it("should reject namespace-approved-all from unauthorized user", async () => { + context.payload = createPRLabeledPayload({ + action: "labeled", + labelName: "namespace-approved-all", + actor: "random-user", + labels: ["namespace-review-required", "dotnet-namespace-pending"], + }); + + await validateApproval(args()); + + expect(github.rest.issues.removeLabel).toHaveBeenCalledWith( + expect.objectContaining({ name: "namespace-approved-all" }), + ); + expect(github.rest.issues.createComment).toHaveBeenCalled(); + const calls = vi.mocked(github.rest.issues.createComment).mock.calls; + const commentBody = String(/** @type {Record} */ (calls[0][0]).body); + expect(commentBody).toContain("not authorized"); + }); + }); + + describe("unlabeled - guard against unauthorized removal", () => { + it("should re-apply pending label when removed by unauthorized user", async () => { + context.payload = createPRLabeledPayload({ + action: "unlabeled", + labelName: "java-namespace-pending", + actor: "random-user", + labels: ["namespace-review-required"], + }); + + await validateApproval(args()); + + expect(github.rest.issues.addLabels).toHaveBeenCalledWith( + expect.objectContaining({ + labels: ["java-namespace-pending"], + }), + ); + expect(github.rest.issues.createComment).toHaveBeenCalled(); + const calls = vi.mocked(github.rest.issues.createComment).mock.calls; + const commentBody = String(/** @type {Record} */ (calls[0][0]).body); + expect(commentBody).toContain("not authorized to remove"); + }); + + it("should allow trusted bot to remove pending label", async () => { + context.payload = createPRLabeledPayload({ + action: "unlabeled", + labelName: "java-namespace-pending", + actor: "github-actions[bot]", + labels: ["namespace-review-required"], + }); + + await validateApproval(args()); + + expect(github.rest.issues.addLabels).not.toHaveBeenCalled(); + const infoArgs = core.info.mock.calls.map((/** @type {unknown[]} */ c) => String(c[0])); + expect(infoArgs.some((/** @type {string} */ m) => m.includes("trusted bot"))).toBe(true); + }); + + it("should allow authorized approver to remove pending label", async () => { + context.payload = createPRLabeledPayload({ + action: "unlabeled", + labelName: "dotnet-namespace-pending", + actor: "jsquire", + labels: ["namespace-review-required"], + }); + + await validateApproval(args()); + + expect(github.rest.issues.addLabels).not.toHaveBeenCalled(); + const infoArgs = core.info.mock.calls.map((/** @type {unknown[]} */ c) => String(c[0])); + expect(infoArgs.some((/** @type {string} */ m) => m.includes("authorized approver"))).toBe( + true, + ); + }); + + it("should ignore non-namespace labels on unlabeled", async () => { + context.payload = createPRLabeledPayload({ + action: "unlabeled", + labelName: "some-other-label", + actor: "random-user", + labels: ["namespace-review-required"], + }); + + await validateApproval(args()); + + expect(github.rest.issues.addLabels).not.toHaveBeenCalled(); + const infoArgs = core.info.mock.calls.map((/** @type {unknown[]} */ c) => String(c[0])); + expect(infoArgs.some((/** @type {string} */ m) => m.includes("not a namespace label"))).toBe( + true, + ); + }); + + it("should guard namespace-review-required label too", async () => { + context.payload = createPRLabeledPayload({ + action: "unlabeled", + labelName: "namespace-review-required", + actor: "random-user", + labels: [], + }); + + await validateApproval(args()); + + expect(github.rest.issues.addLabels).toHaveBeenCalledWith( + expect.objectContaining({ + labels: ["namespace-review-required"], + }), + ); + }); + }); +}); diff --git a/.github/workflows/test/namespace-approval/validate-format.test.js b/.github/workflows/test/namespace-approval/validate-format.test.js new file mode 100644 index 000000000000..33c3ba6d97d0 --- /dev/null +++ b/.github/workflows/test/namespace-approval/validate-format.test.js @@ -0,0 +1,161 @@ +import { describe, expect, it } from "vitest"; +import { + validateAllNamespaces, + validateNamespaceFormat, +} from "../../src/namespace-approval/validate-format.js"; + +/** @type {import("../../src/namespace-approval/validate-format.js").FormatRulesConfig} */ +const rules = { + dotnet: { + pattern: "^Azure\\.ResourceManager\\.(?:[A-Z][a-zA-Z0-9]*)(?:\\.[A-Z][a-zA-Z0-9]*)?$", + description: "Azure.ResourceManager.{Name} (PascalCase)", + }, + java: [ + { + pattern: "^azure-resourcemanager-[a-z0-9]+(?:-[a-z0-9]+)?$", + description: "Maven artifact (lowercase)", + }, + { + pattern: "^com\\.azure\\.resourcemanager\\.[a-z0-9]+(?:\\.[a-z0-9]+)?$", + description: "Java package (lowercase)", + }, + ], + go: { + pattern: "^sdk/resourcemanager/(?:[a-z0-9]+)/(?:arm[a-z0-9]+)$", + description: "sdk/resourcemanager/{name}/arm{name}", + }, + typescript: { + pattern: "^@azure/arm-[a-z0-9]+$", + description: "@azure/arm-{name}", + }, + python: { + pattern: "^azure-mgmt-[a-z0-9]+$", + description: "azure-mgmt-{name}", + }, +}; + +describe("validate-format", () => { + describe("dotnet", () => { + it("should accept valid 3-level PascalCase namespace", () => { + const result = validateNamespaceFormat("dotnet", "Azure.ResourceManager.Compute", rules); + expect(result.valid).toBe(true); + }); + + it("should accept valid 4-level PascalCase namespace", () => { + const result = validateNamespaceFormat( + "dotnet", + "Azure.ResourceManager.Monitor.Workspaces", + rules, + ); + expect(result.valid).toBe(true); + }); + + it("should reject lowercase namespace", () => { + const result = validateNamespaceFormat("dotnet", "Azure.ResourceManager.compute", rules); + expect(result.valid).toBe(false); + expect(result.error).toContain("does not match"); + }); + + it("should reject missing ResourceManager prefix", () => { + const result = validateNamespaceFormat("dotnet", "Azure.Compute", rules); + expect(result.valid).toBe(false); + }); + }); + + describe("java", () => { + it("should accept valid Maven artifact name", () => { + const result = validateNamespaceFormat("java", "azure-resourcemanager-compute", rules); + expect(result.valid).toBe(true); + }); + + it("should accept valid Java package name", () => { + const result = validateNamespaceFormat("java", "com.azure.resourcemanager.compute", rules); + expect(result.valid).toBe(true); + }); + + it("should accept 4-level Maven artifact", () => { + const result = validateNamespaceFormat( + "java", + "azure-resourcemanager-monitor-workspaces", + rules, + ); + expect(result.valid).toBe(true); + }); + + it("should reject PascalCase Java name", () => { + const result = validateNamespaceFormat("java", "azure-resourcemanager-Compute", rules); + expect(result.valid).toBe(false); + }); + }); + + describe("go", () => { + it("should accept valid 3-level Go module path", () => { + const result = validateNamespaceFormat("go", "sdk/resourcemanager/compute/armcompute", rules); + expect(result.valid).toBe(true); + }); + + it("should reject missing arm prefix", () => { + const result = validateNamespaceFormat("go", "sdk/resourcemanager/compute/compute", rules); + expect(result.valid).toBe(false); + }); + + it("should reject PascalCase", () => { + const result = validateNamespaceFormat("go", "sdk/resourcemanager/Compute/armCompute", rules); + expect(result.valid).toBe(false); + }); + }); + + describe("typescript", () => { + it("should accept valid JS/TS package name", () => { + const result = validateNamespaceFormat("typescript", "@azure/arm-compute", rules); + expect(result.valid).toBe(true); + }); + + it("should reject non-arm scoped package", () => { + const result = validateNamespaceFormat("typescript", "@azure/compute", rules); + expect(result.valid).toBe(false); + }); + + it("should reject PascalCase", () => { + const result = validateNamespaceFormat("typescript", "@azure/arm-Compute", rules); + expect(result.valid).toBe(false); + }); + }); + + describe("python", () => { + it("should accept valid Python package name", () => { + const result = validateNamespaceFormat("python", "azure-mgmt-compute", rules); + expect(result.valid).toBe(true); + }); + + it("should reject non-mgmt prefix", () => { + const result = validateNamespaceFormat("python", "azure-compute", rules); + expect(result.valid).toBe(false); + }); + }); + + describe("unknown language", () => { + it("should pass when no rules exist for language", () => { + const result = validateNamespaceFormat("swift", "SomeNamespace", rules); + expect(result.valid).toBe(true); + expect(result.matchedRule).toBe("no rules defined"); + }); + }); + + describe("validateAllNamespaces", () => { + it("should validate all namespaces and return mixed results", () => { + const namespaces = { + dotnet: "Azure.ResourceManager.Compute", + java: "INVALID-NAME", + python: "azure-mgmt-network", + }; + + const results = validateAllNamespaces(namespaces, rules); + + expect(results).toHaveLength(3); + expect(results.find((r) => r.language === "dotnet")?.valid).toBe(true); + expect(results.find((r) => r.language === "java")?.valid).toBe(false); + expect(results.find((r) => r.language === "python")?.valid).toBe(true); + }); + }); +}); From 67ed6c868af9207274a7dcc38b44ff63343a484d Mon Sep 17 00:00:00 2001 From: Sameeksha Vaity Date: Tue, 23 Jun 2026 11:37:34 -0700 Subject: [PATCH 04/23] Address review: split workflow, use repo helpers, add zod schema - Split namespace-approval.yml into code + status workflows (namespace-approval-code.yaml / namespace-approval-status.yaml) to support fork PRs (read-only vs write permissions) - Use repo helpers: getChangedFiles(), execNpmExec(), extractInputs(), commentOrUpdate(), async fs APIs - Add zod schema for typespec-metadata.json validation - Write emitter output to RUNNER_TEMP instead of clone dir - Drop YAML fallback codepath (emitter-only) - Move NAMESPACE-REVIEW-PROCESS.md into src/namespace-approval/ - Create post-results.js for status workflow (artifact-based) - Add reopened + edited trigger types, fetch-depth: 2 Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- .../workflows/namespace-approval-code.yaml | 44 ++ .../workflows/namespace-approval-status.yaml | 85 ++++ .github/workflows/namespace-approval.yml | 102 ----- .../NAMESPACE-REVIEW-PROCESS.md | 36 +- .../namespace-approval/detect-namespaces.js | 433 ++++++------------ .../src/namespace-approval/post-results.js | 292 ++++++++++++ .../src/namespace-approval/status-check.js | 16 +- .../namespace-approval/validate-approval.js | 28 +- .../namespace-approval/status-check.test.js | 18 +- .../validate-approval.test.js | 40 +- 10 files changed, 635 insertions(+), 459 deletions(-) create mode 100644 .github/workflows/namespace-approval-code.yaml create mode 100644 .github/workflows/namespace-approval-status.yaml delete mode 100644 .github/workflows/namespace-approval.yml rename .github/{ => workflows/src/namespace-approval}/NAMESPACE-REVIEW-PROCESS.md (73%) create mode 100644 .github/workflows/src/namespace-approval/post-results.js diff --git a/.github/workflows/namespace-approval-code.yaml b/.github/workflows/namespace-approval-code.yaml new file mode 100644 index 000000000000..2199d5378ea8 --- /dev/null +++ b/.github/workflows/namespace-approval-code.yaml @@ -0,0 +1,44 @@ +name: "Namespace Approval - Analyze Code" + +on: + pull_request: + types: + - opened + - synchronize + - reopened + - edited + +permissions: + contents: read + +jobs: + detect-namespaces: + name: "Namespace Approval - Analyze Code" + runs-on: ubuntu-24.04 + steps: + - uses: actions/checkout@v6 + with: + sparse-checkout: | + .github/ + fetch-depth: 2 + + - name: Setup Node and install dependencies + uses: ./.github/actions/setup-node-install-deps + + - name: Detect namespace changes + id: detect + uses: actions/github-script@v8 + with: + script: | + const { default: detectNamespaces } = + await import('${{ github.workspace }}/.github/workflows/src/namespace-approval/detect-namespaces.js'); + await detectNamespaces({ github, context, core }); + + - name: Upload namespace results + if: always() && steps.detect.outputs.results + uses: actions/upload-artifact@v7 + with: + name: namespace-results + path: ${{ steps.detect.outputs.results_path }} + if-no-files-found: ignore + diff --git a/.github/workflows/namespace-approval-status.yaml b/.github/workflows/namespace-approval-status.yaml new file mode 100644 index 000000000000..47c033fd107a --- /dev/null +++ b/.github/workflows/namespace-approval-status.yaml @@ -0,0 +1,85 @@ +name: "Namespace Approval - Set Status" + +on: + pull_request_target: + types: + - labeled + - unlabeled + workflow_run: + workflows: ["Namespace Approval - Analyze Code"] + types: [completed] + +permissions: + actions: read + contents: read + pull-requests: write + +jobs: + post-results: + if: github.event_name == 'workflow_run' + name: Post Namespace Results + runs-on: ubuntu-24.04 + steps: + - uses: actions/checkout@v6 + with: + sparse-checkout: | + .github/ + + - name: Setup Node and install dependencies + uses: ./.github/actions/setup-node-install-deps + + - name: Install dependencies for github-script actions + uses: ./.github/actions/install-deps-github-script + + - name: Post namespace review comment + uses: actions/github-script@v8 + with: + script: | + const { default: postResults } = + await import('${{ github.workspace }}/.github/workflows/src/namespace-approval/post-results.js'); + await postResults({ github, context, core }); + + validate-approval: + if: > + github.event_name == 'pull_request_target' && + (github.event.action == 'labeled' || github.event.action == 'unlabeled') + name: Validate Namespace Approval + runs-on: ubuntu-24.04 + steps: + - uses: actions/checkout@v6 + with: + sparse-checkout: | + .github/ + + - name: Install dependencies for github-script actions + uses: ./.github/actions/install-deps-github-script + + - name: Validate namespace approval label + uses: actions/github-script@v8 + with: + script: | + const { default: validateApproval } = + await import('${{ github.workspace }}/.github/workflows/src/namespace-approval/validate-approval.js'); + await validateApproval({ github, context, core }); + + status-check: + name: Namespace Approval Status + needs: [post-results, validate-approval] + if: always() + runs-on: ubuntu-24.04 + steps: + - uses: actions/checkout@v6 + with: + sparse-checkout: | + .github/ + + - name: Install dependencies for github-script actions + uses: ./.github/actions/install-deps-github-script + + - name: Verify namespace approval status + uses: actions/github-script@v8 + with: + script: | + const { default: statusCheck } = + await import('${{ github.workspace }}/.github/workflows/src/namespace-approval/status-check.js'); + await statusCheck({ github, context, core }); diff --git a/.github/workflows/namespace-approval.yml b/.github/workflows/namespace-approval.yml deleted file mode 100644 index bd377ee95ab3..000000000000 --- a/.github/workflows/namespace-approval.yml +++ /dev/null @@ -1,102 +0,0 @@ -name: Namespace Approval - -on: - pull_request: - types: - - opened - - synchronize - - labeled - - unlabeled - -permissions: - pull-requests: write - contents: read - -jobs: - detect-namespaces: - if: github.event.action == 'opened' || github.event.action == 'synchronize' - name: Detect Namespace Changes - - runs-on: ubuntu-24.04 - - outputs: - has_pending: ${{ steps.detect.outputs.has_pending }} - - steps: - - uses: actions/checkout@v6 - with: - fetch-depth: 0 - - - name: Setup Node and install dependencies - uses: ./.github/actions/setup-node-install-deps - - - name: Install dependencies for github-script actions - uses: ./.github/actions/install-deps-github-script - - - id: detect - name: Detect namespace changes - uses: actions/github-script@v8 - with: - script: | - const { default: detectNamespaces } = - await import('${{ github.workspace }}/.github/workflows/src/namespace-approval/detect-namespaces.js'); - await detectNamespaces({ github, context, core }); - - validate-approval: - if: github.event.action == 'labeled' || github.event.action == 'unlabeled' - name: Validate Namespace Approval - - runs-on: ubuntu-24.04 - - steps: - - uses: actions/checkout@v6 - with: - ref: ${{ github.event.pull_request.base.ref }} - sparse-checkout: | - .github - - - name: Install dependencies for github-script actions - uses: ./.github/actions/install-deps-github-script - - - name: Validate namespace approval label - uses: actions/github-script@v8 - with: - script: | - const { default: validateApproval } = - await import('${{ github.workspace }}/.github/workflows/src/namespace-approval/validate-approval.js'); - await validateApproval({ github, context, core }); - - status-check: - name: Namespace Approval Status - needs: [detect-namespaces, validate-approval] - if: always() - - runs-on: ubuntu-24.04 - - steps: - - name: Verify namespace approval status - uses: actions/github-script@v8 - with: - script: | - const { data: pr } = await github.rest.pulls.get({ - owner: context.repo.owner, - repo: context.repo.repo, - pull_number: context.payload.pull_request.number, - }); - const labels = pr.labels.map((l) => l.name); - - if (!labels.includes("namespace-review-required")) { - core.info("No namespace review required, passing"); - return; - } - - const pendingLabels = labels.filter((l) => l.endsWith("-namespace-pending")); - if (pendingLabels.length > 0) { - core.setFailed( - `Namespace approval pending for: ${pendingLabels - .map((l) => l.replace("-namespace-pending", "")) - .join(", ")}`, - ); - } else if (labels.includes("namespace-approved")) { - core.info("All namespaces approved - merge allowed"); - } diff --git a/.github/NAMESPACE-REVIEW-PROCESS.md b/.github/workflows/src/namespace-approval/NAMESPACE-REVIEW-PROCESS.md similarity index 73% rename from .github/NAMESPACE-REVIEW-PROCESS.md rename to .github/workflows/src/namespace-approval/NAMESPACE-REVIEW-PROCESS.md index fcd84adb3838..1fbfe35ac0b1 100644 --- a/.github/NAMESPACE-REVIEW-PROCESS.md +++ b/.github/workflows/src/namespace-approval/NAMESPACE-REVIEW-PROCESS.md @@ -10,14 +10,16 @@ This process is automated via GitHub Actions on this repository. ### Detection -When a PR modifies any `tspconfig.yaml` file, the workflow: +When a PR modifies any `tspconfig.yaml` file, the analyze-code workflow: 1. Identifies which languages have namespace/package-name configuration -2. Extracts namespaces using the `@azure-tools/typespec-metadata` emitter (primary) with YAML parsing fallback +2. Extracts namespaces using the `@azure-tools/typespec-metadata` emitter 3. Determines if the service is data-plane or management-plane -4. Applies `-namespace-pending` labels for each detected language +4. Uploads namespace results for the status workflow -### Approval +### Status + Approval + +The status workflow posts the namespace review comment, applies `-namespace-pending` labels, and validates architect approval labels. - **Data plane:** Each language architect approves their language by applying `-namespace-approved` - **Management plane:** ArthurMa1978 or m-nash can apply `namespace-approved-all` to approve all languages at once @@ -32,14 +34,12 @@ A required status check (`namespace-approval`) blocks merge until all pending na ### Reset on Change -If a service team pushes a commit that **changes** a namespace value (not just TypeSpec code), the approval for that language is reset: +If a service team pushes a commit that changes namespace configuration, the approval for that language is reset: - The `-namespace-approved` label is removed - The `-namespace-pending` label is re-applied - The status check fails again until re-approved -TypeSpec-only changes (no namespace modification) do **not** reset approval. - ## Labels | Label | Applied by | Meaning | @@ -53,18 +53,18 @@ Where `` is one of: `dotnet`, `java`, `python`, `typescript`, `go` ## Authorized Approvers -Defined in [`.github/namespace-approvers.yml`](../.github/namespace-approvers.yml): +Defined in [`.github/namespace-approvers.yml`](../../../namespace-approvers.yml): -| Scope | Approvers | -| -------------------- | ----------------------------- | -| .NET | @jsquire, @m-nash | +| Scope | Approvers | +| -------------------- | ------------------------------ | +| .NET | @jsquire, @m-nash | | Java | @JonathanGiles, @alzimmermsft | -| Python | @xirzec, @kashifkhan | -| TypeScript | @xirzec, @maorleger | -| Go | @jhendrixMSFT | -| Mgmt (all languages) | @ArthurMa1978, @m-nash | +| Python | @xirzec, @kashifkhan | +| TypeScript | @xirzec, @maorleger | +| Go | @jhendrixMSFT | +| Mgmt (all languages) | @ArthurMa1978, @m-nash | @@ -72,10 +72,10 @@ Defined in [`.github/namespace-approvers.yml`](../.github/namespace-approvers.ym No action needed beyond your normal workflow: -1. Add/modify namespace in your `tspconfig.yaml` +1. Add or modify namespace in your `tspconfig.yaml` 2. Open a PR as usual -3. The bot will apply pending labels and post a status table -4. Wait for architects to approve — they're notified via labels +3. The bot posts a status table and applies pending labels +4. Wait for architects to approve 5. Once approved, the status check passes and the PR can merge ## For Architects diff --git a/.github/workflows/src/namespace-approval/detect-namespaces.js b/.github/workflows/src/namespace-approval/detect-namespaces.js index 4d0d5a3a1ddc..215f27fa87f2 100644 --- a/.github/workflows/src/namespace-approval/detect-namespaces.js +++ b/.github/workflows/src/namespace-approval/detect-namespaces.js @@ -1,38 +1,32 @@ -import { execSync } from "child_process"; -import { existsSync, readFileSync, rmSync } from "fs"; -import yaml from "js-yaml"; -import { dirname, join } from "path"; +import { access, readFile, rm, writeFile } from "fs/promises"; +import { tmpdir } from "os"; +import { basename, dirname, join } from "path"; +import { z } from "zod"; +import { getChangedFiles } from "../../../shared/src/changed-files.js"; +import { execFile, execNpmExec } from "../../../shared/src/exec.js"; import { loadFormatRules, validateNamespaceFormat } from "./validate-format.js"; -/** - * @typedef {Object} ApproversConfig - * @property {Record} [data-plane] - * @property {{ all?: string[] }} [management-plane] - */ - -/** - * @typedef {Object} TypeSpecMetadataEntry - * @property {string} [namespace] - * @property {string} [packageName] - */ - -/** - * @typedef {Object} TypeSpecMetadata - * @property {{ type?: string }} [typespec] - * @property {Record} [languages] - */ - -/** - * @typedef {Object} TspConfigEmitterOptions - * @property {string} [namespace] - * @property {{ name?: string }} [package-details] - * @property {string} [module] - */ - -/** - * @typedef {Object} TspConfig - * @property {Record} [options] - */ +const TypeSpecMetadataSchema = z.object({ + typespec: z + .object({ + type: z.enum(["management", "data"]).optional(), + }) + .optional(), + languages: z + .record( + z.string(), + z.array( + z.object({ + namespace: z.string().optional(), + packageName: z.string().optional(), + }), + ), + ) + .optional(), +}); + +/** @typedef {{ namespace?: string, packageName?: string }} TypeSpecMetadataEntry */ +/** @typedef {Record} TypeSpecLanguages */ /** @type {Record} */ const EMITTER_TO_LANG = { @@ -45,46 +39,55 @@ const EMITTER_TO_LANG = { go: "go", }; -/** @type {Record} */ -const FALLBACK_EMITTER_MAP = { - "@azure-typespec/http-client-csharp": "dotnet", - "@azure-typespec/http-client-csharp-mgmt": "dotnet", - "@azure-tools/typespec-csharp": "dotnet", - "@azure-tools/typespec-java": "java", - "@azure-tools/typespec-python": "python", - "@azure-tools/typespec-ts": "typescript", - "@azure-tools/typespec-go": "go", -}; +/** + * @param {string} path + * @returns {Promise} + */ +async function exists(path) { + try { + await access(path); + return true; + } catch { + return false; + } +} /** - * Load approvers configuration from YAML file. + * Sparse checkout only materializes .github/, so hydrate changed TypeSpec directories on demand. * + * @param {string} file * @param {import("@actions/core")} core - * @returns {ApproversConfig | null} */ -function loadApproversConfig(core) { - try { - const content = readFileSync(".github/namespace-approvers.yml", "utf8"); - return /** @type {ApproversConfig} */ (yaml.load(content)); - } catch (e) { - core.setFailed("Failed to load .github/namespace-approvers.yml: " + String(e)); - return null; +async function ensureTypeSpecFilesAvailable(file, core) { + if (await exists(file)) { + return; } + + const tspDir = dirname(file); + const workspace = process.env.GITHUB_WORKSPACE ?? process.cwd(); + core.info(`Hydrating ${tspDir} from git for namespace detection`); + await execFile("git", ["checkout", "HEAD", "--", tspDir], { + cwd: workspace, + }); } /** - * Extract namespaces from tspconfig.yaml using typespec-metadata emitter with YAML fallback. + * Extract namespaces from tspconfig.yaml using typespec-metadata emitter. * * @param {string} file - Path to tspconfig.yaml * @param {Record} namespacesFound - Map of language to namespace (mutated) * @param {import("@actions/core")} core - * @returns {{ isMgmt: boolean, isDataPlane: boolean }} + * @returns {Promise<{ isMgmt: boolean, isDataPlane: boolean }>} */ -function extractNamespaces(file, namespacesFound, core) { +async function extractNamespaces(file, namespacesFound, core) { let isMgmt = false; let isDataPlane = false; - if (!existsSync(file)) return { isMgmt, isDataPlane }; + await ensureTypeSpecFilesAvailable(file, core); + if (!(await exists(file))) { + core.info(`Skipping missing file: ${file}`); + return { isMgmt, isDataPlane }; + } const tspDir = dirname(file); @@ -94,262 +97,118 @@ function extractNamespaces(file, namespacesFound, core) { isDataPlane = true; } - // Try typespec-metadata emitter first - try { - core.info(`Running typespec-metadata emitter on ${tspDir}`); - execSync( - `npx tsp compile "${tspDir}" --emit "@azure-tools/typespec-metadata" --output-dir "${tspDir}" --option "@azure-tools/typespec-metadata.format=json"`, - { stdio: "pipe", timeout: 120000 }, - ); + const outputDir = join(process.env.RUNNER_TEMP || tmpdir(), "typespec-metadata", basename(tspDir)); + await rm(outputDir, { recursive: true, force: true }); - const metadataPath = join( + core.info(`Running typespec-metadata emitter on ${tspDir}`); + await execNpmExec( + [ + "tsp", + "compile", tspDir, - "@azure-tools", - "typespec-metadata", - "typespec-metadata.json", - ); - if (!existsSync(metadataPath)) throw new Error("metadata file not found"); - - const metadataRaw = /** @type {unknown} */ (JSON.parse(readFileSync(metadataPath, "utf8"))); - const metadata = /** @type {TypeSpecMetadata} */ (metadataRaw); - - if (metadata.typespec?.type === "management") isMgmt = true; - if (metadata.typespec?.type === "data") isDataPlane = true; + "--emit", + "@azure-tools/typespec-metadata", + "--output-dir", + outputDir, + "--option", + "@azure-tools/typespec-metadata.format=json", + ], + { + cwd: process.env.GITHUB_WORKSPACE ?? process.cwd(), + }, + ); - const languages = metadata.languages ?? {}; - for (const [langKey, entries] of Object.entries(languages)) { - const lang = EMITTER_TO_LANG[langKey] ?? langKey; - if (entries.length > 0) { - const firstEntry = entries[0]; - const ns = firstEntry.namespace ?? firstEntry.packageName; - if (ns) namespacesFound[lang] = ns; - } - } + const metadataPath = join( + outputDir, + "@azure-tools", + "typespec-metadata", + "typespec-metadata.json", + ); + const metadata = TypeSpecMetadataSchema.parse(JSON.parse(await readFile(metadataPath, "utf8"))); - try { - rmSync(join(tspDir, "@azure-tools"), { recursive: true }); - } catch { - // ignore cleanup errors - } - } catch (e) { - core.warning(`typespec-metadata emitter failed: ${String(e)}. Falling back to YAML parse.`); - const content = readFileSync(file, "utf8"); - const parsed = /** @type {TspConfig} */ (yaml.load(content)); - if (!parsed?.options) return { isMgmt, isDataPlane }; + if (metadata.typespec?.type === "management") { + isMgmt = true; + } + if (metadata.typespec?.type === "data") { + isDataPlane = true; + } - for (const [emitter, lang] of Object.entries(FALLBACK_EMITTER_MAP)) { - const opts = parsed.options[emitter]; - if (!opts) continue; - if (opts.namespace) namespacesFound[lang] = opts.namespace; - else if (opts["package-details"]?.name) namespacesFound[lang] = opts["package-details"].name; - else if (opts.module) namespacesFound[lang] = opts.module; + const languages = /** @type {TypeSpecLanguages} */ (metadata.languages ?? {}); + for (const [langKey, entries] of Object.entries(languages)) { + const lang = EMITTER_TO_LANG[langKey] ?? langKey; + const firstEntry = entries[0]; + const namespace = firstEntry?.namespace ?? firstEntry?.packageName; + if (namespace) { + namespacesFound[lang] = namespace; } } + await rm(outputDir, { recursive: true, force: true }); return { isMgmt, isDataPlane }; } /** - * Detect namespace changes in a PR and apply pending labels. + * Detect namespace changes in a PR and write results to an artifact file. * * @param {import("@actions/github-script").AsyncFunctionArguments} args */ -export default async function detectNamespaces({ github, context, core }) { - const approversConfig = loadApproversConfig(core); - if (!approversConfig) return; - - const payload = /** @type {import("@octokit/webhooks-types").PullRequestEvent} */ ( - context.payload - ); - - const baseSha = payload.pull_request.base.sha; - const headSha = payload.pull_request.head.sha; - const diffOutput = execSync(`git diff --name-only ${baseSha}...${headSha}`).toString().trim(); - const changedFiles = diffOutput - .split("\n") - .filter((f) => /specification\/.*\/tspconfig\.yaml$/.test(f)); - - if (changedFiles.length === 0) { - core.info("No tspconfig.yaml changes detected, skipping"); - return; - } - - core.info(`Found tspconfig.yaml changes: ${changedFiles.join(", ")}`); +export default async function detectNamespaces({ context, core }) { + try { + const payload = /** @type {import("@octokit/webhooks-types").PullRequestEvent} */ ( + context.payload + ); - /** @type {Record} */ - const namespacesFound = {}; - let isMgmt = false; - let isDataPlane = false; + const changedFiles = ( + await getChangedFiles({ + cwd: process.env.GITHUB_WORKSPACE ?? process.cwd(), + paths: ["specification"], + }) + ).filter((file) => /specification\/.*\/tspconfig\.yaml$/.test(file)); - for (const file of changedFiles) { - const result = extractNamespaces(file, namespacesFound, core); - if (result.isMgmt) isMgmt = true; - if (result.isDataPlane) isDataPlane = true; - } + if (changedFiles.length === 0) { + core.info("No tspconfig.yaml changes detected, skipping"); + return; + } - const prNumber = payload.pull_request.number; + core.info(`Found tspconfig.yaml changes: ${changedFiles.join(", ")}`); - // On synchronize: reset approvals for changed namespaces - if (payload.action === "synchronize") { - const { data: currentPR } = await github.rest.pulls.get({ - owner: context.repo.owner, - repo: context.repo.repo, - pull_number: prNumber, - }); - /** @type {string[]} */ - const existingLabels = currentPR.labels.map((l) => l.name ?? ""); + /** @type {Record} */ + const namespacesFound = {}; + let isMgmt = false; + let isDataPlane = false; - for (const lang of Object.keys(namespacesFound)) { - const approvedLabel = `${lang}-namespace-approved`; - if (existingLabels.includes(approvedLabel)) { - core.info(`Namespace changed for ${lang}, resetting approval`); - try { - await github.rest.issues.removeLabel({ - owner: context.repo.owner, - repo: context.repo.repo, - issue_number: prNumber, - name: approvedLabel, - }); - } catch { - // label may not exist - } + for (const file of changedFiles) { + const result = await extractNamespaces(file, namespacesFound, core); + if (result.isMgmt) { + isMgmt = true; } - } - - for (const label of ["namespace-approved-all", "namespace-approved"]) { - if (existingLabels.includes(label)) { - try { - await github.rest.issues.removeLabel({ - owner: context.repo.owner, - repo: context.repo.repo, - issue_number: prNumber, - name: label, - }); - } catch { - // label may not exist - } + if (result.isDataPlane) { + isDataPlane = true; } } - } - - const languages = Object.keys(namespacesFound); - if (languages.length === 0) { - core.info("No namespace changes detected"); - return; - } - - // Apply labels - /** @type {string[]} */ - const labelsToAdd = ["namespace-review-required"]; - if (isMgmt) labelsToAdd.push("Mgmt"); - if (isDataPlane) labelsToAdd.push("data-plane"); - for (const lang of languages) labelsToAdd.push(`${lang}-namespace-pending`); - - // cspell:words fbbf - for (const label of labelsToAdd) { - try { - await github.rest.issues.getLabel({ - owner: context.repo.owner, - repo: context.repo.repo, - name: label, - }); - } catch { - /** @type {Record} */ - const colors = { "namespace-review-required": "e11d48" }; - await github.rest.issues.createLabel({ - owner: context.repo.owner, - repo: context.repo.repo, - name: label, - color: colors[label] ?? (label.includes("pending") ? "fbbf24" : "94a3b8"), - }); - } - } - - await github.rest.issues.addLabels({ - owner: context.repo.owner, - repo: context.repo.repo, - issue_number: prNumber, - labels: labelsToAdd, - }); - - // Build bot comment - /** - * @param {string} lang - * @returns {string[]} - */ - const getApprovers = (lang) => { - if (isMgmt) { - const mgmtApprovers = approversConfig["management-plane"]?.all; - if (mgmtApprovers) return mgmtApprovers; - } - return approversConfig["data-plane"]?.[lang] ?? ["TBD"]; - }; - - // Validate namespace format against rules - const formatRules = loadFormatRules(core); - /** @type {Record} */ - const formatResults = {}; - if (formatRules) { - for (const [lang, ns] of Object.entries(namespacesFound)) { - formatResults[lang] = validateNamespaceFormat(lang, ns, formatRules); - } - } - const planeType = isMgmt ? "Management Plane" : "Data Plane"; - const baseRef = payload.pull_request.base.ref; - let body = `## Namespace Review Required\n\n**Plane:** ${planeType}\n\n`; - body += `| Language | Proposed Namespace | Format | Status | Approvers |\n`; - body += `|----------|-------------------|--------|--------|----------|\n`; - for (const [lang, ns] of Object.entries(namespacesFound)) { - const fmt = formatResults[lang]; - const formatStatus = !fmt ? "—" : fmt.valid ? "✅" : `⚠️ Invalid`; - body += `| ${lang} | \`${ns}\` | ${formatStatus} | ⏳ Pending | ${getApprovers(lang).join(", ")} |\n`; - } - - // Add format warnings if any - const formatErrors = Object.values(formatResults).filter((r) => !r.valid); - if (formatErrors.length > 0) { - body += `\n> **⚠️ Format issues detected:**\n`; - for (const err of formatErrors) { - body += `> - **${err.language}:** ${err.error}\n`; + const formatRules = loadFormatRules(core); + /** @type {Record} */ + const formatResults = {}; + if (formatRules) { + for (const [language, namespace] of Object.entries(namespacesFound)) { + formatResults[language] = validateNamespaceFormat(language, namespace, formatRules); + } } - body += `>\n> _Format validation does not block approval but should be reviewed._\n`; - } - body += `\n**How to approve:**\n`; - body += `- Per language: apply \`-namespace-approved\` label\n`; - body += `- All at once: apply \`namespace-approved-all\` label (shortcut for mgmt plane)\n\n`; - body += `Merge is blocked until all languages are approved.\n`; - if (payload.action === "synchronize") { - body += `\n> ⚠️ **Namespace changed** — approvals for affected languages have been reset.\n`; - } - body += `\n_Approver list: [.github/namespace-approvers.yml](../blob/${baseRef}/.github/namespace-approvers.yml)_\n`; - body += `_Namespaces extracted via [@azure-tools/typespec-metadata](https://www.npmjs.com/package/@azure-tools/typespec-metadata) emitter_\n`; - body += ``; - const comments = await github.rest.issues.listComments({ - owner: context.repo.owner, - repo: context.repo.repo, - issue_number: prNumber, - }); - const botComment = comments.data.find( - (c) => c.user?.type === "Bot" && c.body?.includes(""), - ); - - if (botComment) { - await github.rest.issues.updateComment({ - owner: context.repo.owner, - repo: context.repo.repo, - comment_id: botComment.id, - body, - }); - } else { - await github.rest.issues.createComment({ - owner: context.repo.owner, - repo: context.repo.repo, - issue_number: prNumber, - body, - }); + const results = { + namespacesFound, + isMgmt, + isDataPlane, + formatResults, + prNumber: payload.pull_request.number, + action: payload.action, + }; + const resultsPath = join(process.env.RUNNER_TEMP || ".", "namespace-results.json"); + await writeFile(resultsPath, JSON.stringify(results, null, 2)); + core.setOutput("results", "true"); + core.setOutput("results_path", resultsPath); + } catch (error) { + core.setFailed(`Namespace detection failed: ${String(error)}`); } - - core.setOutput("has_pending", "true"); } diff --git a/.github/workflows/src/namespace-approval/post-results.js b/.github/workflows/src/namespace-approval/post-results.js new file mode 100644 index 000000000000..73ada11dcf95 --- /dev/null +++ b/.github/workflows/src/namespace-approval/post-results.js @@ -0,0 +1,292 @@ +import { readFile, unlink, writeFile } from "fs/promises"; +import { tmpdir } from "os"; +import { join } from "path"; +import yaml from "js-yaml"; +import { z } from "zod"; +import { execFile } from "../../../shared/src/exec.js"; +import { PER_PAGE_MAX } from "../../../shared/src/github.js"; +import { commentOrUpdate } from "../comment.js"; +import { extractInputs } from "../context.js"; + +const FormatValidationResultSchema = z.object({ + valid: z.boolean(), + namespace: z.string(), + language: z.string(), + matchedRule: z.string().optional(), + error: z.string().optional(), +}); + +const NamespaceResultsSchema = z.object({ + namespacesFound: z.record(z.string(), z.string()), + isMgmt: z.boolean(), + isDataPlane: z.boolean(), + formatResults: z.record(z.string(), FormatValidationResultSchema), + prNumber: z.number().int().positive(), + action: z.string().optional(), +}); + +/** + * @typedef {Object} ApproversConfig + * @property {Record} [data-plane] + * @property {{ all?: string[] }} [management-plane] + */ + +/** @type {Record} */ +const LABEL_COLORS = { + "namespace-review-required": "e11d48", +}; + +/** + * @param {import("@actions/core")} core + * @returns {Promise} + */ +async function loadApproversConfig(core) { + try { + const content = await readFile(".github/namespace-approvers.yml", "utf8"); + return /** @type {ApproversConfig} */ (yaml.load(content)); + } catch (error) { + core.setFailed("Failed to load .github/namespace-approvers.yml: " + String(error)); + return null; + } +} + +/** + * @param {import("@actions/github-script").AsyncFunctionArguments["github"]} github + * @param {import("@actions/core")} core + * @param {string} owner + * @param {string} repo + * @param {number} runId + * @returns {Promise | null>} + */ +async function downloadNamespaceResults(github, core, owner, repo, runId) { + const artifacts = await github.paginate(github.rest.actions.listWorkflowRunArtifacts, { + owner, + repo, + run_id: runId, + name: "namespace-results", + per_page: PER_PAGE_MAX, + }); + + const artifact = artifacts.sort((left, right) => { + const leftTime = new Date(left.updated_at || 0).getTime(); + const rightTime = new Date(right.updated_at || 0).getTime(); + return rightTime - leftTime; + })[0]; + + if (!artifact) { + core.info(`No namespace-results artifact found for run ${runId}`); + return null; + } + + const download = await github.rest.actions.downloadArtifact({ + owner, + repo, + artifact_id: artifact.id, + archive_format: "zip", + }); + + const zipPath = join(process.env.RUNNER_TEMP || tmpdir(), `namespace-results-${runId}.zip`); + const zipBuffer = Buffer.from(new Uint8Array(/** @type {ArrayBuffer} */ (download.data))); + await writeFile(zipPath, zipBuffer); + + try { + const { stdout } = await execFile("unzip", ["-p", zipPath, "namespace-results.json"]); + return /** @type {z.infer} */ ( + NamespaceResultsSchema.parse(JSON.parse(stdout)) + ); + } finally { + await unlink(zipPath).catch(() => undefined); + } +} + +/** + * @param {import("@actions/github-script").AsyncFunctionArguments["github"]} github + * @param {string} owner + * @param {string} repo + * @param {string} label + */ +async function ensureLabel(github, owner, repo, label) { + try { + await github.rest.issues.getLabel({ + owner, + repo, + name: label, + }); + } catch { + await github.rest.issues.createLabel({ + owner, + repo, + name: label, + color: LABEL_COLORS[label] ?? (label.includes("pending") ? "fbbf24" : "94a3b8"), + }); + } +} + +/** + * @param {import("@actions/github-script").AsyncFunctionArguments["github"]} github + * @param {string} owner + * @param {string} repo + * @param {number} issueNumber + * @param {string} label + */ +async function removeLabelIfPresent(github, owner, repo, issueNumber, label) { + try { + await github.rest.issues.removeLabel({ + owner, + repo, + issue_number: issueNumber, + name: label, + }); + } catch (error) { + if (error instanceof Error && "status" in error && error.status === 404) { + return; + } + throw error; + } +} + +/** + * @param {ApproversConfig} approversConfig + * @param {boolean} isMgmt + * @param {string} language + * @returns {string[]} + */ +function getApprovers(approversConfig, isMgmt, language) { + if (isMgmt) { + const mgmtApprovers = approversConfig["management-plane"]?.all; + if (mgmtApprovers) { + return mgmtApprovers; + } + } + + return approversConfig["data-plane"]?.[language] ?? ["TBD"]; +} + +/** + * @param {Object} params + * @param {ApproversConfig} params.approversConfig + * @param {Record} params.namespacesFound + * @param {Record>} params.formatResults + * @param {boolean} params.isMgmt + * @param {string} params.baseRef + * @param {boolean} params.wasSynchronized + */ +function buildCommentBody({ + approversConfig, + namespacesFound, + formatResults, + isMgmt, + baseRef, + wasSynchronized, +}) { + const planeType = isMgmt ? "Management Plane" : "Data Plane"; + let body = `## Namespace Review Required\n\n**Plane:** ${planeType}\n\n`; + body += `| Language | Proposed Namespace | Format | Status | Approvers |\n`; + body += `|----------|-------------------|--------|--------|----------|\n`; + + for (const [language, namespace] of Object.entries(namespacesFound)) { + const formatResult = formatResults[language]; + const formatStatus = !formatResult ? "—" : formatResult.valid ? "✅" : "⚠️ Invalid"; + body += `| ${language} | \`${namespace}\` | ${formatStatus} | ⏳ Pending | ${getApprovers(approversConfig, isMgmt, language).join(", ")} |\n`; + } + + const formatErrors = Object.values(formatResults).filter((result) => !result.valid); + if (formatErrors.length > 0) { + body += `\n> **⚠️ Format issues detected:**\n`; + for (const error of formatErrors) { + body += `> - **${error.language}:** ${error.error}\n`; + } + body += `>\n> _Format validation does not block approval but should be reviewed._\n`; + } + + body += `\n**How to approve:**\n`; + body += `- Per language: apply \`-namespace-approved\` label\n`; + body += `- All at once: apply \`namespace-approved-all\` label (shortcut for mgmt plane)\n\n`; + body += `Merge is blocked until all languages are approved.\n`; + if (wasSynchronized) { + body += `\n> ⚠️ **Namespace changed** — approvals for affected languages have been reset.\n`; + } + body += `\n_Approver list: [.github/namespace-approvers.yml](../blob/${baseRef}/.github/namespace-approvers.yml)_\n`; + body += `_Process: [.github/workflows/src/namespace-approval/NAMESPACE-REVIEW-PROCESS.md](../blob/${baseRef}/.github/workflows/src/namespace-approval/NAMESPACE-REVIEW-PROCESS.md)_\n`; + body += `_Namespaces extracted via [@azure-tools/typespec-metadata](https://www.npmjs.com/package/@azure-tools/typespec-metadata) emitter_`; + return body; +} + +/** + * @param {import("@actions/github-script").AsyncFunctionArguments} args + */ +export default async function postResults({ github, context, core }) { + const { owner, repo, issue_number, run_id } = await extractInputs(github, context, core); + const approversConfig = await loadApproversConfig(core); + if (!approversConfig) { + return; + } + + const results = run_id ? await downloadNamespaceResults(github, core, owner, repo, run_id) : null; + if (!results) { + return; + } + + const { data: pr } = await github.rest.pulls.get({ + owner, + repo, + pull_number: issue_number, + }); + /** @type {string[]} */ + const existingLabels = pr.labels.map((label) => label.name ?? ""); + const languages = Object.keys(results.namespacesFound); + + if (results.action === "synchronize") { + for (const language of languages) { + const approvedLabel = `${language}-namespace-approved`; + if (existingLabels.includes(approvedLabel)) { + core.info(`Namespace changed for ${language}, resetting approval`); + await removeLabelIfPresent(github, owner, repo, issue_number, approvedLabel); + } + } + + for (const label of ["namespace-approved-all", "namespace-approved"]) { + if (existingLabels.includes(label)) { + await removeLabelIfPresent(github, owner, repo, issue_number, label); + } + } + } + + if (languages.length === 0) { + core.info("No namespace changes detected"); + return; + } + + const labelsToAdd = new Set(["namespace-review-required"]); + if (results.isMgmt) { + labelsToAdd.add("Mgmt"); + } + if (results.isDataPlane) { + labelsToAdd.add("data-plane"); + } + for (const language of languages) { + labelsToAdd.add(`${language}-namespace-pending`); + } + + for (const label of labelsToAdd) { + await ensureLabel(github, owner, repo, label); + } + + await github.rest.issues.addLabels({ + owner, + repo, + issue_number, + labels: [...labelsToAdd], + }); + + const body = buildCommentBody({ + approversConfig, + namespacesFound: results.namespacesFound, + formatResults: results.formatResults, + isMgmt: results.isMgmt, + baseRef: pr.base.ref, + wasSynchronized: results.action === "synchronize", + }); + + await commentOrUpdate(github, core, owner, repo, issue_number, body, "namespace-review-bot"); +} diff --git a/.github/workflows/src/namespace-approval/status-check.js b/.github/workflows/src/namespace-approval/status-check.js index ef0f77f657d1..10640eb5900b 100644 --- a/.github/workflows/src/namespace-approval/status-check.js +++ b/.github/workflows/src/namespace-approval/status-check.js @@ -1,27 +1,29 @@ +import { extractInputs } from "../context.js"; + /** * Verify namespace approval status and set check result. * * @param {import("@actions/github-script").AsyncFunctionArguments} args */ export default async function statusCheck({ github, context, core }) { + const { owner, repo, issue_number } = await extractInputs(github, context, core); const { data: pr } = await github.rest.pulls.get({ - owner: context.repo.owner, - repo: context.repo.repo, - pull_number: /** @type {import("@octokit/webhooks-types").PullRequestEvent} */ (context.payload) - .pull_request.number, + owner, + repo, + pull_number: issue_number, }); /** @type {string[]} */ - const labels = pr.labels.map((l) => l.name ?? ""); + const labels = pr.labels.map((label) => label.name ?? ""); if (!labels.includes("namespace-review-required")) { core.info("No namespace review required, passing"); return; } - const pendingLabels = labels.filter((l) => l.endsWith("-namespace-pending")); + const pendingLabels = labels.filter((label) => label.endsWith("-namespace-pending")); if (pendingLabels.length > 0) { core.setFailed( - `Namespace approval pending for: ${pendingLabels.map((l) => l.replace("-namespace-pending", "")).join(", ")}`, + `Namespace approval pending for: ${pendingLabels.map((label) => label.replace("-namespace-pending", "")).join(", ")}`, ); } else if (labels.includes("namespace-approved")) { core.info("All namespaces approved - merge allowed"); diff --git a/.github/workflows/src/namespace-approval/validate-approval.js b/.github/workflows/src/namespace-approval/validate-approval.js index fd0ede0b9f81..9dfb26ccb9fa 100644 --- a/.github/workflows/src/namespace-approval/validate-approval.js +++ b/.github/workflows/src/namespace-approval/validate-approval.js @@ -1,4 +1,4 @@ -import { readFileSync } from "fs"; +import { readFile } from "fs/promises"; import yaml from "js-yaml"; /** @@ -49,13 +49,11 @@ async function handleUnlabeled({ actor, prNumber, }) { - // Only guard namespace-related labels if (!targetLabel.endsWith("-namespace-pending") && targetLabel !== "namespace-review-required") { core.info(`${targetLabel} is not a namespace label, skipping`); return; } - // Allow bots and authorized approvers to remove labels if (ALLOWED_BOT_LOGINS.includes(actor)) { core.info(`${actor} is a trusted bot, allowing label removal`); return; @@ -68,7 +66,6 @@ async function handleUnlabeled({ return; } - // Unauthorized removal — re-apply the label core.warning(`${actor} is not authorized to remove ${targetLabel}, re-applying`); await github.rest.issues.addLabels({ @@ -126,8 +123,8 @@ async function handleLabeled({ } langsToApprove = labels - .filter((l) => l.endsWith("-namespace-pending")) - .map((l) => l.replace("-namespace-pending", "")); + .filter((label) => label.endsWith("-namespace-pending")) + .map((label) => label.replace("-namespace-pending", "")); } else { const match = targetLabel.match(/^(\w+)-namespace-approved$/); if (!match) { @@ -165,7 +162,6 @@ async function handleLabeled({ core.info(`Approving languages: ${langsToApprove.join(", ")} by ${actor}`); - // Apply per-language approved labels and remove pending labels for (const lang of langsToApprove) { try { await github.rest.issues.removeLabel({ @@ -203,15 +199,14 @@ async function handleLabeled({ } } - // Update bot comment const { data: currentPR } = await github.rest.pulls.get({ owner: context.repo.owner, repo: context.repo.repo, pull_number: prNumber, }); /** @type {string[]} */ - const currentLabels = currentPR.labels.map((l) => l.name ?? ""); - const pendingLabels = currentLabels.filter((l) => l.endsWith("-namespace-pending")); + const currentLabels = currentPR.labels.map((label) => label.name ?? ""); + const pendingLabels = currentLabels.filter((label) => label.endsWith("-namespace-pending")); const comments = await github.rest.issues.listComments({ owner: context.repo.owner, @@ -219,7 +214,7 @@ async function handleLabeled({ issue_number: prNumber, }); const botComment = comments.data.find( - (c) => c.user?.type === "Bot" && c.body?.includes(""), + (comment) => comment.user?.type === "Bot" && comment.body?.includes(""), ); if (botComment?.body) { @@ -287,10 +282,10 @@ export default async function validateApproval({ github, context, core }) { /** @type {ApproversConfig} */ let approversConfig; try { - const content = readFileSync(".github/namespace-approvers.yml", "utf8"); + const content = await readFile(".github/namespace-approvers.yml", "utf8"); approversConfig = /** @type {ApproversConfig} */ (yaml.load(content)); - } catch (e) { - core.setFailed("Failed to load .github/namespace-approvers.yml: " + String(e)); + } catch (error) { + core.setFailed("Failed to load .github/namespace-approvers.yml: " + String(error)); return; } @@ -301,12 +296,11 @@ export default async function validateApproval({ github, context, core }) { const prNumber = payload.pull_request.number; /** @type {string[]} */ - const labels = payload.pull_request.labels.map((l) => l.name); + const labels = payload.pull_request.labels.map((label) => label.name); const targetLabel = payload.label.name; const actor = payload.sender.login; const isMgmt = labels.includes("Mgmt"); - // Handle unlabeled: guard against unauthorized removal of pending/required labels if (payload.action === "unlabeled") { return await handleUnlabeled({ github, @@ -320,13 +314,11 @@ export default async function validateApproval({ github, context, core }) { }); } - // Skip if no namespace review is required if (!labels.includes("namespace-review-required")) { core.info("No namespace review, skipping"); return; } - // Handle labeled: validate approval labels return await handleLabeled({ github, context, diff --git a/.github/workflows/test/namespace-approval/status-check.test.js b/.github/workflows/test/namespace-approval/status-check.test.js index b28a4bcaccfd..be8b75281fc7 100644 --- a/.github/workflows/test/namespace-approval/status-check.test.js +++ b/.github/workflows/test/namespace-approval/status-check.test.js @@ -1,7 +1,13 @@ import { beforeEach, describe, expect, it, vi } from "vitest"; -import statusCheck from "../../src/namespace-approval/status-check.js"; import { createMockContext, createMockCore, createMockGithub } from "../mocks.js"; +vi.mock("../../src/context.js", () => ({ + extractInputs: vi.fn(), +})); + +import { extractInputs } from "../../src/context.js"; +import statusCheck from "../../src/namespace-approval/status-check.js"; + describe("status-check", () => { /** @type {ReturnType} */ let github; @@ -22,10 +28,16 @@ describe("status-check", () => { github = createMockGithub(); context = createMockContext(); core = createMockCore(); + vi.mocked(extractInputs).mockResolvedValue({ + owner: "owner", + repo: "repo", + issue_number: 42, + head_sha: "head", + run_id: Number.NaN, + }); }); it("should pass when no namespace-review-required label", async () => { - context.payload = { pull_request: { number: 42 } }; github.rest.pulls.get.mockResolvedValue({ data: { labels: [{ name: "other-label" }] }, }); @@ -37,7 +49,6 @@ describe("status-check", () => { }); it("should fail when pending labels remain", async () => { - context.payload = { pull_request: { number: 42 } }; github.rest.pulls.get.mockResolvedValue({ data: { labels: [ @@ -56,7 +67,6 @@ describe("status-check", () => { }); it("should pass when all namespaces approved", async () => { - context.payload = { pull_request: { number: 42 } }; github.rest.pulls.get.mockResolvedValue({ data: { labels: [ diff --git a/.github/workflows/test/namespace-approval/validate-approval.test.js b/.github/workflows/test/namespace-approval/validate-approval.test.js index 77992c067b4b..6975769c155c 100644 --- a/.github/workflows/test/namespace-approval/validate-approval.test.js +++ b/.github/workflows/test/namespace-approval/validate-approval.test.js @@ -3,15 +3,14 @@ import { createMockContext, createMockCore, createMockGithub } from "../mocks.js // cspell:words jsquire xirzec -// Mock fs and js-yaml so validate-approval.js doesn't hit disk -vi.mock("fs", () => ({ - readFileSync: vi.fn(), +vi.mock("fs/promises", () => ({ + readFile: vi.fn(), })); vi.mock("js-yaml", () => ({ default: { load: vi.fn() }, })); -import { readFileSync } from "fs"; +import { readFile } from "fs/promises"; import yaml from "js-yaml"; import validateApproval from "../../src/namespace-approval/validate-approval.js"; @@ -28,7 +27,7 @@ const approversConfig = { }; function setupMocks() { - /** @type {ReturnType} */ (readFileSync).mockReturnValue("yaml-content"); + /** @type {ReturnType} */ (readFile).mockResolvedValue("yaml-content"); /** @type {ReturnType} */ (yaml.load).mockReturnValue(approversConfig); } @@ -77,7 +76,6 @@ describe("validate-approval", () => { context = createMockContext(); core = createMockCore(); - // Extend mock with methods used by validate-approval /** @type {any} */ (github.rest.issues).createComment = vi.fn(); /** @type {any} */ (github.rest.issues).getLabel = vi.fn(); /** @type {any} */ (github.rest.issues).createLabel = vi.fn(); @@ -125,9 +123,7 @@ describe("validate-approval", () => { github.rest.pulls.get.mockResolvedValue({ data: { labels: [{ name: "namespace-review-required" }] }, }); - /** @type {any} */ (github.rest.issues).listComments = vi - .fn() - .mockResolvedValue({ data: [] }); + /** @type {any} */ (github.rest.issues).listComments = vi.fn().mockResolvedValue({ data: [] }); await validateApproval(args()); @@ -167,9 +163,7 @@ describe("validate-approval", () => { github.rest.pulls.get.mockResolvedValue({ data: { labels: [{ name: "namespace-review-required" }] }, }); - /** @type {any} */ (github.rest.issues).listComments = vi - .fn() - .mockResolvedValue({ data: [] }); + /** @type {any} */ (github.rest.issues).listComments = vi.fn().mockResolvedValue({ data: [] }); await validateApproval(args()); @@ -191,9 +185,7 @@ describe("validate-approval", () => { github.rest.pulls.get.mockResolvedValue({ data: { labels: [] }, }); - /** @type {any} */ (github.rest.issues).listComments = vi - .fn() - .mockResolvedValue({ data: [] }); + /** @type {any} */ (github.rest.issues).listComments = vi.fn().mockResolvedValue({ data: [] }); await validateApproval(args()); @@ -258,8 +250,10 @@ describe("validate-approval", () => { await validateApproval(args()); expect(github.rest.issues.addLabels).not.toHaveBeenCalled(); - const infoArgs = core.info.mock.calls.map((/** @type {unknown[]} */ c) => String(c[0])); - expect(infoArgs.some((/** @type {string} */ m) => m.includes("trusted bot"))).toBe(true); + const infoArgs = core.info.mock.calls.map((/** @type {unknown[]} */ call) => String(call[0])); + expect(infoArgs.some((/** @type {string} */ message) => message.includes("trusted bot"))).toBe( + true, + ); }); it("should allow authorized approver to remove pending label", async () => { @@ -273,10 +267,10 @@ describe("validate-approval", () => { await validateApproval(args()); expect(github.rest.issues.addLabels).not.toHaveBeenCalled(); - const infoArgs = core.info.mock.calls.map((/** @type {unknown[]} */ c) => String(c[0])); - expect(infoArgs.some((/** @type {string} */ m) => m.includes("authorized approver"))).toBe( - true, - ); + const infoArgs = core.info.mock.calls.map((/** @type {unknown[]} */ call) => String(call[0])); + expect( + infoArgs.some((/** @type {string} */ message) => message.includes("authorized approver")), + ).toBe(true); }); it("should ignore non-namespace labels on unlabeled", async () => { @@ -290,8 +284,8 @@ describe("validate-approval", () => { await validateApproval(args()); expect(github.rest.issues.addLabels).not.toHaveBeenCalled(); - const infoArgs = core.info.mock.calls.map((/** @type {unknown[]} */ c) => String(c[0])); - expect(infoArgs.some((/** @type {string} */ m) => m.includes("not a namespace label"))).toBe( + const infoArgs = core.info.mock.calls.map((/** @type {unknown[]} */ call) => String(call[0])); + expect(infoArgs.some((/** @type {string} */ message) => message.includes("not a namespace label"))).toBe( true, ); }); From 01ea7c635233a020d4d7067b2b197cc6869eb4b0 Mon Sep 17 00:00:00 2001 From: Sameeksha Vaity Date: Tue, 23 Jun 2026 14:39:30 -0700 Subject: [PATCH 05/23] Fix CI: prettier formatting and cspell word - Run prettier --write on 6 files to match repo style - Add cspell:ignore for hex color fbbf24 in post-results.js - Protected Files check requires maintainer approval (expected) Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- .../workflows/namespace-approval-code.yaml | 1 - .../NAMESPACE-REVIEW-PROCESS.md | 14 +++++------ .../namespace-approval/detect-namespaces.js | 6 ++++- .../src/namespace-approval/post-results.js | 3 ++- .../namespace-approval/validate-approval.js | 3 ++- .../validate-approval.test.js | 24 ++++++++++++------- 6 files changed, 31 insertions(+), 20 deletions(-) diff --git a/.github/workflows/namespace-approval-code.yaml b/.github/workflows/namespace-approval-code.yaml index 2199d5378ea8..8c81396d2302 100644 --- a/.github/workflows/namespace-approval-code.yaml +++ b/.github/workflows/namespace-approval-code.yaml @@ -41,4 +41,3 @@ jobs: name: namespace-results path: ${{ steps.detect.outputs.results_path }} if-no-files-found: ignore - diff --git a/.github/workflows/src/namespace-approval/NAMESPACE-REVIEW-PROCESS.md b/.github/workflows/src/namespace-approval/NAMESPACE-REVIEW-PROCESS.md index 1fbfe35ac0b1..254d762b814c 100644 --- a/.github/workflows/src/namespace-approval/NAMESPACE-REVIEW-PROCESS.md +++ b/.github/workflows/src/namespace-approval/NAMESPACE-REVIEW-PROCESS.md @@ -57,14 +57,14 @@ Defined in [`.github/namespace-approvers.yml`](../../../namespace-approvers.yml) -| Scope | Approvers | -| -------------------- | ------------------------------ | -| .NET | @jsquire, @m-nash | +| Scope | Approvers | +| -------------------- | ----------------------------- | +| .NET | @jsquire, @m-nash | | Java | @JonathanGiles, @alzimmermsft | -| Python | @xirzec, @kashifkhan | -| TypeScript | @xirzec, @maorleger | -| Go | @jhendrixMSFT | -| Mgmt (all languages) | @ArthurMa1978, @m-nash | +| Python | @xirzec, @kashifkhan | +| TypeScript | @xirzec, @maorleger | +| Go | @jhendrixMSFT | +| Mgmt (all languages) | @ArthurMa1978, @m-nash | diff --git a/.github/workflows/src/namespace-approval/detect-namespaces.js b/.github/workflows/src/namespace-approval/detect-namespaces.js index 215f27fa87f2..9d88e79d57b4 100644 --- a/.github/workflows/src/namespace-approval/detect-namespaces.js +++ b/.github/workflows/src/namespace-approval/detect-namespaces.js @@ -97,7 +97,11 @@ async function extractNamespaces(file, namespacesFound, core) { isDataPlane = true; } - const outputDir = join(process.env.RUNNER_TEMP || tmpdir(), "typespec-metadata", basename(tspDir)); + const outputDir = join( + process.env.RUNNER_TEMP || tmpdir(), + "typespec-metadata", + basename(tspDir), + ); await rm(outputDir, { recursive: true, force: true }); core.info(`Running typespec-metadata emitter on ${tspDir}`); diff --git a/.github/workflows/src/namespace-approval/post-results.js b/.github/workflows/src/namespace-approval/post-results.js index 73ada11dcf95..2be469ca263d 100644 --- a/.github/workflows/src/namespace-approval/post-results.js +++ b/.github/workflows/src/namespace-approval/post-results.js @@ -1,7 +1,7 @@ import { readFile, unlink, writeFile } from "fs/promises"; +import yaml from "js-yaml"; import { tmpdir } from "os"; import { join } from "path"; -import yaml from "js-yaml"; import { z } from "zod"; import { execFile } from "../../../shared/src/exec.js"; import { PER_PAGE_MAX } from "../../../shared/src/github.js"; @@ -117,6 +117,7 @@ async function ensureLabel(github, owner, repo, label) { owner, repo, name: label, + // cspell:ignore fbbf color: LABEL_COLORS[label] ?? (label.includes("pending") ? "fbbf24" : "94a3b8"), }); } diff --git a/.github/workflows/src/namespace-approval/validate-approval.js b/.github/workflows/src/namespace-approval/validate-approval.js index 9dfb26ccb9fa..23b88d9444d6 100644 --- a/.github/workflows/src/namespace-approval/validate-approval.js +++ b/.github/workflows/src/namespace-approval/validate-approval.js @@ -214,7 +214,8 @@ async function handleLabeled({ issue_number: prNumber, }); const botComment = comments.data.find( - (comment) => comment.user?.type === "Bot" && comment.body?.includes(""), + (comment) => + comment.user?.type === "Bot" && comment.body?.includes(""), ); if (botComment?.body) { diff --git a/.github/workflows/test/namespace-approval/validate-approval.test.js b/.github/workflows/test/namespace-approval/validate-approval.test.js index 6975769c155c..5eb884d05d43 100644 --- a/.github/workflows/test/namespace-approval/validate-approval.test.js +++ b/.github/workflows/test/namespace-approval/validate-approval.test.js @@ -123,7 +123,9 @@ describe("validate-approval", () => { github.rest.pulls.get.mockResolvedValue({ data: { labels: [{ name: "namespace-review-required" }] }, }); - /** @type {any} */ (github.rest.issues).listComments = vi.fn().mockResolvedValue({ data: [] }); + /** @type {any} */ (github.rest.issues).listComments = vi + .fn() + .mockResolvedValue({ data: [] }); await validateApproval(args()); @@ -163,7 +165,9 @@ describe("validate-approval", () => { github.rest.pulls.get.mockResolvedValue({ data: { labels: [{ name: "namespace-review-required" }] }, }); - /** @type {any} */ (github.rest.issues).listComments = vi.fn().mockResolvedValue({ data: [] }); + /** @type {any} */ (github.rest.issues).listComments = vi + .fn() + .mockResolvedValue({ data: [] }); await validateApproval(args()); @@ -185,7 +189,9 @@ describe("validate-approval", () => { github.rest.pulls.get.mockResolvedValue({ data: { labels: [] }, }); - /** @type {any} */ (github.rest.issues).listComments = vi.fn().mockResolvedValue({ data: [] }); + /** @type {any} */ (github.rest.issues).listComments = vi + .fn() + .mockResolvedValue({ data: [] }); await validateApproval(args()); @@ -251,9 +257,9 @@ describe("validate-approval", () => { expect(github.rest.issues.addLabels).not.toHaveBeenCalled(); const infoArgs = core.info.mock.calls.map((/** @type {unknown[]} */ call) => String(call[0])); - expect(infoArgs.some((/** @type {string} */ message) => message.includes("trusted bot"))).toBe( - true, - ); + expect( + infoArgs.some((/** @type {string} */ message) => message.includes("trusted bot")), + ).toBe(true); }); it("should allow authorized approver to remove pending label", async () => { @@ -285,9 +291,9 @@ describe("validate-approval", () => { expect(github.rest.issues.addLabels).not.toHaveBeenCalled(); const infoArgs = core.info.mock.calls.map((/** @type {unknown[]} */ call) => String(call[0])); - expect(infoArgs.some((/** @type {string} */ message) => message.includes("not a namespace label"))).toBe( - true, - ); + expect( + infoArgs.some((/** @type {string} */ message) => message.includes("not a namespace label")), + ).toBe(true); }); it("should guard namespace-review-required label too", async () => { From cf496efbb84adb97152f4991bb0d48b7c974ff82 Mon Sep 17 00:00:00 2001 From: Sameeksha Vaity Date: Tue, 23 Jun 2026 17:30:03 -0700 Subject: [PATCH 06/23] Fix: use sparse-checkout add instead of git checkout for hydration git checkout HEAD -- fails when sparse-checkout cone mode is active and the path is outside the cone. Use git sparse-checkout add to expand the cone, which materializes the files properly. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- .github/workflows/src/namespace-approval/detect-namespaces.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.github/workflows/src/namespace-approval/detect-namespaces.js b/.github/workflows/src/namespace-approval/detect-namespaces.js index 9d88e79d57b4..0104aaa813c9 100644 --- a/.github/workflows/src/namespace-approval/detect-namespaces.js +++ b/.github/workflows/src/namespace-approval/detect-namespaces.js @@ -66,7 +66,7 @@ async function ensureTypeSpecFilesAvailable(file, core) { const tspDir = dirname(file); const workspace = process.env.GITHUB_WORKSPACE ?? process.cwd(); core.info(`Hydrating ${tspDir} from git for namespace detection`); - await execFile("git", ["checkout", "HEAD", "--", tspDir], { + await execFile("git", ["sparse-checkout", "add", tspDir], { cwd: workspace, }); } From c11675fcefafb08e3b75ae110f85567a5e436f99 Mon Sep 17 00:00:00 2001 From: Sameeksha Vaity Date: Wed, 24 Jun 2026 13:13:00 -0700 Subject: [PATCH 07/23] Parse tspconfig.yaml directly, add commit status gate, fix plane detection - Replace tsp compile with direct YAML parsing (works with sparse checkout) - Add module/crate-name parsing for Go/Rust emitters - Create commit status (Namespace Approval) as single PR merge gate - Fix plane detection: linter extends overrides path-based guess - Update docs and bot comment to reflect YAML parsing approach Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- .../workflows/namespace-approval-status.yaml | 1 + .../NAMESPACE-REVIEW-PROCESS.md | 4 +- .../namespace-approval/detect-namespaces.js | 166 +++++++++--------- .../src/namespace-approval/post-results.js | 2 +- .../src/namespace-approval/status-check.js | 31 +++- .../namespace-approval/status-check.test.js | 20 ++- 6 files changed, 134 insertions(+), 90 deletions(-) diff --git a/.github/workflows/namespace-approval-status.yaml b/.github/workflows/namespace-approval-status.yaml index 47c033fd107a..2cdfb483ac34 100644 --- a/.github/workflows/namespace-approval-status.yaml +++ b/.github/workflows/namespace-approval-status.yaml @@ -13,6 +13,7 @@ permissions: actions: read contents: read pull-requests: write + statuses: write jobs: post-results: diff --git a/.github/workflows/src/namespace-approval/NAMESPACE-REVIEW-PROCESS.md b/.github/workflows/src/namespace-approval/NAMESPACE-REVIEW-PROCESS.md index 254d762b814c..12f62514b62c 100644 --- a/.github/workflows/src/namespace-approval/NAMESPACE-REVIEW-PROCESS.md +++ b/.github/workflows/src/namespace-approval/NAMESPACE-REVIEW-PROCESS.md @@ -13,8 +13,8 @@ This process is automated via GitHub Actions on this repository. When a PR modifies any `tspconfig.yaml` file, the analyze-code workflow: 1. Identifies which languages have namespace/package-name configuration -2. Extracts namespaces using the `@azure-tools/typespec-metadata` emitter -3. Determines if the service is data-plane or management-plane +2. Extracts namespaces by parsing `tspconfig.yaml` emitter options directly +3. Determines if the service is data-plane or management-plane (via path and linter config) 4. Uploads namespace results for the status workflow ### Status + Approval diff --git a/.github/workflows/src/namespace-approval/detect-namespaces.js b/.github/workflows/src/namespace-approval/detect-namespaces.js index 0104aaa813c9..8231214da559 100644 --- a/.github/workflows/src/namespace-approval/detect-namespaces.js +++ b/.github/workflows/src/namespace-approval/detect-namespaces.js @@ -1,42 +1,23 @@ -import { access, readFile, rm, writeFile } from "fs/promises"; -import { tmpdir } from "os"; -import { basename, dirname, join } from "path"; -import { z } from "zod"; +import { access, readFile, writeFile } from "fs/promises"; +import yaml from "js-yaml"; +import { dirname, join } from "path"; import { getChangedFiles } from "../../../shared/src/changed-files.js"; -import { execFile, execNpmExec } from "../../../shared/src/exec.js"; +import { execFile } from "../../../shared/src/exec.js"; import { loadFormatRules, validateNamespaceFormat } from "./validate-format.js"; -const TypeSpecMetadataSchema = z.object({ - typespec: z - .object({ - type: z.enum(["management", "data"]).optional(), - }) - .optional(), - languages: z - .record( - z.string(), - z.array( - z.object({ - namespace: z.string().optional(), - packageName: z.string().optional(), - }), - ), - ) - .optional(), -}); - -/** @typedef {{ namespace?: string, packageName?: string }} TypeSpecMetadataEntry */ -/** @typedef {Record} TypeSpecLanguages */ - -/** @type {Record} */ +/** + * Map of emitter package name suffix to normalized language key. + * @type {Record} + */ const EMITTER_TO_LANG = { - csharp: "dotnet", + "typespec-csharp": "dotnet", "http-client-csharp": "dotnet", "http-client-csharp-mgmt": "dotnet", - java: "java", - python: "python", - typescript: "typescript", - go: "go", + "typespec-java": "java", + "typespec-python": "python", + "typespec-ts": "typescript", + "typespec-go": "go", + "typespec-rust": "rust", }; /** @@ -72,7 +53,24 @@ async function ensureTypeSpecFilesAvailable(file, core) { } /** - * Extract namespaces from tspconfig.yaml using typespec-metadata emitter. + * Resolve the normalized language from an emitter package name. + * e.g. "@azure-tools/typespec-java" → "java" + * + * @param {string} emitterKey - Full emitter package name (e.g. "@azure-tools/typespec-java") + * @returns {string | undefined} + */ +function resolveLanguage(emitterKey) { + for (const [suffix, lang] of Object.entries(EMITTER_TO_LANG)) { + if (emitterKey.endsWith(`/${suffix}`) || emitterKey === suffix) { + return lang; + } + } + return undefined; +} + +/** + * Extract namespaces directly from tspconfig.yaml emitter options. + * Parses the `options` section to find `namespace` or `package-details.name` per language emitter. * * @param {string} file - Path to tspconfig.yaml * @param {Record} namespacesFound - Map of language to namespace (mutated) @@ -89,65 +87,75 @@ async function extractNamespaces(file, namespacesFound, core) { return { isMgmt, isDataPlane }; } - const tspDir = dirname(file); - if (file.includes(".Management/") || file.includes("/resource-manager/")) { isMgmt = true; } else { isDataPlane = true; } - const outputDir = join( - process.env.RUNNER_TEMP || tmpdir(), - "typespec-metadata", - basename(tspDir), - ); - await rm(outputDir, { recursive: true, force: true }); - - core.info(`Running typespec-metadata emitter on ${tspDir}`); - await execNpmExec( - [ - "tsp", - "compile", - tspDir, - "--emit", - "@azure-tools/typespec-metadata", - "--output-dir", - outputDir, - "--option", - "@azure-tools/typespec-metadata.format=json", - ], - { - cwd: process.env.GITHUB_WORKSPACE ?? process.cwd(), - }, - ); + core.info(`Parsing tspconfig.yaml for namespace info: ${file}`); + const content = await readFile(file, "utf8"); + const config = /** @type {Record} */ (yaml.load(content)); - const metadataPath = join( - outputDir, - "@azure-tools", - "typespec-metadata", - "typespec-metadata.json", - ); - const metadata = TypeSpecMetadataSchema.parse(JSON.parse(await readFile(metadataPath, "utf8"))); + if (!config || typeof config !== "object") { + core.warning(`Could not parse tspconfig.yaml: ${file}`); + return { isMgmt, isDataPlane }; + } - if (metadata.typespec?.type === "management") { - isMgmt = true; + // Linter extends is the most reliable plane indicator — override path-based guess + const linter = /** @type {Record | undefined} */ (config.linter); + if (linter) { + const linterExtends = /** @type {string[] | undefined} */ (linter.extends); + if (linterExtends?.some((e) => e.includes("resource-manager"))) { + isMgmt = true; + isDataPlane = false; + } else if (linterExtends?.some((e) => e.includes("data-plane"))) { + isDataPlane = true; + isMgmt = false; + } } - if (metadata.typespec?.type === "data") { - isDataPlane = true; + + const options = /** @type {Record> | undefined} */ ( + config.options + ); + if (!options) { + core.info(`No emitter options found in ${file}`); + return { isMgmt, isDataPlane }; } - const languages = /** @type {TypeSpecLanguages} */ (metadata.languages ?? {}); - for (const [langKey, entries] of Object.entries(languages)) { - const lang = EMITTER_TO_LANG[langKey] ?? langKey; - const firstEntry = entries[0]; - const namespace = firstEntry?.namespace ?? firstEntry?.packageName; - if (namespace) { - namespacesFound[lang] = namespace; + for (const [emitterKey, emitterOpts] of Object.entries(options)) { + const lang = resolveLanguage(emitterKey); + if (!lang || !emitterOpts || typeof emitterOpts !== "object") { + continue; + } + + // Prefer `namespace`, fall back to `package-details.name`, `module`, `crate-name` + const ns = /** @type {string | undefined} */ (emitterOpts.namespace); + if (ns) { + namespacesFound[lang] = ns; + continue; + } + + const packageDetails = /** @type {Record | undefined} */ ( + emitterOpts["package-details"] + ); + if (packageDetails?.name) { + namespacesFound[lang] = String(packageDetails.name); + continue; + } + + const module = /** @type {string | undefined} */ (emitterOpts.module); + if (module) { + namespacesFound[lang] = module; + continue; + } + + const crateName = /** @type {string | undefined} */ (emitterOpts["crate-name"]); + if (crateName) { + namespacesFound[lang] = crateName; } } - await rm(outputDir, { recursive: true, force: true }); return { isMgmt, isDataPlane }; } diff --git a/.github/workflows/src/namespace-approval/post-results.js b/.github/workflows/src/namespace-approval/post-results.js index 2be469ca263d..4e113f2eece4 100644 --- a/.github/workflows/src/namespace-approval/post-results.js +++ b/.github/workflows/src/namespace-approval/post-results.js @@ -209,7 +209,7 @@ function buildCommentBody({ } body += `\n_Approver list: [.github/namespace-approvers.yml](../blob/${baseRef}/.github/namespace-approvers.yml)_\n`; body += `_Process: [.github/workflows/src/namespace-approval/NAMESPACE-REVIEW-PROCESS.md](../blob/${baseRef}/.github/workflows/src/namespace-approval/NAMESPACE-REVIEW-PROCESS.md)_\n`; - body += `_Namespaces extracted via [@azure-tools/typespec-metadata](https://www.npmjs.com/package/@azure-tools/typespec-metadata) emitter_`; + body += `_Namespaces extracted from tspconfig.yaml emitter options_`; return body; } diff --git a/.github/workflows/src/namespace-approval/status-check.js b/.github/workflows/src/namespace-approval/status-check.js index 10640eb5900b..62b1542148bd 100644 --- a/.github/workflows/src/namespace-approval/status-check.js +++ b/.github/workflows/src/namespace-approval/status-check.js @@ -1,7 +1,7 @@ import { extractInputs } from "../context.js"; /** - * Verify namespace approval status and set check result. + * Verify namespace approval status and report as a commit status on the PR. * * @param {import("@actions/github-script").AsyncFunctionArguments} args */ @@ -17,15 +17,38 @@ export default async function statusCheck({ github, context, core }) { if (!labels.includes("namespace-review-required")) { core.info("No namespace review required, passing"); + await github.rest.repos.createCommitStatus({ + owner, + repo, + sha: pr.head.sha, + state: "success", + context: "Namespace Approval", + description: "No namespace review required", + }); return; } const pendingLabels = labels.filter((label) => label.endsWith("-namespace-pending")); if (pendingLabels.length > 0) { - core.setFailed( - `Namespace approval pending for: ${pendingLabels.map((label) => label.replace("-namespace-pending", "")).join(", ")}`, - ); + const pending = pendingLabels.map((l) => l.replace("-namespace-pending", "")).join(", "); + await github.rest.repos.createCommitStatus({ + owner, + repo, + sha: pr.head.sha, + state: "pending", + context: "Namespace Approval", + description: `Pending: ${pending}`, + }); + core.info(`Namespace approval pending for: ${pending}`); } else if (labels.includes("namespace-approved")) { core.info("All namespaces approved - merge allowed"); + await github.rest.repos.createCommitStatus({ + owner, + repo, + sha: pr.head.sha, + state: "success", + context: "Namespace Approval", + description: "All namespaces approved", + }); } } diff --git a/.github/workflows/test/namespace-approval/status-check.test.js b/.github/workflows/test/namespace-approval/status-check.test.js index be8b75281fc7..45fb3419a686 100644 --- a/.github/workflows/test/namespace-approval/status-check.test.js +++ b/.github/workflows/test/namespace-approval/status-check.test.js @@ -39,18 +39,22 @@ describe("status-check", () => { it("should pass when no namespace-review-required label", async () => { github.rest.pulls.get.mockResolvedValue({ - data: { labels: [{ name: "other-label" }] }, + data: { labels: [{ name: "other-label" }], head: { sha: "abc123" } }, }); await statusCheck(args()); expect(core.info).toHaveBeenCalledWith("No namespace review required, passing"); expect(core.setFailed).not.toHaveBeenCalled(); + expect(github.rest.repos.createCommitStatus).toHaveBeenCalledWith( + expect.objectContaining({ state: "success", context: "Namespace Approval" }), + ); }); it("should fail when pending labels remain", async () => { github.rest.pulls.get.mockResolvedValue({ data: { + head: { sha: "abc123" }, labels: [ { name: "namespace-review-required" }, { name: "java-namespace-pending" }, @@ -61,14 +65,19 @@ describe("status-check", () => { await statusCheck(args()); - const failMsg = String(core.setFailed.mock.calls[0][0]); - expect(failMsg).toContain("java"); - expect(failMsg).toContain("dotnet"); + const infoMsg = String(core.info.mock.calls.find((c) => String(c[0]).includes("pending"))?.[0]); + expect(infoMsg).toContain("java"); + expect(infoMsg).toContain("dotnet"); + expect(core.setFailed).not.toHaveBeenCalled(); + expect(github.rest.repos.createCommitStatus).toHaveBeenCalledWith( + expect.objectContaining({ state: "pending", context: "Namespace Approval" }), + ); }); it("should pass when all namespaces approved", async () => { github.rest.pulls.get.mockResolvedValue({ data: { + head: { sha: "abc123" }, labels: [ { name: "namespace-review-required" }, { name: "namespace-approved" }, @@ -81,5 +90,8 @@ describe("status-check", () => { expect(core.info).toHaveBeenCalledWith("All namespaces approved - merge allowed"); expect(core.setFailed).not.toHaveBeenCalled(); + expect(github.rest.repos.createCommitStatus).toHaveBeenCalledWith( + expect.objectContaining({ state: "success", context: "Namespace Approval" }), + ); }); }); From 1b9a3b6fbaaaf689641c492dd763e677baa8a1ac Mon Sep 17 00:00:00 2001 From: Sameeksha Vaity Date: Wed, 24 Jun 2026 13:40:05 -0700 Subject: [PATCH 08/23] Split status workflow into separate files following repo convention Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- .../workflows/namespace-approval-status.yaml | 47 +------------------ .../namespace-approval-validation.yaml | 41 ++++++++++++++++ 2 files changed, 42 insertions(+), 46 deletions(-) create mode 100644 .github/workflows/namespace-approval-validation.yaml diff --git a/.github/workflows/namespace-approval-status.yaml b/.github/workflows/namespace-approval-status.yaml index 2cdfb483ac34..f639d723f591 100644 --- a/.github/workflows/namespace-approval-status.yaml +++ b/.github/workflows/namespace-approval-status.yaml @@ -1,10 +1,6 @@ name: "Namespace Approval - Set Status" on: - pull_request_target: - types: - - labeled - - unlabeled workflow_run: workflows: ["Namespace Approval - Analyze Code"] types: [completed] @@ -17,7 +13,6 @@ permissions: jobs: post-results: - if: github.event_name == 'workflow_run' name: Post Namespace Results runs-on: ubuntu-24.04 steps: @@ -26,9 +21,6 @@ jobs: sparse-checkout: | .github/ - - name: Setup Node and install dependencies - uses: ./.github/actions/setup-node-install-deps - - name: Install dependencies for github-script actions uses: ./.github/actions/install-deps-github-script @@ -40,44 +32,7 @@ jobs: await import('${{ github.workspace }}/.github/workflows/src/namespace-approval/post-results.js'); await postResults({ github, context, core }); - validate-approval: - if: > - github.event_name == 'pull_request_target' && - (github.event.action == 'labeled' || github.event.action == 'unlabeled') - name: Validate Namespace Approval - runs-on: ubuntu-24.04 - steps: - - uses: actions/checkout@v6 - with: - sparse-checkout: | - .github/ - - - name: Install dependencies for github-script actions - uses: ./.github/actions/install-deps-github-script - - - name: Validate namespace approval label - uses: actions/github-script@v8 - with: - script: | - const { default: validateApproval } = - await import('${{ github.workspace }}/.github/workflows/src/namespace-approval/validate-approval.js'); - await validateApproval({ github, context, core }); - - status-check: - name: Namespace Approval Status - needs: [post-results, validate-approval] - if: always() - runs-on: ubuntu-24.04 - steps: - - uses: actions/checkout@v6 - with: - sparse-checkout: | - .github/ - - - name: Install dependencies for github-script actions - uses: ./.github/actions/install-deps-github-script - - - name: Verify namespace approval status + - name: Set namespace approval status uses: actions/github-script@v8 with: script: | diff --git a/.github/workflows/namespace-approval-validation.yaml b/.github/workflows/namespace-approval-validation.yaml new file mode 100644 index 000000000000..4c1323a6d7cb --- /dev/null +++ b/.github/workflows/namespace-approval-validation.yaml @@ -0,0 +1,41 @@ +name: "Namespace Approval - Validate Labels" + +on: + pull_request_target: + types: + - labeled + - unlabeled + +permissions: + contents: read + pull-requests: write + statuses: write + +jobs: + validate-approval: + name: Validate Namespace Approval + runs-on: ubuntu-24.04 + steps: + - uses: actions/checkout@v6 + with: + sparse-checkout: | + .github/ + + - name: Install dependencies for github-script actions + uses: ./.github/actions/install-deps-github-script + + - name: Validate namespace approval label + uses: actions/github-script@v8 + with: + script: | + const { default: validateApproval } = + await import('${{ github.workspace }}/.github/workflows/src/namespace-approval/validate-approval.js'); + await validateApproval({ github, context, core }); + + - name: Update namespace approval status + uses: actions/github-script@v8 + with: + script: | + const { default: statusCheck } = + await import('${{ github.workspace }}/.github/workflows/src/namespace-approval/status-check.js'); + await statusCheck({ github, context, core }); From 02ccf7e38601408551d6ab09e538a3b657bd7d1f Mon Sep 17 00:00:00 2001 From: Sameeksha Vaity Date: Thu, 25 Jun 2026 16:08:57 -0700 Subject: [PATCH 09/23] Fix: deduplicate job name in code workflow to match repo convention --- .github/workflows/namespace-approval-code.yaml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.github/workflows/namespace-approval-code.yaml b/.github/workflows/namespace-approval-code.yaml index 8c81396d2302..880bfd3d89c6 100644 --- a/.github/workflows/namespace-approval-code.yaml +++ b/.github/workflows/namespace-approval-code.yaml @@ -13,7 +13,7 @@ permissions: jobs: detect-namespaces: - name: "Namespace Approval - Analyze Code" + name: Analyze Code runs-on: ubuntu-24.04 steps: - uses: actions/checkout@v6 From a55968cd5cbb436a9f0a8859ce68a8414b658bce Mon Sep 17 00:00:00 2001 From: Sameeksha Vaity Date: Mon, 29 Jun 2026 11:35:09 -0700 Subject: [PATCH 10/23] Address review: add else fallback in status check, fix check name in docs --- .../NAMESPACE-REVIEW-PROCESS.md | 2 +- .../src/namespace-approval/status-check.js | 10 +++++++++ .../namespace-approval/status-check.test.js | 22 +++++++++++++++++++ 3 files changed, 33 insertions(+), 1 deletion(-) diff --git a/.github/workflows/src/namespace-approval/NAMESPACE-REVIEW-PROCESS.md b/.github/workflows/src/namespace-approval/NAMESPACE-REVIEW-PROCESS.md index 12f62514b62c..5179c60a37d8 100644 --- a/.github/workflows/src/namespace-approval/NAMESPACE-REVIEW-PROCESS.md +++ b/.github/workflows/src/namespace-approval/NAMESPACE-REVIEW-PROCESS.md @@ -26,7 +26,7 @@ The status workflow posts the namespace review comment, applies `-namespac ### Status Check -A required status check (`namespace-approval`) blocks merge until all pending namespaces are approved. The check: +A required status check (`Namespace Approval`) blocks merge until all pending namespaces are approved. The check: - ✅ Passes when all `*-namespace-pending` labels have corresponding `*-namespace-approved` labels - ❌ Fails when any pending namespace lacks approval diff --git a/.github/workflows/src/namespace-approval/status-check.js b/.github/workflows/src/namespace-approval/status-check.js index 62b1542148bd..7b77482f74d8 100644 --- a/.github/workflows/src/namespace-approval/status-check.js +++ b/.github/workflows/src/namespace-approval/status-check.js @@ -50,5 +50,15 @@ export default async function statusCheck({ github, context, core }) { context: "Namespace Approval", description: "All namespaces approved", }); + } else { + core.warning("namespace-review-required is set but no pending or approved labels found"); + await github.rest.repos.createCommitStatus({ + owner, + repo, + sha: pr.head.sha, + state: "pending", + context: "Namespace Approval", + description: "Namespace review in progress", + }); } } diff --git a/.github/workflows/test/namespace-approval/status-check.test.js b/.github/workflows/test/namespace-approval/status-check.test.js index 45fb3419a686..6c74581434fa 100644 --- a/.github/workflows/test/namespace-approval/status-check.test.js +++ b/.github/workflows/test/namespace-approval/status-check.test.js @@ -94,4 +94,26 @@ describe("status-check", () => { expect.objectContaining({ state: "success", context: "Namespace Approval" }), ); }); + + it("should set pending when namespace-review-required but no pending or approved labels", async () => { + github.rest.pulls.get.mockResolvedValue({ + data: { + head: { sha: "abc123" }, + labels: [{ name: "namespace-review-required" }], + }, + }); + + await statusCheck(args()); + + expect(core.warning).toHaveBeenCalledWith( + "namespace-review-required is set but no pending or approved labels found", + ); + expect(github.rest.repos.createCommitStatus).toHaveBeenCalledWith( + expect.objectContaining({ + state: "pending", + context: "Namespace Approval", + description: "Namespace review in progress", + }), + ); + }); }); From bb3a30c033597f0f5c54a69c9da1bf0ac7616097 Mon Sep 17 00:00:00 2001 From: Sameeksha Vaity Date: Mon, 29 Jun 2026 11:43:28 -0700 Subject: [PATCH 11/23] Add Namespace Approval to summarize-checks for Next Steps to Merge --- .github/workflows/src/summarize-checks/summarize-checks.js | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/.github/workflows/src/summarize-checks/summarize-checks.js b/.github/workflows/src/summarize-checks/summarize-checks.js index a03a5535b4de..c6055032e93e 100644 --- a/.github/workflows/src/summarize-checks/summarize-checks.js +++ b/.github/workflows/src/summarize-checks/summarize-checks.js @@ -232,6 +232,12 @@ const CHECK_METADATA = [ suppressionLabels: [], troubleshootingGuide: checkAndDiagramTsg(3), }, + { + precedence: 1, + name: "Namespace Approval", + suppressionLabels: [], + troubleshootingGuide: defaultTsg, + }, { precedence: 10, name: AUTOMATED_CHECK_NAME, From 9dc65fd2561952278703e0d777d4f64a790b3019 Mon Sep 17 00:00:00 2001 From: Sameeksha Vaity Date: Tue, 30 Jun 2026 17:27:25 -0700 Subject: [PATCH 12/23] Add Java Maven artifact dual namespace, selective reset, lint and type fixes - Extract Maven artifact name from Java emitter-output-dir in tspconfig - Show both package and artifact rows in namespace review table - Update validate-approval regex to approve all rows per language (gi flag) - Implement selective per-language namespace reset on synchronize - Add parseCommentTable for comparing old vs new namespaces - Add unit tests: detect-namespaces (5), post-results (16) - Fix TypeScript type errors: use shared mocks, double-cast args() pattern - Fix eslint: type-annotate Map declarations, callback params - Fix spellcheck: replace jsquire with approver-a in tests - Update NAMESPACE-REVIEW-PROCESS.md with selective reset docs Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- .../workflows/namespace-approval-code.yaml | 2 - .../namespace-approval-validation.yaml | 2 + .../NAMESPACE-REVIEW-PROCESS.md | 30 +- .../namespace-approval/detect-namespaces.js | 77 ++-- .../src/namespace-approval/post-results.js | 188 +++++++--- .../src/namespace-approval/status-check.js | 2 +- .../namespace-approval/validate-approval.js | 16 +- .../detect-namespaces.test.js | 174 +++++++++ .../namespace-approval/post-results.test.js | 348 ++++++++++++++++++ .../namespace-approval/status-check.test.js | 6 +- 10 files changed, 720 insertions(+), 125 deletions(-) create mode 100644 .github/workflows/test/namespace-approval/detect-namespaces.test.js create mode 100644 .github/workflows/test/namespace-approval/post-results.test.js diff --git a/.github/workflows/namespace-approval-code.yaml b/.github/workflows/namespace-approval-code.yaml index 880bfd3d89c6..51f740fd7218 100644 --- a/.github/workflows/namespace-approval-code.yaml +++ b/.github/workflows/namespace-approval-code.yaml @@ -18,8 +18,6 @@ jobs: steps: - uses: actions/checkout@v6 with: - sparse-checkout: | - .github/ fetch-depth: 2 - name: Setup Node and install dependencies diff --git a/.github/workflows/namespace-approval-validation.yaml b/.github/workflows/namespace-approval-validation.yaml index 4c1323a6d7cb..d9581f4e6209 100644 --- a/.github/workflows/namespace-approval-validation.yaml +++ b/.github/workflows/namespace-approval-validation.yaml @@ -16,6 +16,8 @@ jobs: name: Validate Namespace Approval runs-on: ubuntu-24.04 steps: + # IMPORTANT: This workflow uses pull_request_target, which runs with repo write permissions. + # Only base-branch code is checked out below. Do NOT checkout or run code from the PR head. - uses: actions/checkout@v6 with: sparse-checkout: | diff --git a/.github/workflows/src/namespace-approval/NAMESPACE-REVIEW-PROCESS.md b/.github/workflows/src/namespace-approval/NAMESPACE-REVIEW-PROCESS.md index 5179c60a37d8..b07f02433217 100644 --- a/.github/workflows/src/namespace-approval/NAMESPACE-REVIEW-PROCESS.md +++ b/.github/workflows/src/namespace-approval/NAMESPACE-REVIEW-PROCESS.md @@ -15,7 +15,8 @@ When a PR modifies any `tspconfig.yaml` file, the analyze-code workflow: 1. Identifies which languages have namespace/package-name configuration 2. Extracts namespaces by parsing `tspconfig.yaml` emitter options directly 3. Determines if the service is data-plane or management-plane (via path and linter config) -4. Uploads namespace results for the status workflow +4. For management-plane PRs, validates namespace format against language-specific naming rules (defined in `.github/namespace-format-rules.yml`) +5. Uploads namespace results for the status workflow ### Status + Approval @@ -34,22 +35,27 @@ A required status check (`Namespace Approval`) blocks merge until all pending na ### Reset on Change -If a service team pushes a commit that changes namespace configuration, the approval for that language is reset: +If a service team pushes a commit that changes namespace configuration, only the **affected languages** are reset: -- The `-namespace-approved` label is removed -- The `-namespace-pending` label is re-applied +- Only languages whose namespace actually changed have their `-namespace-approved` label removed +- Languages with unchanged namespaces keep their approval +- The `-namespace-pending` label is re-applied for reset languages - The status check fails again until re-approved +- The bot comment names which specific languages were reset ## Labels -| Label | Applied by | Meaning | -| --------------------------- | ---------------- | ---------------------------------------- | -| `-namespace-pending` | Bot | Namespace detected, awaiting approval | -| `-namespace-approved` | Architect | Namespace approved for this language | -| `namespace-approved-all` | Architect (mgmt) | Shortcut: approves all pending languages | -| `Mgmt` | Bot | PR is management plane | - -Where `` is one of: `dotnet`, `java`, `python`, `typescript`, `go` +| Label | Applied by | Meaning | +| ---------------------------- | ---------------- | --------------------------------------------- | +| `namespace-review-required` | Bot | PR has namespaces that need architect approval | +| `-namespace-pending` | Bot | Namespace detected, awaiting approval | +| `-namespace-approved` | Architect | Namespace approved for this language | +| `namespace-approved-all` | Architect (mgmt) | Shortcut: approves all pending languages | +| `namespace-approved` | Bot | All languages approved | +| `Mgmt` | Bot | PR is management plane | +| `data-plane` | Bot | PR is data plane | + +Where `` is one of: `dotnet`, `java`, `python`, `typescript`, `go`, `rust` ## Authorized Approvers diff --git a/.github/workflows/src/namespace-approval/detect-namespaces.js b/.github/workflows/src/namespace-approval/detect-namespaces.js index 8231214da559..8277e6e1d21c 100644 --- a/.github/workflows/src/namespace-approval/detect-namespaces.js +++ b/.github/workflows/src/namespace-approval/detect-namespaces.js @@ -1,8 +1,7 @@ -import { access, readFile, writeFile } from "fs/promises"; +import { readFile, writeFile } from "fs/promises"; import yaml from "js-yaml"; -import { dirname, join } from "path"; +import { join } from "path"; import { getChangedFiles } from "../../../shared/src/changed-files.js"; -import { execFile } from "../../../shared/src/exec.js"; import { loadFormatRules, validateNamespaceFormat } from "./validate-format.js"; /** @@ -20,38 +19,6 @@ const EMITTER_TO_LANG = { "typespec-rust": "rust", }; -/** - * @param {string} path - * @returns {Promise} - */ -async function exists(path) { - try { - await access(path); - return true; - } catch { - return false; - } -} - -/** - * Sparse checkout only materializes .github/, so hydrate changed TypeSpec directories on demand. - * - * @param {string} file - * @param {import("@actions/core")} core - */ -async function ensureTypeSpecFilesAvailable(file, core) { - if (await exists(file)) { - return; - } - - const tspDir = dirname(file); - const workspace = process.env.GITHUB_WORKSPACE ?? process.cwd(); - core.info(`Hydrating ${tspDir} from git for namespace detection`); - await execFile("git", ["sparse-checkout", "add", tspDir], { - cwd: workspace, - }); -} - /** * Resolve the normalized language from an emitter package name. * e.g. "@azure-tools/typespec-java" → "java" @@ -74,19 +41,14 @@ function resolveLanguage(emitterKey) { * * @param {string} file - Path to tspconfig.yaml * @param {Record} namespacesFound - Map of language to namespace (mutated) + * @param {Record} artifactNames - Map of language to artifact name (mutated) * @param {import("@actions/core")} core * @returns {Promise<{ isMgmt: boolean, isDataPlane: boolean }>} */ -async function extractNamespaces(file, namespacesFound, core) { +async function extractNamespaces(file, namespacesFound, artifactNames, core) { let isMgmt = false; let isDataPlane = false; - await ensureTypeSpecFilesAvailable(file, core); - if (!(await exists(file))) { - core.info(`Skipping missing file: ${file}`); - return { isMgmt, isDataPlane }; - } - if (file.includes(".Management/") || file.includes("/resource-manager/")) { isMgmt = true; } else { @@ -133,14 +95,30 @@ async function extractNamespaces(file, namespacesFound, core) { const ns = /** @type {string | undefined} */ (emitterOpts.namespace); if (ns) { namespacesFound[lang] = ns; + } + + // For Java, also extract Maven artifact name from emitter-output-dir + if (lang === "java") { + const outputDir = /** @type {string | undefined} */ (emitterOpts["emitter-output-dir"]); + if (outputDir) { + // Last path segment is the artifact name, e.g. "{output-dir}/{service-dir}/azure-resourcemanager-contoso" + const segments = outputDir.split("/"); + const artifactName = segments[segments.length - 1]; + if (artifactName && !artifactName.startsWith("{")) { + artifactNames[lang] = artifactName; + } + } + } + + if (ns) { continue; } const packageDetails = /** @type {Record | undefined} */ ( emitterOpts["package-details"] ); - if (packageDetails?.name) { - namespacesFound[lang] = String(packageDetails.name); + if (packageDetails?.name && typeof packageDetails.name === "string") { + namespacesFound[lang] = packageDetails.name; continue; } @@ -186,11 +164,13 @@ export default async function detectNamespaces({ context, core }) { /** @type {Record} */ const namespacesFound = {}; + /** @type {Record} */ + const artifactNames = {}; let isMgmt = false; let isDataPlane = false; for (const file of changedFiles) { - const result = await extractNamespaces(file, namespacesFound, core); + const result = await extractNamespaces(file, namespacesFound, artifactNames, core); if (result.isMgmt) { isMgmt = true; } @@ -202,14 +182,19 @@ export default async function detectNamespaces({ context, core }) { const formatRules = loadFormatRules(core); /** @type {Record} */ const formatResults = {}; - if (formatRules) { + if (formatRules && isMgmt) { for (const [language, namespace] of Object.entries(namespacesFound)) { formatResults[language] = validateNamespaceFormat(language, namespace, formatRules); } + for (const [language, artifact] of Object.entries(artifactNames)) { + const key = `${language}-artifact`; + formatResults[key] = validateNamespaceFormat(language, artifact, formatRules); + } } const results = { namespacesFound, + artifactNames, isMgmt, isDataPlane, formatResults, diff --git a/.github/workflows/src/namespace-approval/post-results.js b/.github/workflows/src/namespace-approval/post-results.js index 4e113f2eece4..28a537ef721c 100644 --- a/.github/workflows/src/namespace-approval/post-results.js +++ b/.github/workflows/src/namespace-approval/post-results.js @@ -5,7 +5,7 @@ import { join } from "path"; import { z } from "zod"; import { execFile } from "../../../shared/src/exec.js"; import { PER_PAGE_MAX } from "../../../shared/src/github.js"; -import { commentOrUpdate } from "../comment.js"; +import { commentOrUpdate, parseExistingComments } from "../comment.js"; import { extractInputs } from "../context.js"; const FormatValidationResultSchema = z.object({ @@ -18,6 +18,7 @@ const FormatValidationResultSchema = z.object({ const NamespaceResultsSchema = z.object({ namespacesFound: z.record(z.string(), z.string()), + artifactNames: z.record(z.string(), z.string()).optional().default({}), isMgmt: z.boolean(), isDataPlane: z.boolean(), formatResults: z.record(z.string(), FormatValidationResultSchema), @@ -31,11 +32,6 @@ const NamespaceResultsSchema = z.object({ * @property {{ all?: string[] }} [management-plane] */ -/** @type {Record} */ -const LABEL_COLORS = { - "namespace-review-required": "e11d48", -}; - /** * @param {import("@actions/core")} core * @returns {Promise} @@ -67,11 +63,13 @@ async function downloadNamespaceResults(github, core, owner, repo, runId) { per_page: PER_PAGE_MAX, }); - const artifact = artifacts.sort((left, right) => { - const leftTime = new Date(left.updated_at || 0).getTime(); - const rightTime = new Date(right.updated_at || 0).getTime(); - return rightTime - leftTime; - })[0]; + const artifact = artifacts.sort( + (/** @type {{ updated_at?: string }} */ left, /** @type {{ updated_at?: string }} */ right) => { + const leftTime = new Date(left.updated_at || 0).getTime(); + const rightTime = new Date(right.updated_at || 0).getTime(); + return rightTime - leftTime; + }, + )[0]; if (!artifact) { core.info(`No namespace-results artifact found for run ${runId}`); @@ -99,30 +97,6 @@ async function downloadNamespaceResults(github, core, owner, repo, runId) { } } -/** - * @param {import("@actions/github-script").AsyncFunctionArguments["github"]} github - * @param {string} owner - * @param {string} repo - * @param {string} label - */ -async function ensureLabel(github, owner, repo, label) { - try { - await github.rest.issues.getLabel({ - owner, - repo, - name: label, - }); - } catch { - await github.rest.issues.createLabel({ - owner, - repo, - name: label, - // cspell:ignore fbbf - color: LABEL_COLORS[label] ?? (label.includes("pending") ? "fbbf24" : "94a3b8"), - }); - } -} - /** * @param {import("@actions/github-script").AsyncFunctionArguments["github"]} github * @param {string} owner @@ -163,22 +137,54 @@ function getApprovers(approversConfig, isMgmt, language) { return approversConfig["data-plane"]?.[language] ?? ["TBD"]; } +/** + * Parse the namespace review table from an existing bot comment. + * + * Extracts language, namespace, and approval status from each row. + * + * @param {string} body - The full comment body. + * @returns {Map} + */ +export function parseCommentTable(body) { + /** @type {Map} */ + const results = new Map(); + // Table row: | language (possibly with suffix) | `namespace` | format | status | approvers | + // Captures the base language name (word chars before optional space/underscore/paren suffix) + const rowRegex = /\| (\w+)[^|]*\| `([^`]+)` \| [^|]+ \| ([^|]+) \|/g; + let match; + while ((match = rowRegex.exec(body)) !== null) { + const lang = match[1]; + // Keep only the first match per language (primary namespace, not artifact) + if (!results.has(lang)) { + results.set(lang, { + namespace: match[2], + status: match[3].trim(), + }); + } + } + return results; +} + /** * @param {Object} params * @param {ApproversConfig} params.approversConfig * @param {Record} params.namespacesFound + * @param {Record} [params.artifactNames] - Secondary artifact names per language (e.g. Java Maven artifact). * @param {Record>} params.formatResults * @param {boolean} params.isMgmt * @param {string} params.baseRef - * @param {boolean} params.wasSynchronized + * @param {string[]} [params.resetLanguages] - Languages whose approvals were reset on this push. + * @param {Map} [params.preservedApprovals] - Approval statuses preserved from the previous comment for unchanged namespaces. */ function buildCommentBody({ approversConfig, namespacesFound, + artifactNames, formatResults, isMgmt, baseRef, - wasSynchronized, + resetLanguages, + preservedApprovals, }) { const planeType = isMgmt ? "Management Plane" : "Data Plane"; let body = `## Namespace Review Required\n\n**Plane:** ${planeType}\n\n`; @@ -188,7 +194,23 @@ function buildCommentBody({ for (const [language, namespace] of Object.entries(namespacesFound)) { const formatResult = formatResults[language]; const formatStatus = !formatResult ? "—" : formatResult.valid ? "✅" : "⚠️ Invalid"; - body += `| ${language} | \`${namespace}\` | ${formatStatus} | ⏳ Pending | ${getApprovers(approversConfig, isMgmt, language).join(", ")} |\n`; + const preserved = preservedApprovals?.get(language); + const status = preserved?.status ?? "⏳ Pending"; + const hasArtifact = artifactNames && language in artifactNames; + const label = hasArtifact ? `${language} _(package)_` : language; + body += `| ${label} | \`${namespace}\` | ${formatStatus} | ${status} | ${getApprovers(approversConfig, isMgmt, language).join(", ")} |\n`; + + // Show artifact row for languages that have one (e.g. Java Maven artifact) + if (hasArtifact) { + const artifactNs = artifactNames[language]; + const artifactFormat = formatResults[`${language}-artifact`]; + const artifactFormatStatus = !artifactFormat + ? "—" + : artifactFormat.valid + ? "✅" + : "⚠️ Invalid"; + body += `| ${language} _(artifact)_ | \`${artifactNs}\` | ${artifactFormatStatus} | ${status} | ${getApprovers(approversConfig, isMgmt, language).join(", ")} |\n`; + } } const formatErrors = Object.values(formatResults).filter((result) => !result.valid); @@ -204,8 +226,8 @@ function buildCommentBody({ body += `- Per language: apply \`-namespace-approved\` label\n`; body += `- All at once: apply \`namespace-approved-all\` label (shortcut for mgmt plane)\n\n`; body += `Merge is blocked until all languages are approved.\n`; - if (wasSynchronized) { - body += `\n> ⚠️ **Namespace changed** — approvals for affected languages have been reset.\n`; + if (resetLanguages && resetLanguages.length > 0) { + body += `\n> ⚠️ **Namespace changed** — approvals for ${resetLanguages.join(", ")} have been reset.\n`; } body += `\n_Approver list: [.github/namespace-approvers.yml](../blob/${baseRef}/.github/namespace-approvers.yml)_\n`; body += `_Process: [.github/workflows/src/namespace-approval/NAMESPACE-REVIEW-PROCESS.md](../blob/${baseRef}/.github/workflows/src/namespace-approval/NAMESPACE-REVIEW-PROCESS.md)_\n`; @@ -234,21 +256,57 @@ export default async function postResults({ github, context, core }) { pull_number: issue_number, }); /** @type {string[]} */ - const existingLabels = pr.labels.map((label) => label.name ?? ""); + const existingLabels = pr.labels.map( + (/** @type {{ name?: string }} */ label) => label.name ?? "", + ); const languages = Object.keys(results.namespacesFound); + /** @type {string[]} */ + let resetLanguages = []; + /** @type {Map} */ + let preservedApprovals = new Map(); + if (results.action === "synchronize") { + // Fetch existing bot comment to compare namespaces per language + const comments = await github.paginate(github.rest.issues.listComments, { + owner, + repo, + issue_number, + per_page: PER_PAGE_MAX, + }); + const [, existingBody] = parseExistingComments(comments, "namespace-review-bot"); + /** @type {Map} */ + const previousTable = existingBody ? parseCommentTable(existingBody) : new Map(); + for (const language of languages) { - const approvedLabel = `${language}-namespace-approved`; - if (existingLabels.includes(approvedLabel)) { - core.info(`Namespace changed for ${language}, resetting approval`); - await removeLabelIfPresent(github, owner, repo, issue_number, approvedLabel); + const prev = previousTable.get(language); + const newNs = results.namespacesFound[language]; + + if (!prev || prev.namespace !== newNs) { + // Namespace changed or new language: reset approval + const approvedLabel = `${language}-namespace-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 + core.info(`Namespace unchanged for ${language}: "${newNs}", preserving approval`); + preservedApprovals.set(language, prev); } } - for (const label of ["namespace-approved-all", "namespace-approved"]) { - if (existingLabels.includes(label)) { - await removeLabelIfPresent(github, owner, repo, issue_number, label); + // Only remove global approval labels if any language was actually reset + if (resetLanguages.length > 0) { + for (const label of ["namespace-approved-all", "namespace-approved"]) { + if (existingLabels.includes(label)) { + await removeLabelIfPresent(github, owner, repo, issue_number, label); + existingLabels.splice(existingLabels.indexOf(label), 1); + } } } } @@ -266,27 +324,41 @@ export default async function postResults({ github, context, core }) { labelsToAdd.add("data-plane"); } for (const language of languages) { - labelsToAdd.add(`${language}-namespace-pending`); + // Skip adding pending label if language is already approved + const approvedLabel = `${language}-namespace-approved`; + if (!existingLabels.includes(approvedLabel)) { + labelsToAdd.add(`${language}-namespace-pending`); + } } - for (const label of labelsToAdd) { - await ensureLabel(github, owner, repo, label); + // Don't re-add namespace-review-required if everything is already approved + const allApproved = languages.every((lang) => + existingLabels.includes(`${lang}-namespace-approved`), + ); + if (allApproved && languages.length > 0) { + labelsToAdd.delete("namespace-review-required"); } - await github.rest.issues.addLabels({ - owner, - repo, - issue_number, - labels: [...labelsToAdd], - }); + for (const label of labelsToAdd) { + if (!existingLabels.includes(label)) { + await github.rest.issues.addLabels({ + owner, + repo, + issue_number, + labels: [label], + }); + } + } const body = buildCommentBody({ approversConfig, namespacesFound: results.namespacesFound, + artifactNames: results.artifactNames, formatResults: results.formatResults, isMgmt: results.isMgmt, baseRef: pr.base.ref, - wasSynchronized: results.action === "synchronize", + resetLanguages, + preservedApprovals, }); await commentOrUpdate(github, core, owner, repo, issue_number, body, "namespace-review-bot"); diff --git a/.github/workflows/src/namespace-approval/status-check.js b/.github/workflows/src/namespace-approval/status-check.js index 7b77482f74d8..f3897ff8fb2d 100644 --- a/.github/workflows/src/namespace-approval/status-check.js +++ b/.github/workflows/src/namespace-approval/status-check.js @@ -13,7 +13,7 @@ export default async function statusCheck({ github, context, core }) { pull_number: issue_number, }); /** @type {string[]} */ - const labels = pr.labels.map((label) => label.name ?? ""); + const labels = pr.labels.map((/** @type {{ name?: string }} */ label) => label.name ?? ""); if (!labels.includes("namespace-review-required")) { core.info("No namespace review required, passing"); diff --git a/.github/workflows/src/namespace-approval/validate-approval.js b/.github/workflows/src/namespace-approval/validate-approval.js index 23b88d9444d6..3952f53b518b 100644 --- a/.github/workflows/src/namespace-approval/validate-approval.js +++ b/.github/workflows/src/namespace-approval/validate-approval.js @@ -205,8 +205,12 @@ async function handleLabeled({ pull_number: prNumber, }); /** @type {string[]} */ - const currentLabels = currentPR.labels.map((label) => label.name ?? ""); - const pendingLabels = currentLabels.filter((label) => label.endsWith("-namespace-pending")); + const currentLabels = currentPR.labels.map( + (/** @type {{ name?: string }} */ label) => label.name ?? "", + ); + const pendingLabels = currentLabels.filter((/** @type {string} */ label) => + label.endsWith("-namespace-pending"), + ); const comments = await github.rest.issues.listComments({ owner: context.repo.owner, @@ -214,14 +218,14 @@ async function handleLabeled({ issue_number: prNumber, }); const botComment = comments.data.find( - (comment) => + (/** @type {{ user?: { type?: string }, body?: string }} */ comment) => comment.user?.type === "Bot" && comment.body?.includes(""), ); if (botComment?.body) { let body = botComment.body; for (const lang of langsToApprove) { - const rowRegex = new RegExp(`(\\| ${lang} \\|[^|]+\\|) ⏳ Pending (\\|)`, "i"); + const rowRegex = new RegExp(`(\\| ${lang}[^|]*\\|[^|]+\\|[^|]+\\|) ⏳ Pending (\\|)`, "gi"); body = body.replace(rowRegex, `$1 ✅ Approved by @${actor} $2`); } await github.rest.issues.updateComment({ @@ -297,7 +301,9 @@ export default async function validateApproval({ github, context, core }) { const prNumber = payload.pull_request.number; /** @type {string[]} */ - const labels = payload.pull_request.labels.map((label) => label.name); + const labels = payload.pull_request.labels.map( + (/** @type {{ name: string }} */ label) => label.name, + ); const targetLabel = payload.label.name; const actor = payload.sender.login; const isMgmt = labels.includes("Mgmt"); diff --git a/.github/workflows/test/namespace-approval/detect-namespaces.test.js b/.github/workflows/test/namespace-approval/detect-namespaces.test.js new file mode 100644 index 000000000000..4857856573ec --- /dev/null +++ b/.github/workflows/test/namespace-approval/detect-namespaces.test.js @@ -0,0 +1,174 @@ +import { readFile } from "fs/promises"; +import yaml from "js-yaml"; +import { describe, expect, it, vi } from "vitest"; +import { createMockContext, createMockCore } from "../mocks.js"; + +// We can't easily test the default export (it depends on getChangedFiles and process.env), +// but we can test extractNamespaces and resolveLanguage by re-importing the module internals. +// Since they're not exported, we test them indirectly through tspconfig content. + +// Mock readFile to return tspconfig content +vi.mock("fs/promises", async (importOriginal) => { + /** @type {typeof import("fs/promises")} */ + const actual = /** @type {any} */ (await importOriginal()); + return { + ...actual, + readFile: vi.fn(), + writeFile: vi.fn(), + }; +}); + +// Mock getChangedFiles — path must match the import in detect-namespaces.js +vi.mock("../../../shared/src/changed-files.js", () => ({ + getChangedFiles: vi.fn().mockResolvedValue([]), +})); + +// Mock format validation +vi.mock("../../src/namespace-approval/validate-format.js", () => ({ + loadFormatRules: vi.fn().mockReturnValue(null), + validateNamespaceFormat: vi.fn(), +})); + +// Import after mocks +const { default: detectNamespaces } = + await import("../../src/namespace-approval/detect-namespaces.js"); +const { getChangedFiles } = await import("../../../shared/src/changed-files.js"); + +/** @type {import("vitest").Mock} */ +const readFileMock = /** @type {any} */ (readFile); +/** @type {import("vitest").Mock} */ +const getChangedFilesMock = /** @type {any} */ (getChangedFiles); + +/** @type {ReturnType} */ +let core; +/** @type {ReturnType} */ +let context; + +/** @returns {import("@actions/github-script").AsyncFunctionArguments} */ +function args() { + return /** @type {import("@actions/github-script").AsyncFunctionArguments} */ ( + /** @type {unknown} */ ({ context, core }) + ); +} + +function mgmtTspconfig() { + return yaml.dump({ + linter: { + extends: ["@azure-tools/typespec-azure-resource-manager/all"], + }, + options: { + "@azure-tools/typespec-csharp": { + namespace: "Azure.ResourceManager.Compute", + }, + "@azure-tools/typespec-java": { + "emitter-output-dir": "{output-dir}/{service-dir}/azure-resourcemanager-compute", + namespace: "com.azure.resourcemanager.compute", + }, + "@azure-tools/typespec-python": { + "package-details": { name: "azure-mgmt-compute" }, + }, + "@azure-tools/typespec-ts": { + namespace: "@azure/arm-compute", + }, + "@azure-tools/typespec-go": { + module: "sdk/resourcemanager/compute/armcompute", + }, + }, + }); +} + +function dataplaneTspconfig() { + return yaml.dump({ + linter: { + extends: ["@azure-tools/typespec-azure-core/all"], + }, + options: { + "@azure-tools/typespec-java": { + namespace: "com.azure.messaging.eventgrid", + }, + "@azure-tools/typespec-python": { + "package-details": { name: "azure-eventgrid" }, + }, + "@azure-tools/typespec-ts": { + namespace: "@azure/eventgrid", + }, + }, + }); +} + +function rustTspconfig() { + return yaml.dump({ + options: { + "@azure-tools/typespec-rust": { + "crate-name": "azure_storage_blobs", + }, + }, + }); +} + +describe("detect-namespaces", () => { + it("should detect management plane namespaces from path", async () => { + core = createMockCore(); + context = createMockContext(); + context.payload = { pull_request: { number: 42 }, action: "opened" }; + const file = "specification/compute/Compute.Management/tspconfig.yaml"; + getChangedFilesMock.mockResolvedValue([file]); + readFileMock.mockResolvedValue(mgmtTspconfig()); + + await detectNamespaces(args()); + + expect(core.setOutput).toHaveBeenCalledWith("results", "true"); + expect(core.setFailed).not.toHaveBeenCalled(); + }); + + it("should detect data plane from linter extends", async () => { + core = createMockCore(); + context = createMockContext(); + context.payload = { pull_request: { number: 43 }, action: "opened" }; + const file = "specification/eventgrid/EventGrid/tspconfig.yaml"; + getChangedFilesMock.mockResolvedValue([file]); + readFileMock.mockResolvedValue(dataplaneTspconfig()); + + await detectNamespaces(args()); + + expect(core.setOutput).toHaveBeenCalledWith("results", "true"); + }); + + it("should extract rust crate-name", async () => { + core = createMockCore(); + context = createMockContext(); + context.payload = { pull_request: { number: 44 }, action: "opened" }; + const file = "specification/storage/Storage/tspconfig.yaml"; + getChangedFilesMock.mockResolvedValue([file]); + readFileMock.mockResolvedValue(rustTspconfig()); + + await detectNamespaces(args()); + + expect(core.setOutput).toHaveBeenCalledWith("results", "true"); + }); + + it("should skip when no tspconfig.yaml changes detected", async () => { + core = createMockCore(); + context = createMockContext(); + context.payload = { pull_request: { number: 45 }, action: "opened" }; + getChangedFilesMock.mockResolvedValue(["specification/compute/readme.md"]); + + await detectNamespaces(args()); + + expect(core.info).toHaveBeenCalledWith("No tspconfig.yaml changes detected, skipping"); + expect(core.setOutput).not.toHaveBeenCalled(); + }); + + it("should handle tspconfig with no emitter options", async () => { + core = createMockCore(); + context = createMockContext(); + context.payload = { pull_request: { number: 46 }, action: "opened" }; + const file = "specification/compute/Compute.Management/tspconfig.yaml"; + getChangedFilesMock.mockResolvedValue([file]); + readFileMock.mockResolvedValue(yaml.dump({ linter: {} })); + + await detectNamespaces(args()); + + expect(core.info).toHaveBeenCalledWith(expect.stringContaining("No emitter options found")); + }); +}); diff --git a/.github/workflows/test/namespace-approval/post-results.test.js b/.github/workflows/test/namespace-approval/post-results.test.js new file mode 100644 index 000000000000..adf6dc5e7cc0 --- /dev/null +++ b/.github/workflows/test/namespace-approval/post-results.test.js @@ -0,0 +1,348 @@ +import { describe, expect, it, vi } from "vitest"; +import { parseCommentTable } from "../../src/namespace-approval/post-results.js"; + +// Import only the pure functions we can test without heavy mocking +// buildCommentBody and getApprovers are the key testable units + +// We need to import the module, but it has side-effect imports. +// Use dynamic import with mocks. + +vi.mock("fs/promises", () => ({ + readFile: vi.fn(), + writeFile: vi.fn(), + unlink: vi.fn(), +})); + +vi.mock("../../../../shared/src/exec.js", () => ({ + execFile: vi.fn(), +})); + +vi.mock("../../../../shared/src/github.js", () => ({ + PER_PAGE_MAX: 100, +})); + +vi.mock("../../src/comment.js", () => ({ + commentOrUpdate: vi.fn(), + parseExistingComments: vi.fn().mockReturnValue([undefined, undefined]), +})); + +vi.mock("../../src/context.js", () => ({ + extractInputs: vi.fn(), +})); + +describe("post-results", () => { + describe("parseCommentTable", () => { + it("should extract language, namespace, and pending status from table rows", () => { + const body = [ + "## Namespace Review Required", + "", + "| Language | Proposed Namespace | Format | Status | Approvers |", + "|----------|-------------------|--------|--------|----------|", + "| java | `com.azure.resourcemanager.compute` | ✅ | ⏳ Pending | JonathanGiles |", + "| dotnet | `Azure.ResourceManager.Compute` | ✅ | ⏳ Pending | approver-a |", + "", + "", + ].join("\n"); + + const result = parseCommentTable(body); + + expect(result.size).toBe(2); + expect(result.get("java")).toEqual({ + namespace: "com.azure.resourcemanager.compute", + status: "⏳ Pending", + }); + expect(result.get("dotnet")).toEqual({ + namespace: "Azure.ResourceManager.Compute", + status: "⏳ Pending", + }); + }); + + it("should extract approved status from table rows", () => { + const body = [ + "| Language | Proposed Namespace | Format | Status | Approvers |", + "|----------|-------------------|--------|--------|----------|", + "| java | `com.azure.resourcemanager.compute` | ✅ | ✅ Approved by @JonathanGiles | JonathanGiles |", + "| dotnet | `Azure.ResourceManager.Compute` | ✅ | ⏳ Pending | approver-a |", + ].join("\n"); + + const result = parseCommentTable(body); + + expect(result.get("java")?.status).toBe("✅ Approved by @JonathanGiles"); + expect(result.get("dotnet")?.status).toBe("⏳ Pending"); + }); + + it("should return empty map for body without table", () => { + const result = parseCommentTable("No table here"); + expect(result.size).toBe(0); + }); + + it("should handle format warning column values", () => { + const body = "| dotnet | `Azure.Compute` | ⚠️ Invalid | ⏳ Pending | ArthurMa1978, m-nash |"; + + const result = parseCommentTable(body); + + expect(result.get("dotnet")).toEqual({ + namespace: "Azure.Compute", + status: "⏳ Pending", + }); + }); + + it("should keep primary namespace when artifact row follows", () => { + const body = [ + "| java _(package)_ | `com.azure.resourcemanager.compute` | ✅ | ⏳ Pending | approver-b |", + "| java _(artifact)_ | `azure-resourcemanager-compute` | ✅ | ⏳ Pending | approver-b |", + ].join("\n"); + + const result = parseCommentTable(body); + + // First match (package) is kept, artifact row is skipped + expect(result.get("java")).toEqual({ + namespace: "com.azure.resourcemanager.compute", + status: "⏳ Pending", + }); + expect(result.size).toBe(1); + }); + }); + + describe("selective reset logic", () => { + it("should identify changed namespaces for reset", () => { + /** @type {Map} */ + const previousTable = new Map([ + [ + "java", + { + namespace: "com.azure.resourcemanager.compute", + status: "✅ Approved by @JonathanGiles", + }, + ], + [ + "dotnet", + { namespace: "Azure.ResourceManager.Compute", status: "✅ Approved by @approver-a" }, + ], + ]); + + const newNamespaces = { + java: "com.azure.resourcemanager.network", // changed + dotnet: "Azure.ResourceManager.Compute", // unchanged + }; + + const resetLanguages = []; + /** @type {Map} */ + const preservedApprovals = new Map(); + + for (const [language, newNs] of Object.entries(newNamespaces)) { + const prev = previousTable.get(language); + if (!prev || prev.namespace !== newNs) { + resetLanguages.push(language); + } else if (prev.status && !prev.status.includes("Pending")) { + preservedApprovals.set(language, prev); + } + } + + expect(resetLanguages).toEqual(["java"]); + expect(preservedApprovals.size).toBe(1); + expect(preservedApprovals.get("dotnet")?.status).toBe("✅ Approved by @approver-a"); + }); + + it("should reset new languages not in previous comment", () => { + /** @type {Map} */ + const previousTable = new Map([ + [ + "java", + { + namespace: "com.azure.resourcemanager.compute", + status: "✅ Approved by @JonathanGiles", + }, + ], + ]); + + const newNamespaces = { + java: "com.azure.resourcemanager.compute", + python: "azure-mgmt-compute", // new language + }; + + const resetLanguages = []; + /** @type {Map} */ + const preservedApprovals = new Map(); + + for (const [language, newNs] of Object.entries(newNamespaces)) { + const prev = previousTable.get(language); + if (!prev || prev.namespace !== newNs) { + resetLanguages.push(language); + } else if (prev.status && !prev.status.includes("Pending")) { + preservedApprovals.set(language, prev); + } + } + + expect(resetLanguages).toEqual(["python"]); + expect(preservedApprovals.get("java")?.status).toBe("✅ Approved by @JonathanGiles"); + }); + + it("should reset all when no previous comment exists", () => { + /** @type {Map} */ + const previousTable = new Map(); // empty - first run + + const newNamespaces = { + java: "com.azure.resourcemanager.compute", + dotnet: "Azure.ResourceManager.Compute", + }; + + const resetLanguages = []; + for (const [language, newNs] of Object.entries(newNamespaces)) { + const prev = previousTable.get(language); + if (!prev || prev.namespace !== newNs) { + resetLanguages.push(language); + } + } + + expect(resetLanguages).toEqual(["java", "dotnet"]); + }); + + it("should not reset any when all namespaces unchanged", () => { + /** @type {Map} */ + const previousTable = new Map([ + [ + "java", + { + namespace: "com.azure.resourcemanager.compute", + status: "✅ Approved by @JonathanGiles", + }, + ], + [ + "dotnet", + { namespace: "Azure.ResourceManager.Compute", status: "✅ Approved by @approver-a" }, + ], + ]); + + const newNamespaces = { + java: "com.azure.resourcemanager.compute", + dotnet: "Azure.ResourceManager.Compute", + }; + + const resetLanguages = []; + /** @type {Map} */ + const preservedApprovals = new Map(); + + for (const [language, newNs] of Object.entries(newNamespaces)) { + const prev = previousTable.get(language); + if (!prev || prev.namespace !== newNs) { + resetLanguages.push(language); + } else if (prev.status && !prev.status.includes("Pending")) { + preservedApprovals.set(language, prev); + } + } + + expect(resetLanguages).toEqual([]); + expect(preservedApprovals.size).toBe(2); + }); + }); + + describe("comment body generation", () => { + it("should generate a 5-column table that matches validate-approval regex", () => { + const body = [ + "## Namespace Review Required", + "", + "**Plane:** Data Plane", + "", + "| Language | Proposed Namespace | Format | Status | Approvers |", + "|----------|-------------------|--------|--------|----------|", + "| java | `com.azure.messaging.eventgrid` | — | ⏳ Pending | JonathanGiles |", + ].join("\n"); + + const rowRegex = new RegExp(`(\\| java[^|]*\\|[^|]+\\|[^|]+\\|) ⏳ Pending (\\|)`, "gi"); + expect(rowRegex.test(body)).toBe(true); + }); + + it("should approve both package and artifact rows with g flag regex", () => { + const body = [ + "| java _(package)_ | `com.azure.resourcemanager.compute` | ✅ | ⏳ Pending | approver-b |", + "| java _(artifact)_ | `azure-resourcemanager-compute` | ✅ | ⏳ Pending | approver-b |", + ].join("\n"); + + const rowRegex = new RegExp(`(\\| java[^|]*\\|[^|]+\\|[^|]+\\|) ⏳ Pending (\\|)`, "gi"); + const replaced = body.replace(rowRegex, "$1 ✅ Approved by @approver-b $2"); + + expect(replaced).not.toContain("⏳ Pending"); + expect(replaced.match(/Approved by/g)).toHaveLength(2); + }); + + it("should include specific language names in reset warning", () => { + const resetLanguages = ["java", "python"]; + const warning = `> ⚠️ **Namespace changed** — approvals for ${resetLanguages.join(", ")} have been reset.\n`; + + expect(warning).toContain("java, python"); + expect(warning).not.toContain("affected languages"); + }); + + it("should preserve approved status for unchanged languages in comment", () => { + const preservedApprovals = new Map([ + [ + "dotnet", + { namespace: "Azure.ResourceManager.Compute", status: "✅ Approved by @approver-a" }, + ], + ]); + + const preserved = preservedApprovals.get("dotnet"); + const status = preserved?.status ?? "⏳ Pending"; + + expect(status).toBe("✅ Approved by @approver-a"); + }); + }); + + describe("label skip logic", () => { + it("should not add pending when language already approved", () => { + const existingLabels = ["java-namespace-approved", "namespace-review-required"]; + const languages = ["java", "dotnet"]; + const labelsToAdd = new Set(["namespace-review-required"]); + + for (const language of languages) { + const approvedLabel = `${language}-namespace-approved`; + if (!existingLabels.includes(approvedLabel)) { + labelsToAdd.add(`${language}-namespace-pending`); + } + } + + expect(labelsToAdd.has("java-namespace-pending")).toBe(false); + expect(labelsToAdd.has("dotnet-namespace-pending")).toBe(true); + }); + + it("should remove namespace-review-required when all approved", () => { + const existingLabels = [ + "java-namespace-approved", + "dotnet-namespace-approved", + "namespace-review-required", + ]; + const languages = ["java", "dotnet"]; + const labelsToAdd = new Set(["namespace-review-required"]); + + for (const language of languages) { + const approvedLabel = `${language}-namespace-approved`; + if (!existingLabels.includes(approvedLabel)) { + labelsToAdd.add(`${language}-namespace-pending`); + } + } + + const allApproved = languages.every((lang) => + existingLabels.includes(`${lang}-namespace-approved`), + ); + if (allApproved && languages.length > 0) { + labelsToAdd.delete("namespace-review-required"); + } + + expect(labelsToAdd.has("namespace-review-required")).toBe(false); + }); + + it("should keep namespace-review-required for empty languages (vacuous every)", () => { + /** @type {string[]} */ + const languages = []; + const labelsToAdd = new Set(["namespace-review-required"]); + + const allApproved = languages.every(() => false); + if (allApproved && languages.length > 0) { + labelsToAdd.delete("namespace-review-required"); + } + + expect(labelsToAdd.has("namespace-review-required")).toBe(true); + }); + }); +}); diff --git a/.github/workflows/test/namespace-approval/status-check.test.js b/.github/workflows/test/namespace-approval/status-check.test.js index 6c74581434fa..1fa69b74ee14 100644 --- a/.github/workflows/test/namespace-approval/status-check.test.js +++ b/.github/workflows/test/namespace-approval/status-check.test.js @@ -65,7 +65,11 @@ describe("status-check", () => { await statusCheck(args()); - const infoMsg = String(core.info.mock.calls.find((c) => String(c[0]).includes("pending"))?.[0]); + const infoMsg = String( + core.info.mock.calls.find((/** @type {unknown[]} */ c) => + String(c[0]).includes("pending"), + )?.[0], + ); expect(infoMsg).toContain("java"); expect(infoMsg).toContain("dotnet"); expect(core.setFailed).not.toHaveBeenCalled(); From 849da69bd085aaa505af7c03cb55827e347018d4 Mon Sep 17 00:00:00 2001 From: Sameeksha Vaity Date: Tue, 30 Jun 2026 17:48:18 -0700 Subject: [PATCH 13/23] Fix namespace approval TS type errors --- .github/workflows/src/namespace-approval/post-results.js | 9 ++++++--- .../src/namespace-approval/validate-approval.js | 5 +++-- 2 files changed, 9 insertions(+), 5 deletions(-) diff --git a/.github/workflows/src/namespace-approval/post-results.js b/.github/workflows/src/namespace-approval/post-results.js index 28a537ef721c..dc81235f439b 100644 --- a/.github/workflows/src/namespace-approval/post-results.js +++ b/.github/workflows/src/namespace-approval/post-results.js @@ -64,9 +64,12 @@ async function downloadNamespaceResults(github, core, owner, repo, runId) { }); const artifact = artifacts.sort( - (/** @type {{ updated_at?: string }} */ left, /** @type {{ updated_at?: string }} */ right) => { - const leftTime = new Date(left.updated_at || 0).getTime(); - const rightTime = new Date(right.updated_at || 0).getTime(); + ( + /** @type {{ updated_at?: string | null }} */ left, + /** @type {{ updated_at?: string | null }} */ right, + ) => { + const leftTime = new Date(left.updated_at ?? 0).getTime(); + const rightTime = new Date(right.updated_at ?? 0).getTime(); return rightTime - leftTime; }, )[0]; diff --git a/.github/workflows/src/namespace-approval/validate-approval.js b/.github/workflows/src/namespace-approval/validate-approval.js index 3952f53b518b..e8d563a0a238 100644 --- a/.github/workflows/src/namespace-approval/validate-approval.js +++ b/.github/workflows/src/namespace-approval/validate-approval.js @@ -218,8 +218,9 @@ async function handleLabeled({ issue_number: prNumber, }); const botComment = comments.data.find( - (/** @type {{ user?: { type?: string }, body?: string }} */ comment) => - comment.user?.type === "Bot" && comment.body?.includes(""), + (/** @type {{ user?: { type?: string } | null, body?: string }} */ comment) => + comment.user?.type === "Bot" && + (comment.body?.includes("") ?? false), ); if (botComment?.body) { From 843da1629915dcb99387766d37398af7be758ecd Mon Sep 17 00:00:00 2001 From: Sameeksha Vaity Date: Thu, 2 Jul 2026 15:39:05 -0700 Subject: [PATCH 14/23] Address review feedback: error handling, shared helpers, and cleanup - Remove try/catch + core.setFailed pattern, let exceptions bubble up (detect-namespaces.js, post-results.js, validate-approval.js) - Use extractInputs() helper in validate-approval.js for consistent context extraction with logging and GitHub API hooks - Add tspconfig() filter to shared changed-files.js helper - Remove RUNNER_TEMP fallbacks (always set in CI) - Remove unnecessary JSDoc type casts on already-typed artifacts - Remove tmpdir import (no longer needed) - Throw error when no approvers configured instead of silent TBD fallback - Throw error when no namespace-results artifact found instead of null - Remove null-check anti-patterns in postResults caller - Fix prettier formatting on NAMESPACE-REVIEW-PROCESS.md - Update tests for new extractInputs usage and removed setFailed calls Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- .github/shared/src/changed-files.js | 8 ++ .../NAMESPACE-REVIEW-PROCESS.md | 18 +-- .../namespace-approval/detect-namespaces.js | 110 +++++++++--------- .../src/namespace-approval/post-results.js | 54 ++++----- .../namespace-approval/validate-approval.js | 15 +-- .../detect-namespaces.test.js | 5 +- .../validate-approval.test.js | 3 + 7 files changed, 103 insertions(+), 110 deletions(-) diff --git a/.github/shared/src/changed-files.js b/.github/shared/src/changed-files.js index f167e7056680..68347b8e30c0 100644 --- a/.github/shared/src/changed-files.js +++ b/.github/shared/src/changed-files.js @@ -281,6 +281,14 @@ export function typespec(file) { ); } +/** + * @param {string} file + * @returns {boolean} + */ +export function tspconfig(file) { + return typeof file === "string" && /specification\/.*\/tspconfig\.yaml$/.test(file); +} + /** * @param {string} [file] * @returns {boolean} diff --git a/.github/workflows/src/namespace-approval/NAMESPACE-REVIEW-PROCESS.md b/.github/workflows/src/namespace-approval/NAMESPACE-REVIEW-PROCESS.md index b07f02433217..e75641a4b37b 100644 --- a/.github/workflows/src/namespace-approval/NAMESPACE-REVIEW-PROCESS.md +++ b/.github/workflows/src/namespace-approval/NAMESPACE-REVIEW-PROCESS.md @@ -45,15 +45,15 @@ If a service team pushes a commit that changes namespace configuration, only the ## Labels -| Label | Applied by | Meaning | -| ---------------------------- | ---------------- | --------------------------------------------- | -| `namespace-review-required` | Bot | PR has namespaces that need architect approval | -| `-namespace-pending` | Bot | Namespace detected, awaiting approval | -| `-namespace-approved` | Architect | Namespace approved for this language | -| `namespace-approved-all` | Architect (mgmt) | Shortcut: approves all pending languages | -| `namespace-approved` | Bot | All languages approved | -| `Mgmt` | Bot | PR is management plane | -| `data-plane` | Bot | PR is data plane | +| Label | Applied by | Meaning | +| --------------------------- | ---------------- | ---------------------------------------------- | +| `namespace-review-required` | Bot | PR has namespaces that need architect approval | +| `-namespace-pending` | Bot | Namespace detected, awaiting approval | +| `-namespace-approved` | Architect | Namespace approved for this language | +| `namespace-approved-all` | Architect (mgmt) | Shortcut: approves all pending languages | +| `namespace-approved` | Bot | All languages approved | +| `Mgmt` | Bot | PR is management plane | +| `data-plane` | Bot | PR is data plane | Where `` is one of: `dotnet`, `java`, `python`, `typescript`, `go`, `rust` diff --git a/.github/workflows/src/namespace-approval/detect-namespaces.js b/.github/workflows/src/namespace-approval/detect-namespaces.js index 8277e6e1d21c..7e57d4e1fe2e 100644 --- a/.github/workflows/src/namespace-approval/detect-namespaces.js +++ b/.github/workflows/src/namespace-approval/detect-namespaces.js @@ -1,7 +1,7 @@ import { readFile, writeFile } from "fs/promises"; import yaml from "js-yaml"; import { join } from "path"; -import { getChangedFiles } from "../../../shared/src/changed-files.js"; +import { getChangedFiles, tspconfig } from "../../../shared/src/changed-files.js"; import { loadFormatRules, validateNamespaceFormat } from "./validate-format.js"; /** @@ -143,69 +143,65 @@ async function extractNamespaces(file, namespacesFound, artifactNames, core) { * @param {import("@actions/github-script").AsyncFunctionArguments} args */ export default async function detectNamespaces({ context, core }) { - try { - const payload = /** @type {import("@octokit/webhooks-types").PullRequestEvent} */ ( - context.payload - ); + const payload = /** @type {import("@octokit/webhooks-types").PullRequestEvent} */ ( + context.payload + ); - const changedFiles = ( - await getChangedFiles({ - cwd: process.env.GITHUB_WORKSPACE ?? process.cwd(), - paths: ["specification"], - }) - ).filter((file) => /specification\/.*\/tspconfig\.yaml$/.test(file)); + const changedFiles = ( + await getChangedFiles({ + cwd: process.env.GITHUB_WORKSPACE ?? process.cwd(), + paths: ["specification"], + }) + ).filter((file) => tspconfig(file)); - if (changedFiles.length === 0) { - core.info("No tspconfig.yaml changes detected, skipping"); - return; - } + if (changedFiles.length === 0) { + core.info("No tspconfig.yaml changes detected, skipping"); + return; + } - core.info(`Found tspconfig.yaml changes: ${changedFiles.join(", ")}`); + core.info(`Found tspconfig.yaml changes: ${changedFiles.join(", ")}`); - /** @type {Record} */ - const namespacesFound = {}; - /** @type {Record} */ - const artifactNames = {}; - let isMgmt = false; - let isDataPlane = false; + /** @type {Record} */ + const namespacesFound = {}; + /** @type {Record} */ + const artifactNames = {}; + let isMgmt = false; + let isDataPlane = false; - for (const file of changedFiles) { - const result = await extractNamespaces(file, namespacesFound, artifactNames, core); - if (result.isMgmt) { - isMgmt = true; - } - if (result.isDataPlane) { - isDataPlane = true; - } + for (const file of changedFiles) { + const result = await extractNamespaces(file, namespacesFound, artifactNames, core); + if (result.isMgmt) { + isMgmt = true; } - - const formatRules = loadFormatRules(core); - /** @type {Record} */ - const formatResults = {}; - if (formatRules && isMgmt) { - for (const [language, namespace] of Object.entries(namespacesFound)) { - formatResults[language] = validateNamespaceFormat(language, namespace, formatRules); - } - for (const [language, artifact] of Object.entries(artifactNames)) { - const key = `${language}-artifact`; - formatResults[key] = validateNamespaceFormat(language, artifact, formatRules); - } + if (result.isDataPlane) { + isDataPlane = true; } + } - const results = { - namespacesFound, - artifactNames, - isMgmt, - isDataPlane, - formatResults, - prNumber: payload.pull_request.number, - action: payload.action, - }; - const resultsPath = join(process.env.RUNNER_TEMP || ".", "namespace-results.json"); - await writeFile(resultsPath, JSON.stringify(results, null, 2)); - core.setOutput("results", "true"); - core.setOutput("results_path", resultsPath); - } catch (error) { - core.setFailed(`Namespace detection failed: ${String(error)}`); + const formatRules = loadFormatRules(core); + /** @type {Record} */ + const formatResults = {}; + if (formatRules && isMgmt) { + for (const [language, namespace] of Object.entries(namespacesFound)) { + formatResults[language] = validateNamespaceFormat(language, namespace, formatRules); + } + for (const [language, artifact] of Object.entries(artifactNames)) { + const key = `${language}-artifact`; + formatResults[key] = validateNamespaceFormat(language, artifact, formatRules); + } } + + const results = { + namespacesFound, + artifactNames, + isMgmt, + isDataPlane, + formatResults, + prNumber: payload.pull_request.number, + action: payload.action, + }; + const resultsPath = join(process.env.RUNNER_TEMP, "namespace-results.json"); + await writeFile(resultsPath, JSON.stringify(results, null, 2)); + core.setOutput("results", "true"); + core.setOutput("results_path", resultsPath); } diff --git a/.github/workflows/src/namespace-approval/post-results.js b/.github/workflows/src/namespace-approval/post-results.js index dc81235f439b..f20fac56c2bd 100644 --- a/.github/workflows/src/namespace-approval/post-results.js +++ b/.github/workflows/src/namespace-approval/post-results.js @@ -1,6 +1,5 @@ import { readFile, unlink, writeFile } from "fs/promises"; import yaml from "js-yaml"; -import { tmpdir } from "os"; import { join } from "path"; import { z } from "zod"; import { execFile } from "../../../shared/src/exec.js"; @@ -33,17 +32,11 @@ const NamespaceResultsSchema = z.object({ */ /** - * @param {import("@actions/core")} core - * @returns {Promise} + * @returns {Promise} */ -async function loadApproversConfig(core) { - try { - const content = await readFile(".github/namespace-approvers.yml", "utf8"); - return /** @type {ApproversConfig} */ (yaml.load(content)); - } catch (error) { - core.setFailed("Failed to load .github/namespace-approvers.yml: " + String(error)); - return null; - } +async function loadApproversConfig() { + const content = await readFile(".github/namespace-approvers.yml", "utf8"); + return /** @type {ApproversConfig} */ (yaml.load(content)); } /** @@ -63,20 +56,14 @@ async function downloadNamespaceResults(github, core, owner, repo, runId) { per_page: PER_PAGE_MAX, }); - const artifact = artifacts.sort( - ( - /** @type {{ updated_at?: string | null }} */ left, - /** @type {{ updated_at?: string | null }} */ right, - ) => { - const leftTime = new Date(left.updated_at ?? 0).getTime(); - const rightTime = new Date(right.updated_at ?? 0).getTime(); - return rightTime - leftTime; - }, - )[0]; + const artifact = artifacts.sort((left, right) => { + const leftTime = new Date(left.updated_at ?? 0).getTime(); + const rightTime = new Date(right.updated_at ?? 0).getTime(); + return rightTime - leftTime; + })[0]; if (!artifact) { - core.info(`No namespace-results artifact found for run ${runId}`); - return null; + throw new Error(`No namespace-results artifact found for run ${runId}`); } const download = await github.rest.actions.downloadArtifact({ @@ -86,7 +73,7 @@ async function downloadNamespaceResults(github, core, owner, repo, runId) { archive_format: "zip", }); - const zipPath = join(process.env.RUNNER_TEMP || tmpdir(), `namespace-results-${runId}.zip`); + const zipPath = join(process.env.RUNNER_TEMP, `namespace-results-${runId}.zip`); const zipBuffer = Buffer.from(new Uint8Array(/** @type {ArrayBuffer} */ (download.data))); await writeFile(zipPath, zipBuffer); @@ -137,7 +124,13 @@ function getApprovers(approversConfig, isMgmt, language) { } } - return approversConfig["data-plane"]?.[language] ?? ["TBD"]; + const approvers = approversConfig["data-plane"]?.[language]; + if (!approvers) { + throw new Error( + `No approvers configured for language "${language}" in .github/namespace-approvers.yml`, + ); + } + return approvers; } /** @@ -243,15 +236,8 @@ function buildCommentBody({ */ export default async function postResults({ github, context, core }) { const { owner, repo, issue_number, run_id } = await extractInputs(github, context, core); - const approversConfig = await loadApproversConfig(core); - if (!approversConfig) { - return; - } - - const results = run_id ? await downloadNamespaceResults(github, core, owner, repo, run_id) : null; - if (!results) { - return; - } + const approversConfig = await loadApproversConfig(); + const results = await downloadNamespaceResults(github, core, owner, repo, run_id); const { data: pr } = await github.rest.pulls.get({ owner, diff --git a/.github/workflows/src/namespace-approval/validate-approval.js b/.github/workflows/src/namespace-approval/validate-approval.js index e8d563a0a238..a5cd06d4b11c 100644 --- a/.github/workflows/src/namespace-approval/validate-approval.js +++ b/.github/workflows/src/namespace-approval/validate-approval.js @@ -1,5 +1,6 @@ import { readFile } from "fs/promises"; import yaml from "js-yaml"; +import { extractInputs } from "../context.js"; /** * @typedef {Object} ApproversConfig @@ -285,22 +286,18 @@ async function handleLabeled({ * @param {import("@actions/github-script").AsyncFunctionArguments} args */ export default async function validateApproval({ github, context, core }) { + const content = await readFile(".github/namespace-approvers.yml", "utf8"); /** @type {ApproversConfig} */ - let approversConfig; - try { - const content = await readFile(".github/namespace-approvers.yml", "utf8"); - approversConfig = /** @type {ApproversConfig} */ (yaml.load(content)); - } catch (error) { - core.setFailed("Failed to load .github/namespace-approvers.yml: " + String(error)); - return; - } + const approversConfig = /** @type {ApproversConfig} */ (yaml.load(content)); + + const { issue_number } = await extractInputs(github, context, core); const payload = /** @type {import("@octokit/webhooks-types").PullRequestLabeledEvent | import("@octokit/webhooks-types").PullRequestUnlabeledEvent} */ ( context.payload ); - const prNumber = payload.pull_request.number; + const prNumber = issue_number; /** @type {string[]} */ const labels = payload.pull_request.labels.map( (/** @type {{ name: string }} */ label) => label.name, diff --git a/.github/workflows/test/namespace-approval/detect-namespaces.test.js b/.github/workflows/test/namespace-approval/detect-namespaces.test.js index 4857856573ec..465d40c4029e 100644 --- a/.github/workflows/test/namespace-approval/detect-namespaces.test.js +++ b/.github/workflows/test/namespace-approval/detect-namespaces.test.js @@ -21,6 +21,7 @@ vi.mock("fs/promises", async (importOriginal) => { // Mock getChangedFiles — path must match the import in detect-namespaces.js vi.mock("../../../shared/src/changed-files.js", () => ({ getChangedFiles: vi.fn().mockResolvedValue([]), + tspconfig: (/** @type {string} */ file) => /specification\/.*\/tspconfig\.yaml$/.test(file), })); // Mock format validation @@ -107,6 +108,9 @@ function rustTspconfig() { } describe("detect-namespaces", () => { + // Set RUNNER_TEMP for tests since we removed the fallback + process.env.RUNNER_TEMP = process.env.RUNNER_TEMP ?? "/tmp"; + it("should detect management plane namespaces from path", async () => { core = createMockCore(); context = createMockContext(); @@ -118,7 +122,6 @@ describe("detect-namespaces", () => { await detectNamespaces(args()); expect(core.setOutput).toHaveBeenCalledWith("results", "true"); - expect(core.setFailed).not.toHaveBeenCalled(); }); it("should detect data plane from linter extends", async () => { diff --git a/.github/workflows/test/namespace-approval/validate-approval.test.js b/.github/workflows/test/namespace-approval/validate-approval.test.js index 5eb884d05d43..d82025ef96bb 100644 --- a/.github/workflows/test/namespace-approval/validate-approval.test.js +++ b/.github/workflows/test/namespace-approval/validate-approval.test.js @@ -47,8 +47,10 @@ function createPRLabeledPayload({ action, labelName, actor, labels, isMgmt = fal action, label: { name: labelName }, sender: { login: actor, type: "User" }, + repository: { owner: { login: "owner" }, name: "repo" }, pull_request: { number: 42, + head: { sha: "abc123" }, labels: allLabels, }, }; @@ -74,6 +76,7 @@ describe("validate-approval", () => { setupMocks(); github = createMockGithub(); context = createMockContext(); + context.eventName = "pull_request_target"; core = createMockCore(); /** @type {any} */ (github.rest.issues).createComment = vi.fn(); From 9f0370373fdbfd5e56d669601fe591baad71140e Mon Sep 17 00:00:00 2001 From: Sameeksha Vaity Date: Thu, 2 Jul 2026 15:47:38 -0700 Subject: [PATCH 15/23] Fix CodeQL ReDoS alert: replace regex with string methods in tspconfig filter Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- .github/shared/src/changed-files.js | 6 +++++- .../test/namespace-approval/detect-namespaces.test.js | 3 ++- 2 files changed, 7 insertions(+), 2 deletions(-) diff --git a/.github/shared/src/changed-files.js b/.github/shared/src/changed-files.js index 68347b8e30c0..b479d9a42053 100644 --- a/.github/shared/src/changed-files.js +++ b/.github/shared/src/changed-files.js @@ -286,7 +286,11 @@ export function typespec(file) { * @returns {boolean} */ export function tspconfig(file) { - return typeof file === "string" && /specification\/.*\/tspconfig\.yaml$/.test(file); + return ( + typeof file === "string" && + file.startsWith("specification/") && + file.endsWith("/tspconfig.yaml") + ); } /** diff --git a/.github/workflows/test/namespace-approval/detect-namespaces.test.js b/.github/workflows/test/namespace-approval/detect-namespaces.test.js index 465d40c4029e..9bc5f18544dd 100644 --- a/.github/workflows/test/namespace-approval/detect-namespaces.test.js +++ b/.github/workflows/test/namespace-approval/detect-namespaces.test.js @@ -21,7 +21,8 @@ vi.mock("fs/promises", async (importOriginal) => { // Mock getChangedFiles — path must match the import in detect-namespaces.js vi.mock("../../../shared/src/changed-files.js", () => ({ getChangedFiles: vi.fn().mockResolvedValue([]), - tspconfig: (/** @type {string} */ file) => /specification\/.*\/tspconfig\.yaml$/.test(file), + tspconfig: (/** @type {string} */ file) => + file.startsWith("specification/") && file.endsWith("/tspconfig.yaml"), })); // Mock format validation From 759d37771473489119d63b828a082b6aee99147b Mon Sep 17 00:00:00 2001 From: Sameeksha Vaity Date: Thu, 2 Jul 2026 16:10:16 -0700 Subject: [PATCH 16/23] Add coverage test for tspconfig filter in changed-files --- .github/shared/test/changed-files.test.js | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/.github/shared/test/changed-files.test.js b/.github/shared/test/changed-files.test.js index d9d97c954604..1a4c2d352ad5 100644 --- a/.github/shared/test/changed-files.test.js +++ b/.github/shared/test/changed-files.test.js @@ -24,6 +24,7 @@ import { scenario, stable, swagger, + tspconfig, typespec, } from "../src/changed-files.js"; import { debugLogger } from "../src/logger.js"; @@ -181,6 +182,13 @@ describe("changedFiles", () => { expect(files.filter(typespec)).toEqual(expected); }); + it("filter:tspconfig", () => { + const expected = ["specification/contosowidgetmanager/Contoso.Management/tspconfig.yaml"]; + + expect(files.filter(tspconfig)).toEqual(expected); + expect(filesResolved.filter(tspconfig)).toEqual([]); + }); + it("filter:data-plane", () => { const expected = [ "not-spec/contosowidgetmanager/data-plane/readme.md", From 2a9c182d722cfd7e3e2c3eccf3317e8771c20a08 Mon Sep 17 00:00:00 2001 From: Sameeksha Vaity Date: Thu, 2 Jul 2026 16:26:34 -0700 Subject: [PATCH 17/23] Fix namespace-approval TypeScript null and env typing errors --- .../workflows/src/namespace-approval/detect-namespaces.js | 7 ++++++- .github/workflows/src/namespace-approval/post-results.js | 8 ++++++-- 2 files changed, 12 insertions(+), 3 deletions(-) diff --git a/.github/workflows/src/namespace-approval/detect-namespaces.js b/.github/workflows/src/namespace-approval/detect-namespaces.js index 7e57d4e1fe2e..0bf5f0c7694f 100644 --- a/.github/workflows/src/namespace-approval/detect-namespaces.js +++ b/.github/workflows/src/namespace-approval/detect-namespaces.js @@ -200,7 +200,12 @@ export default async function detectNamespaces({ context, core }) { prNumber: payload.pull_request.number, action: payload.action, }; - const resultsPath = join(process.env.RUNNER_TEMP, "namespace-results.json"); + + const runnerTemp = process.env.RUNNER_TEMP; + if (!runnerTemp) { + throw new Error("RUNNER_TEMP environment variable is required"); + } + const resultsPath = join(runnerTemp, "namespace-results.json"); await writeFile(resultsPath, JSON.stringify(results, null, 2)); core.setOutput("results", "true"); core.setOutput("results_path", resultsPath); diff --git a/.github/workflows/src/namespace-approval/post-results.js b/.github/workflows/src/namespace-approval/post-results.js index f20fac56c2bd..fc551565df45 100644 --- a/.github/workflows/src/namespace-approval/post-results.js +++ b/.github/workflows/src/namespace-approval/post-results.js @@ -45,7 +45,7 @@ async function loadApproversConfig() { * @param {string} owner * @param {string} repo * @param {number} runId - * @returns {Promise | null>} + * @returns {Promise>} */ async function downloadNamespaceResults(github, core, owner, repo, runId) { const artifacts = await github.paginate(github.rest.actions.listWorkflowRunArtifacts, { @@ -73,7 +73,11 @@ async function downloadNamespaceResults(github, core, owner, repo, runId) { archive_format: "zip", }); - const zipPath = join(process.env.RUNNER_TEMP, `namespace-results-${runId}.zip`); + const runnerTemp = process.env.RUNNER_TEMP; + if (!runnerTemp) { + throw new Error("RUNNER_TEMP environment variable is required"); + } + const zipPath = join(runnerTemp, `namespace-results-${runId}.zip`); const zipBuffer = Buffer.from(new Uint8Array(/** @type {ArrayBuffer} */ (download.data))); await writeFile(zipPath, zipBuffer); From 7cead2ec49ed24418be8c5986bb43c3d89e25634 Mon Sep 17 00:00:00 2001 From: Sameeksha Vaity Date: Sun, 5 Jul 2026 17:48:42 -0700 Subject: [PATCH 18/23] Address review issues: shared approvers, pagination, async format, timing, docs - Extract ApproversConfig typedef and loadApproversConfig() into shared approvers.js (#2) - Use github.paginate() for listComments in validate-approval.js (#3) - Reorder label operations: add approved before removing pending (#4) - Add comment explaining mgmt-only format validation scoping (#5) - Change loadFormatRules() from sync readFileSync to async readFile (#7) - Use owner/repo from extractInputs() consistently, remove prNumber alias - Document security model and format validation scope in NAMESPACE-REVIEW-PROCESS.md (#1, #5) Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- .../NAMESPACE-REVIEW-PROCESS.md | 15 ++ .../src/namespace-approval/approvers.js | 21 +++ .../namespace-approval/detect-namespaces.js | 4 +- .../src/namespace-approval/post-results.js | 22 +-- .../namespace-approval/validate-approval.js | 141 ++++++++---------- .../src/namespace-approval/validate-format.js | 8 +- .../validate-approval.test.js | 2 +- 7 files changed, 114 insertions(+), 99 deletions(-) create mode 100644 .github/workflows/src/namespace-approval/approvers.js diff --git a/.github/workflows/src/namespace-approval/NAMESPACE-REVIEW-PROCESS.md b/.github/workflows/src/namespace-approval/NAMESPACE-REVIEW-PROCESS.md index e75641a4b37b..e6957fe011f8 100644 --- a/.github/workflows/src/namespace-approval/NAMESPACE-REVIEW-PROCESS.md +++ b/.github/workflows/src/namespace-approval/NAMESPACE-REVIEW-PROCESS.md @@ -95,6 +95,21 @@ No action needed beyond your normal workflow: To update approvers, submit a PR modifying `.github/namespace-approvers.yml`. Changes take effect immediately on merge. +## Format Validation + +Namespace format validation (regex-based naming rules from `.github/namespace-format-rules.yml`) only applies to **management-plane** namespaces. Data-plane namespaces follow language-specific conventions that vary too widely for centralized regex rules. + +## Security Model + +The approval workflow uses `pull_request_target` events, which grant write access to the repository. This is required because `pull_request` events from forks run with read-only permissions and cannot modify labels or commit statuses. + +Mitigations: + +- **No fork code execution:** The workflow only reads configuration files (approvers YAML) from the base branch, never from the PR branch. +- **Unauthorized label reversal:** If an unauthorized user adds an approval label, the workflow removes it and posts a warning comment. +- **Unauthorized removal guard:** If an unauthorized user removes a pending label, the workflow re-applies it. +- **Approver allowlist:** Only users listed in `.github/namespace-approvers.yml` can approve namespaces. + ## Relationship to API Review Namespace approval is **separate from API review**: diff --git a/.github/workflows/src/namespace-approval/approvers.js b/.github/workflows/src/namespace-approval/approvers.js new file mode 100644 index 000000000000..28f5eb891e95 --- /dev/null +++ b/.github/workflows/src/namespace-approval/approvers.js @@ -0,0 +1,21 @@ +import { readFile } from "fs/promises"; +import yaml from "js-yaml"; + +/** + * @typedef {Object} ApproversConfig + * @property {Record} [data-plane] + * @property {{ all?: string[] }} [management-plane] + */ + +const APPROVERS_PATH = ".github/namespace-approvers.yml"; + +/** + * Load the approvers configuration from the YAML file. + * + * @param {string} [path] + * @returns {Promise} + */ +export async function loadApproversConfig(path = APPROVERS_PATH) { + const content = await readFile(path, "utf8"); + return /** @type {ApproversConfig} */ (yaml.load(content)); +} diff --git a/.github/workflows/src/namespace-approval/detect-namespaces.js b/.github/workflows/src/namespace-approval/detect-namespaces.js index 0bf5f0c7694f..1ccefb834862 100644 --- a/.github/workflows/src/namespace-approval/detect-namespaces.js +++ b/.github/workflows/src/namespace-approval/detect-namespaces.js @@ -178,9 +178,11 @@ export default async function detectNamespaces({ context, core }) { } } - const formatRules = loadFormatRules(core); + const formatRules = await loadFormatRules(core); /** @type {Record} */ const formatResults = {}; + // Format validation only applies to management-plane namespaces. Data-plane namespaces + // follow language-specific conventions that vary too widely for regex-based rules. if (formatRules && isMgmt) { for (const [language, namespace] of Object.entries(namespacesFound)) { formatResults[language] = validateNamespaceFormat(language, namespace, formatRules); diff --git a/.github/workflows/src/namespace-approval/post-results.js b/.github/workflows/src/namespace-approval/post-results.js index fc551565df45..dd6e530cac89 100644 --- a/.github/workflows/src/namespace-approval/post-results.js +++ b/.github/workflows/src/namespace-approval/post-results.js @@ -1,11 +1,11 @@ -import { readFile, unlink, writeFile } from "fs/promises"; -import yaml from "js-yaml"; +import { unlink, writeFile } from "fs/promises"; import { join } from "path"; import { z } from "zod"; import { execFile } from "../../../shared/src/exec.js"; import { PER_PAGE_MAX } from "../../../shared/src/github.js"; import { commentOrUpdate, parseExistingComments } from "../comment.js"; import { extractInputs } from "../context.js"; +import { loadApproversConfig } from "./approvers.js"; const FormatValidationResultSchema = z.object({ valid: z.boolean(), @@ -25,20 +25,6 @@ const NamespaceResultsSchema = z.object({ action: z.string().optional(), }); -/** - * @typedef {Object} ApproversConfig - * @property {Record} [data-plane] - * @property {{ all?: string[] }} [management-plane] - */ - -/** - * @returns {Promise} - */ -async function loadApproversConfig() { - const content = await readFile(".github/namespace-approvers.yml", "utf8"); - return /** @type {ApproversConfig} */ (yaml.load(content)); -} - /** * @param {import("@actions/github-script").AsyncFunctionArguments["github"]} github * @param {import("@actions/core")} core @@ -115,7 +101,7 @@ async function removeLabelIfPresent(github, owner, repo, issueNumber, label) { } /** - * @param {ApproversConfig} approversConfig + * @param {import("./approvers.js").ApproversConfig} approversConfig * @param {boolean} isMgmt * @param {string} language * @returns {string[]} @@ -167,7 +153,7 @@ export function parseCommentTable(body) { /** * @param {Object} params - * @param {ApproversConfig} params.approversConfig + * @param {import("./approvers.js").ApproversConfig} params.approversConfig * @param {Record} params.namespacesFound * @param {Record} [params.artifactNames] - Secondary artifact names per language (e.g. Java Maven artifact). * @param {Record>} params.formatResults diff --git a/.github/workflows/src/namespace-approval/validate-approval.js b/.github/workflows/src/namespace-approval/validate-approval.js index a5cd06d4b11c..82233b3ebb98 100644 --- a/.github/workflows/src/namespace-approval/validate-approval.js +++ b/.github/workflows/src/namespace-approval/validate-approval.js @@ -1,19 +1,14 @@ -import { readFile } from "fs/promises"; -import yaml from "js-yaml"; import { extractInputs } from "../context.js"; - -/** - * @typedef {Object} ApproversConfig - * @property {Record} [data-plane] - * @property {{ all?: string[] }} [management-plane] - */ +import { loadApproversConfig } from "./approvers.js"; /** * @typedef {Object} ValidateContext * @property {import("@actions/github-script").AsyncFunctionArguments["github"]} github * @property {import("@actions/github-script").AsyncFunctionArguments["context"]} context * @property {import("@actions/github-script").AsyncFunctionArguments["core"]} core - * @property {ApproversConfig} approversConfig + * @property {import("./approvers.js").ApproversConfig} approversConfig + * @property {string} owner + * @property {string} repo * @property {string} targetLabel * @property {string} actor * @property {number} prNumber @@ -26,14 +21,14 @@ const ALLOWED_BOT_LOGINS = ["github-actions[bot]", "azure-sdk"]; /** * Get all authorized approvers as a flat list. * - * @param {ApproversConfig} approversConfig + * @param {import("./approvers.js").ApproversConfig} approversConfig * @returns {string[]} */ function getAllApprovers(approversConfig) { const mgmtApprovers = approversConfig["management-plane"]?.all ?? []; - return [ - ...new Set([...mgmtApprovers, ...Object.values(approversConfig["data-plane"] ?? {}).flat()]), - ]; + /** @type {string[][]} */ + const dpValues = Object.values(approversConfig["data-plane"] ?? {}); + return [...new Set([...mgmtApprovers, ...dpValues.flat()])]; } /** @@ -43,9 +38,10 @@ function getAllApprovers(approversConfig) { */ async function handleUnlabeled({ github, - context, core, approversConfig, + owner, + repo, targetLabel, actor, prNumber, @@ -70,15 +66,15 @@ async function handleUnlabeled({ core.warning(`${actor} is not authorized to remove ${targetLabel}, re-applying`); await github.rest.issues.addLabels({ - owner: context.repo.owner, - repo: context.repo.repo, + owner, + repo, issue_number: prNumber, labels: [targetLabel], }); await github.rest.issues.createComment({ - owner: context.repo.owner, - repo: context.repo.repo, + owner, + repo, issue_number: prNumber, body: `⚠️ @${actor} is not authorized to remove \`${targetLabel}\`. Only authorized namespace approvers can remove namespace labels.\n\nLabel has been re-applied.`, }); @@ -91,9 +87,10 @@ async function handleUnlabeled({ */ async function handleLabeled({ github, - context, core, approversConfig, + owner, + repo, targetLabel, actor, prNumber, @@ -109,14 +106,14 @@ async function handleLabeled({ if (!allApprovers.includes(actor)) { await github.rest.issues.removeLabel({ - owner: context.repo.owner, - repo: context.repo.repo, + owner, + repo, issue_number: prNumber, name: targetLabel, }); await github.rest.issues.createComment({ - owner: context.repo.owner, - repo: context.repo.repo, + owner, + repo, issue_number: prNumber, body: `⚠️ @${actor} is not authorized to use \`namespace-approved-all\`. This shortcut is available to authorized namespace approvers.\n\nLabel removed.`, }); @@ -144,14 +141,14 @@ async function handleLabeled({ if (!authorizedList.includes(actor)) { await github.rest.issues.removeLabel({ - owner: context.repo.owner, - repo: context.repo.repo, + owner, + repo, issue_number: prNumber, name: targetLabel, }); await github.rest.issues.createComment({ - owner: context.repo.owner, - repo: context.repo.repo, + owner, + repo, issue_number: prNumber, body: `⚠️ @${actor} is not authorized to approve **${lang}** namespace. Authorized approvers: ${authorizedList.join(", ")}.\n\nLabel \`${targetLabel}\` has been removed.`, }); @@ -164,45 +161,42 @@ async function handleLabeled({ core.info(`Approving languages: ${langsToApprove.join(", ")} by ${actor}`); for (const lang of langsToApprove) { - try { - await github.rest.issues.removeLabel({ - owner: context.repo.owner, - repo: context.repo.repo, - issue_number: prNumber, - name: `${lang}-namespace-pending`, - }); - } catch { - // label may not exist - } - + // Add approved label before removing pending to avoid status check timing gap (#4) if (targetLabel === "namespace-approved-all") { const approvedLabel = `${lang}-namespace-approved`; try { - await github.rest.issues.getLabel({ - owner: context.repo.owner, - repo: context.repo.repo, - name: approvedLabel, - }); + await github.rest.issues.getLabel({ owner, repo, name: approvedLabel }); } catch { await github.rest.issues.createLabel({ - owner: context.repo.owner, - repo: context.repo.repo, + owner, + repo, name: approvedLabel, color: "22c55e", }); } await github.rest.issues.addLabels({ - owner: context.repo.owner, - repo: context.repo.repo, + owner, + repo, issue_number: prNumber, labels: [approvedLabel], }); } + + try { + await github.rest.issues.removeLabel({ + owner, + repo, + issue_number: prNumber, + name: `${lang}-namespace-pending`, + }); + } catch { + // label may not exist + } } const { data: currentPR } = await github.rest.pulls.get({ - owner: context.repo.owner, - repo: context.repo.repo, + owner, + repo, pull_number: prNumber, }); /** @type {string[]} */ @@ -213,12 +207,12 @@ async function handleLabeled({ label.endsWith("-namespace-pending"), ); - const comments = await github.rest.issues.listComments({ - owner: context.repo.owner, - repo: context.repo.repo, + const comments = await github.paginate(github.rest.issues.listComments, { + owner, + repo, issue_number: prNumber, }); - const botComment = comments.data.find( + const botComment = comments.find( (/** @type {{ user?: { type?: string } | null, body?: string }} */ comment) => comment.user?.type === "Bot" && (comment.body?.includes("") ?? false), @@ -231,8 +225,8 @@ async function handleLabeled({ body = body.replace(rowRegex, `$1 ✅ Approved by @${actor} $2`); } await github.rest.issues.updateComment({ - owner: context.repo.owner, - repo: context.repo.repo, + owner, + repo, comment_id: botComment.id, body, }); @@ -242,8 +236,8 @@ async function handleLabeled({ core.info("All namespaces approved!"); try { await github.rest.issues.removeLabel({ - owner: context.repo.owner, - repo: context.repo.repo, + owner, + repo, issue_number: prNumber, name: "namespace-review-required", }); @@ -251,28 +245,24 @@ async function handleLabeled({ // label may not exist } try { - await github.rest.issues.getLabel({ - owner: context.repo.owner, - repo: context.repo.repo, - name: "namespace-approved", - }); + await github.rest.issues.getLabel({ owner, repo, name: "namespace-approved" }); } catch { await github.rest.issues.createLabel({ - owner: context.repo.owner, - repo: context.repo.repo, + owner, + repo, name: "namespace-approved", color: "22c55e", }); } await github.rest.issues.addLabels({ - owner: context.repo.owner, - repo: context.repo.repo, + owner, + repo, issue_number: prNumber, labels: ["namespace-approved"], }); await github.rest.issues.createComment({ - owner: context.repo.owner, - repo: context.repo.repo, + owner, + repo, issue_number: prNumber, body: "## ✅ Namespace Approved\n\nAll required namespace approvals received. This PR is clear to merge from a namespace perspective.", }); @@ -286,18 +276,15 @@ async function handleLabeled({ * @param {import("@actions/github-script").AsyncFunctionArguments} args */ export default async function validateApproval({ github, context, core }) { - const content = await readFile(".github/namespace-approvers.yml", "utf8"); - /** @type {ApproversConfig} */ - const approversConfig = /** @type {ApproversConfig} */ (yaml.load(content)); + const approversConfig = await loadApproversConfig(); - const { issue_number } = await extractInputs(github, context, core); + const { owner, repo, issue_number } = await extractInputs(github, context, core); const payload = /** @type {import("@octokit/webhooks-types").PullRequestLabeledEvent | import("@octokit/webhooks-types").PullRequestUnlabeledEvent} */ ( context.payload ); - const prNumber = issue_number; /** @type {string[]} */ const labels = payload.pull_request.labels.map( (/** @type {{ name: string }} */ label) => label.name, @@ -312,9 +299,11 @@ export default async function validateApproval({ github, context, core }) { context, core, approversConfig, + owner, + repo, targetLabel, actor, - prNumber, + prNumber: issue_number, isMgmt, }); } @@ -329,9 +318,11 @@ export default async function validateApproval({ github, context, core }) { context, core, approversConfig, + owner, + repo, targetLabel, actor, - prNumber, + prNumber: issue_number, isMgmt, labels, }); diff --git a/.github/workflows/src/namespace-approval/validate-format.js b/.github/workflows/src/namespace-approval/validate-format.js index 0d9e199a0454..dd7e1a464701 100644 --- a/.github/workflows/src/namespace-approval/validate-format.js +++ b/.github/workflows/src/namespace-approval/validate-format.js @@ -1,4 +1,4 @@ -import { readFileSync } from "fs"; +import { readFile } from "fs/promises"; import yaml from "js-yaml"; /** @@ -18,11 +18,11 @@ const DEFAULT_RULES_PATH = ".github/namespace-format-rules.yml"; * * @param {import("@actions/core")} core * @param {string} [rulesPath] - * @returns {FormatRulesConfig | null} + * @returns {Promise} */ -export function loadFormatRules(core, rulesPath = DEFAULT_RULES_PATH) { +export async function loadFormatRules(core, rulesPath = DEFAULT_RULES_PATH) { try { - const content = readFileSync(rulesPath, "utf8"); + const content = await readFile(rulesPath, "utf8"); return /** @type {FormatRulesConfig} */ (yaml.load(content)); } catch (e) { core.warning( diff --git a/.github/workflows/test/namespace-approval/validate-approval.test.js b/.github/workflows/test/namespace-approval/validate-approval.test.js index d82025ef96bb..3476b2d836a1 100644 --- a/.github/workflows/test/namespace-approval/validate-approval.test.js +++ b/.github/workflows/test/namespace-approval/validate-approval.test.js @@ -14,7 +14,7 @@ import { readFile } from "fs/promises"; import yaml from "js-yaml"; import validateApproval from "../../src/namespace-approval/validate-approval.js"; -/** @type {import("../../src/namespace-approval/validate-approval.js").ApproversConfig} */ +/** @type {import("../../src/namespace-approval/approvers.js").ApproversConfig} */ const approversConfig = { "data-plane": { dotnet: ["jsquire", "m-nash"], From 93c8ba45360669eceb28844611d34987bcd226ff Mon Sep 17 00:00:00 2001 From: Sameeksha Vaity Date: Sun, 5 Jul 2026 19:05:25 -0700 Subject: [PATCH 19/23] Prevent duplicate 'Namespace Approved' comments on concurrent label events When multiple per-language labels are applied in quick succession, each triggers a separate labeled event. Check if the approval comment already exists before posting to avoid duplicates. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- .../namespace-approval/validate-approval.js | 22 ++++++++++++++----- 1 file changed, 16 insertions(+), 6 deletions(-) diff --git a/.github/workflows/src/namespace-approval/validate-approval.js b/.github/workflows/src/namespace-approval/validate-approval.js index 82233b3ebb98..9ca4d04295f0 100644 --- a/.github/workflows/src/namespace-approval/validate-approval.js +++ b/.github/workflows/src/namespace-approval/validate-approval.js @@ -260,12 +260,22 @@ async function handleLabeled({ issue_number: prNumber, labels: ["namespace-approved"], }); - await github.rest.issues.createComment({ - owner, - repo, - issue_number: prNumber, - body: "## ✅ Namespace Approved\n\nAll required namespace approvals received. This PR is clear to merge from a namespace perspective.", - }); + + // Only post the approval comment if one doesn't already exist (avoids duplicates + // when multiple per-language labels are applied in quick succession) + const approvalCommentExists = comments.some( + (/** @type {{ user?: { type?: string } | null, body?: string }} */ comment) => + comment.user?.type === "Bot" && + (comment.body?.includes("## ✅ Namespace Approved") ?? false), + ); + if (!approvalCommentExists) { + await github.rest.issues.createComment({ + owner, + repo, + issue_number: prNumber, + body: "## ✅ Namespace Approved\n\nAll required namespace approvals received. This PR is clear to merge from a namespace perspective.", + }); + } } } From 54ea1e9a4e78f83dae7efe0e95c9e1ec91581d59 Mon Sep 17 00:00:00 2001 From: Sameeksha Vaity Date: Sun, 5 Jul 2026 19:16:26 -0700 Subject: [PATCH 20/23] Rename labels to namespace--*, add @handles, integrate with NSTM Label rename: -namespace-approved/pending to namespace--approved/pending to align with team consensus (namespace--approved format). GitHub @handles: Approver column in bot comment now uses @username format to tag language architects directly on the main repo. NSTM integration: Added RequiredLabelRule in labelling.js so namespace-review-required triggers a blocker in the Next Steps to Merge comment, pointing users to the detailed namespace review table. Updated all source files, tests, and NAMESPACE-REVIEW-PROCESS.md documentation. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- .../NAMESPACE-REVIEW-PROCESS.md | 22 ++++++----- .../src/namespace-approval/post-results.js | 26 +++++++++---- .../src/namespace-approval/status-check.js | 8 +++- .../namespace-approval/validate-approval.js | 18 +++++---- .../src/summarize-checks/labelling.js | 20 ++++++++++ .../namespace-approval/post-results.test.js | 20 +++++----- .../namespace-approval/status-check.test.js | 6 +-- .../validate-approval.test.js | 38 +++++++++---------- 8 files changed, 100 insertions(+), 58 deletions(-) diff --git a/.github/workflows/src/namespace-approval/NAMESPACE-REVIEW-PROCESS.md b/.github/workflows/src/namespace-approval/NAMESPACE-REVIEW-PROCESS.md index e6957fe011f8..9baf821d8d8a 100644 --- a/.github/workflows/src/namespace-approval/NAMESPACE-REVIEW-PROCESS.md +++ b/.github/workflows/src/namespace-approval/NAMESPACE-REVIEW-PROCESS.md @@ -20,16 +20,16 @@ When a PR modifies any `tspconfig.yaml` file, the analyze-code workflow: ### Status + Approval -The status workflow posts the namespace review comment, applies `-namespace-pending` labels, and validates architect approval labels. +The status workflow posts the namespace review comment, applies `namespace--pending` labels, and validates architect approval labels. -- **Data plane:** Each language architect approves their language by applying `-namespace-approved` +- **Data plane:** Each language architect approves their language by applying `namespace--approved` - **Management plane:** ArthurMa1978 or m-nash can apply `namespace-approved-all` to approve all languages at once ### Status Check A required status check (`Namespace Approval`) blocks merge until all pending namespaces are approved. The check: -- ✅ Passes when all `*-namespace-pending` labels have corresponding `*-namespace-approved` labels +- ✅ Passes when all `namespace-*-pending` labels have corresponding `namespace-*-approved` labels - ❌ Fails when any pending namespace lacks approval - ⏭️ Skipped when the PR doesn't modify tspconfig.yaml (no namespace changes) @@ -37,19 +37,23 @@ A required status check (`Namespace Approval`) blocks merge until all pending na If a service team pushes a commit that changes namespace configuration, only the **affected languages** are reset: -- Only languages whose namespace actually changed have their `-namespace-approved` label removed +- Only languages whose namespace actually changed have their `namespace--approved` label removed - Languages with unchanged namespaces keep their approval -- The `-namespace-pending` label is re-applied for reset languages +- The `namespace--pending` label is re-applied for reset languages - The status check fails again until re-approved - The bot comment names which specific languages were reset +### Next Steps to Merge Integration + +Namespace approval status also appears in the **Next Steps to Merge** comment. When `namespace-review-required` is present but `namespace-approved` is not, the NSTM comment shows a blocker with a link to the detailed namespace review table. + ## Labels | Label | Applied by | Meaning | | --------------------------- | ---------------- | ---------------------------------------------- | | `namespace-review-required` | Bot | PR has namespaces that need architect approval | -| `-namespace-pending` | Bot | Namespace detected, awaiting approval | -| `-namespace-approved` | Architect | Namespace approved for this language | +| `namespace--pending` | Bot | Namespace detected, awaiting approval | +| `namespace--approved` | Architect | Namespace approved for this language | | `namespace-approved-all` | Architect (mgmt) | Shortcut: approves all pending languages | | `namespace-approved` | Bot | All languages approved | | `Mgmt` | Bot | PR is management plane | @@ -86,9 +90,9 @@ No action needed beyond your normal workflow: ## For Architects -1. Watch for PRs with `-namespace-pending` labels +1. Watch for PRs with `namespace--pending` labels 2. Review the proposed namespace in the PR diff -3. If acceptable, apply `-namespace-approved` label +3. If acceptable, apply `namespace--approved` label 4. For management plane PRs, you can use `namespace-approved-all` to approve all at once ## Configuration diff --git a/.github/workflows/src/namespace-approval/post-results.js b/.github/workflows/src/namespace-approval/post-results.js index dd6e530cac89..caec41cd04bb 100644 --- a/.github/workflows/src/namespace-approval/post-results.js +++ b/.github/workflows/src/namespace-approval/post-results.js @@ -184,7 +184,13 @@ function buildCommentBody({ const status = preserved?.status ?? "⏳ Pending"; const hasArtifact = artifactNames && language in artifactNames; const label = hasArtifact ? `${language} _(package)_` : language; - body += `| ${label} | \`${namespace}\` | ${formatStatus} | ${status} | ${getApprovers(approversConfig, isMgmt, language).join(", ")} |\n`; + body += `| ${label} | \`${namespace}\` | ${formatStatus} | ${status} | ${getApprovers( + approversConfig, + isMgmt, + language, + ) + .map((a) => `@${a}`) + .join(", ")} |\n`; // Show artifact row for languages that have one (e.g. Java Maven artifact) if (hasArtifact) { @@ -195,7 +201,13 @@ function buildCommentBody({ : artifactFormat.valid ? "✅" : "⚠️ Invalid"; - body += `| ${language} _(artifact)_ | \`${artifactNs}\` | ${artifactFormatStatus} | ${status} | ${getApprovers(approversConfig, isMgmt, language).join(", ")} |\n`; + body += `| ${language} _(artifact)_ | \`${artifactNs}\` | ${artifactFormatStatus} | ${status} | ${getApprovers( + approversConfig, + isMgmt, + language, + ) + .map((a) => `@${a}`) + .join(", ")} |\n`; } } @@ -209,7 +221,7 @@ function buildCommentBody({ } body += `\n**How to approve:**\n`; - body += `- Per language: apply \`-namespace-approved\` label\n`; + body += `- Per language: apply \`namespace--approved\` label\n`; body += `- All at once: apply \`namespace-approved-all\` label (shortcut for mgmt plane)\n\n`; body += `Merge is blocked until all languages are approved.\n`; if (resetLanguages && resetLanguages.length > 0) { @@ -263,7 +275,7 @@ export default async function postResults({ github, context, core }) { if (!prev || prev.namespace !== newNs) { // Namespace changed or new language: reset approval - const approvedLabel = `${language}-namespace-approved`; + const approvedLabel = `namespace-${language}-approved`; if (existingLabels.includes(approvedLabel)) { core.info( `Namespace changed for ${language}: "${prev?.namespace ?? "(new)"}" → "${newNs}", resetting approval`, @@ -304,15 +316,15 @@ export default async function postResults({ github, context, core }) { } for (const language of languages) { // Skip adding pending label if language is already approved - const approvedLabel = `${language}-namespace-approved`; + const approvedLabel = `namespace-${language}-approved`; if (!existingLabels.includes(approvedLabel)) { - labelsToAdd.add(`${language}-namespace-pending`); + labelsToAdd.add(`namespace-${language}-pending`); } } // Don't re-add namespace-review-required if everything is already approved const allApproved = languages.every((lang) => - existingLabels.includes(`${lang}-namespace-approved`), + existingLabels.includes(`namespace-${lang}-approved`), ); if (allApproved && languages.length > 0) { labelsToAdd.delete("namespace-review-required"); diff --git a/.github/workflows/src/namespace-approval/status-check.js b/.github/workflows/src/namespace-approval/status-check.js index f3897ff8fb2d..55e43a7feb1f 100644 --- a/.github/workflows/src/namespace-approval/status-check.js +++ b/.github/workflows/src/namespace-approval/status-check.js @@ -28,9 +28,13 @@ export default async function statusCheck({ github, context, core }) { return; } - const pendingLabels = labels.filter((label) => label.endsWith("-namespace-pending")); + const pendingLabels = labels.filter( + (label) => label.startsWith("namespace-") && label.endsWith("-pending"), + ); if (pendingLabels.length > 0) { - const pending = pendingLabels.map((l) => l.replace("-namespace-pending", "")).join(", "); + const pending = pendingLabels + .map((l) => l.replace("namespace-", "").replace("-pending", "")) + .join(", "); await github.rest.repos.createCommitStatus({ owner, repo, diff --git a/.github/workflows/src/namespace-approval/validate-approval.js b/.github/workflows/src/namespace-approval/validate-approval.js index 9ca4d04295f0..ee0984f2931c 100644 --- a/.github/workflows/src/namespace-approval/validate-approval.js +++ b/.github/workflows/src/namespace-approval/validate-approval.js @@ -46,7 +46,9 @@ async function handleUnlabeled({ actor, prNumber, }) { - if (!targetLabel.endsWith("-namespace-pending") && targetLabel !== "namespace-review-required") { + const isNamespacePending = + targetLabel.startsWith("namespace-") && targetLabel.endsWith("-pending"); + if (!isNamespacePending && targetLabel !== "namespace-review-required") { core.info(`${targetLabel} is not a namespace label, skipping`); return; } @@ -121,10 +123,10 @@ async function handleLabeled({ } langsToApprove = labels - .filter((label) => label.endsWith("-namespace-pending")) - .map((label) => label.replace("-namespace-pending", "")); + .filter((label) => label.startsWith("namespace-") && label.endsWith("-pending")) + .map((label) => label.replace("namespace-", "").replace("-pending", "")); } else { - const match = targetLabel.match(/^(\w+)-namespace-approved$/); + const match = targetLabel.match(/^namespace-(\w+)-approved$/); if (!match) { core.info(`${targetLabel} is not a namespace approval label`); return; @@ -163,7 +165,7 @@ async function handleLabeled({ for (const lang of langsToApprove) { // Add approved label before removing pending to avoid status check timing gap (#4) if (targetLabel === "namespace-approved-all") { - const approvedLabel = `${lang}-namespace-approved`; + const approvedLabel = `namespace-${lang}-approved`; try { await github.rest.issues.getLabel({ owner, repo, name: approvedLabel }); } catch { @@ -187,7 +189,7 @@ async function handleLabeled({ owner, repo, issue_number: prNumber, - name: `${lang}-namespace-pending`, + name: `namespace-${lang}-pending`, }); } catch { // label may not exist @@ -203,8 +205,8 @@ async function handleLabeled({ const currentLabels = currentPR.labels.map( (/** @type {{ name?: string }} */ label) => label.name ?? "", ); - const pendingLabels = currentLabels.filter((/** @type {string} */ label) => - label.endsWith("-namespace-pending"), + const pendingLabels = currentLabels.filter( + (/** @type {string} */ label) => label.startsWith("namespace-") && label.endsWith("-pending"), ); const comments = await github.paginate(github.rest.issues.listComments, { diff --git a/.github/workflows/src/summarize-checks/labelling.js b/.github/workflows/src/summarize-checks/labelling.js index 192ff0a50c1c..7293ba21ec06 100644 --- a/.github/workflows/src/summarize-checks/labelling.js +++ b/.github/workflows/src/summarize-checks/labelling.js @@ -1157,6 +1157,25 @@ const rulesPri3Blockers = [ }, ]; +/** @type {RequiredLabelRule[]} */ +const rulesPri1Namespace = [ + // When namespace-review-required is present, require namespace-approved before merge. + // See .github/workflows/src/namespace-approval/NAMESPACE-REVIEW-PROCESS.md for details. + { + precedence: 1, + anyPrerequisiteLabels: ["namespace-review-required"], + allPrerequisiteAbsentLabels: ["namespace-approved"], + anyRequiredLabels: ["namespace-approved"], + troubleshootingGuide: + "This PR modifies SDK namespace configuration in tspconfig.yaml and requires " + + "namespace approval from language architects before it can be merged.
" + + "Check the Namespace Review Required comment on this PR for a detailed status table " + + "showing which languages are pending and who can approve.
" + + "Architects: apply namespace-<language>-approved labels for each language, " + + "or namespace-approved-all to approve all at once.", + }, +]; + export const requiredLabelsRules = rulesPri0dataPlane .concat(rulesPri0NotReadyForArmReview) .concat(rulesPri0ArmRpaas) @@ -1164,6 +1183,7 @@ export const requiredLabelsRules = rulesPri0dataPlane .concat(rulesPri0ArmRev) .concat(rulesPri1ArmRev) .concat(rulesPri1Suppressions) + .concat(rulesPri1Namespace) .concat(rulesPri2Sdk) .concat(rulesPri2LegacySdk) .concat(rulesPri3Blockers); diff --git a/.github/workflows/test/namespace-approval/post-results.test.js b/.github/workflows/test/namespace-approval/post-results.test.js index adf6dc5e7cc0..952742522a42 100644 --- a/.github/workflows/test/namespace-approval/post-results.test.js +++ b/.github/workflows/test/namespace-approval/post-results.test.js @@ -291,39 +291,39 @@ describe("post-results", () => { describe("label skip logic", () => { it("should not add pending when language already approved", () => { - const existingLabels = ["java-namespace-approved", "namespace-review-required"]; + const existingLabels = ["namespace-java-approved", "namespace-review-required"]; const languages = ["java", "dotnet"]; const labelsToAdd = new Set(["namespace-review-required"]); for (const language of languages) { - const approvedLabel = `${language}-namespace-approved`; + const approvedLabel = `namespace-${language}-approved`; if (!existingLabels.includes(approvedLabel)) { - labelsToAdd.add(`${language}-namespace-pending`); + labelsToAdd.add(`namespace-${language}-pending`); } } - expect(labelsToAdd.has("java-namespace-pending")).toBe(false); - expect(labelsToAdd.has("dotnet-namespace-pending")).toBe(true); + expect(labelsToAdd.has("namespace-java-pending")).toBe(false); + expect(labelsToAdd.has("namespace-dotnet-pending")).toBe(true); }); it("should remove namespace-review-required when all approved", () => { const existingLabels = [ - "java-namespace-approved", - "dotnet-namespace-approved", + "namespace-java-approved", + "namespace-dotnet-approved", "namespace-review-required", ]; const languages = ["java", "dotnet"]; const labelsToAdd = new Set(["namespace-review-required"]); for (const language of languages) { - const approvedLabel = `${language}-namespace-approved`; + const approvedLabel = `namespace-${language}-approved`; if (!existingLabels.includes(approvedLabel)) { - labelsToAdd.add(`${language}-namespace-pending`); + labelsToAdd.add(`namespace-${language}-pending`); } } const allApproved = languages.every((lang) => - existingLabels.includes(`${lang}-namespace-approved`), + existingLabels.includes(`namespace-${lang}-approved`), ); if (allApproved && languages.length > 0) { labelsToAdd.delete("namespace-review-required"); diff --git a/.github/workflows/test/namespace-approval/status-check.test.js b/.github/workflows/test/namespace-approval/status-check.test.js index 1fa69b74ee14..1a1839558d35 100644 --- a/.github/workflows/test/namespace-approval/status-check.test.js +++ b/.github/workflows/test/namespace-approval/status-check.test.js @@ -57,8 +57,8 @@ describe("status-check", () => { head: { sha: "abc123" }, labels: [ { name: "namespace-review-required" }, - { name: "java-namespace-pending" }, - { name: "dotnet-namespace-pending" }, + { name: "namespace-java-pending" }, + { name: "namespace-dotnet-pending" }, ], }, }); @@ -85,7 +85,7 @@ describe("status-check", () => { labels: [ { name: "namespace-review-required" }, { name: "namespace-approved" }, - { name: "java-namespace-approved" }, + { name: "namespace-java-approved" }, ], }, }); diff --git a/.github/workflows/test/namespace-approval/validate-approval.test.js b/.github/workflows/test/namespace-approval/validate-approval.test.js index 3476b2d836a1..f21a5af76a15 100644 --- a/.github/workflows/test/namespace-approval/validate-approval.test.js +++ b/.github/workflows/test/namespace-approval/validate-approval.test.js @@ -89,9 +89,9 @@ describe("validate-approval", () => { it("should skip when namespace-review-required label is absent", async () => { context.payload = createPRLabeledPayload({ action: "labeled", - labelName: "java-namespace-approved", + labelName: "namespace-java-approved", actor: "JonathanGiles", - labels: ["java-namespace-pending"], + labels: ["namespace-java-pending"], }); await validateApproval(args()); @@ -118,9 +118,9 @@ describe("validate-approval", () => { it("should allow authorized approver to approve their language", async () => { context.payload = createPRLabeledPayload({ action: "labeled", - labelName: "java-namespace-approved", + labelName: "namespace-java-approved", actor: "JonathanGiles", - labels: ["namespace-review-required", "java-namespace-pending"], + labels: ["namespace-review-required", "namespace-java-pending"], }); github.rest.pulls.get.mockResolvedValue({ @@ -133,22 +133,22 @@ describe("validate-approval", () => { await validateApproval(args()); expect(github.rest.issues.removeLabel).toHaveBeenCalledWith( - expect.objectContaining({ name: "java-namespace-pending" }), + expect.objectContaining({ name: "namespace-java-pending" }), ); }); it("should reject unauthorized approver and remove label", async () => { context.payload = createPRLabeledPayload({ action: "labeled", - labelName: "java-namespace-approved", + labelName: "namespace-java-approved", actor: "random-user", - labels: ["namespace-review-required", "java-namespace-pending"], + labels: ["namespace-review-required", "namespace-java-pending"], }); await validateApproval(args()); expect(github.rest.issues.removeLabel).toHaveBeenCalledWith( - expect.objectContaining({ name: "java-namespace-approved" }), + expect.objectContaining({ name: "namespace-java-approved" }), ); expect(github.rest.issues.createComment).toHaveBeenCalled(); const calls = vi.mocked(github.rest.issues.createComment).mock.calls; @@ -159,9 +159,9 @@ describe("validate-approval", () => { it("should allow mgmt-plane approver for mgmt PR", async () => { context.payload = createPRLabeledPayload({ action: "labeled", - labelName: "dotnet-namespace-approved", + labelName: "namespace-dotnet-approved", actor: "ArthurMa1978", - labels: ["namespace-review-required", "dotnet-namespace-pending"], + labels: ["namespace-review-required", "namespace-dotnet-pending"], isMgmt: true, }); @@ -175,7 +175,7 @@ describe("validate-approval", () => { await validateApproval(args()); expect(github.rest.issues.removeLabel).toHaveBeenCalledWith( - expect.objectContaining({ name: "dotnet-namespace-pending" }), + expect.objectContaining({ name: "namespace-dotnet-pending" }), ); }); }); @@ -186,7 +186,7 @@ describe("validate-approval", () => { action: "labeled", labelName: "namespace-approved-all", actor: "ArthurMa1978", - labels: ["namespace-review-required", "dotnet-namespace-pending", "java-namespace-pending"], + labels: ["namespace-review-required", "namespace-dotnet-pending", "namespace-java-pending"], }); github.rest.pulls.get.mockResolvedValue({ @@ -199,10 +199,10 @@ describe("validate-approval", () => { await validateApproval(args()); expect(github.rest.issues.removeLabel).toHaveBeenCalledWith( - expect.objectContaining({ name: "dotnet-namespace-pending" }), + expect.objectContaining({ name: "namespace-dotnet-pending" }), ); expect(github.rest.issues.removeLabel).toHaveBeenCalledWith( - expect.objectContaining({ name: "java-namespace-pending" }), + expect.objectContaining({ name: "namespace-java-pending" }), ); }); @@ -211,7 +211,7 @@ describe("validate-approval", () => { action: "labeled", labelName: "namespace-approved-all", actor: "random-user", - labels: ["namespace-review-required", "dotnet-namespace-pending"], + labels: ["namespace-review-required", "namespace-dotnet-pending"], }); await validateApproval(args()); @@ -230,7 +230,7 @@ describe("validate-approval", () => { it("should re-apply pending label when removed by unauthorized user", async () => { context.payload = createPRLabeledPayload({ action: "unlabeled", - labelName: "java-namespace-pending", + labelName: "namespace-java-pending", actor: "random-user", labels: ["namespace-review-required"], }); @@ -239,7 +239,7 @@ describe("validate-approval", () => { expect(github.rest.issues.addLabels).toHaveBeenCalledWith( expect.objectContaining({ - labels: ["java-namespace-pending"], + labels: ["namespace-java-pending"], }), ); expect(github.rest.issues.createComment).toHaveBeenCalled(); @@ -251,7 +251,7 @@ describe("validate-approval", () => { it("should allow trusted bot to remove pending label", async () => { context.payload = createPRLabeledPayload({ action: "unlabeled", - labelName: "java-namespace-pending", + labelName: "namespace-java-pending", actor: "github-actions[bot]", labels: ["namespace-review-required"], }); @@ -268,7 +268,7 @@ describe("validate-approval", () => { it("should allow authorized approver to remove pending label", async () => { context.payload = createPRLabeledPayload({ action: "unlabeled", - labelName: "dotnet-namespace-pending", + labelName: "namespace-dotnet-pending", actor: "jsquire", labels: ["namespace-review-required"], }); From 05b05870ce0b216d57699296937583b778b96d5f Mon Sep 17 00:00:00 2001 From: Sameeksha Vaity Date: Mon, 6 Jul 2026 13:46:06 -0700 Subject: [PATCH 21/23] Extract shared labels helper, remove createLabel, clarify plane detection - Extract removeLabelIfPresent() into labels.js shared module - Remove inline try/catch duplicates in validate-approval.js - Remove createLabel calls - labels should be pre-created on the repo - Add clarifying comments on two-step plane detection logic Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- .../namespace-approval/detect-namespaces.js | 5 ++- .../src/namespace-approval/labels.js | 24 +++++++++++ .../src/namespace-approval/post-results.js | 24 +---------- .../namespace-approval/validate-approval.js | 43 ++----------------- 4 files changed, 32 insertions(+), 64 deletions(-) create mode 100644 .github/workflows/src/namespace-approval/labels.js diff --git a/.github/workflows/src/namespace-approval/detect-namespaces.js b/.github/workflows/src/namespace-approval/detect-namespaces.js index 1ccefb834862..6c1cb71d2c29 100644 --- a/.github/workflows/src/namespace-approval/detect-namespaces.js +++ b/.github/workflows/src/namespace-approval/detect-namespaces.js @@ -49,6 +49,9 @@ async function extractNamespaces(file, namespacesFound, artifactNames, core) { let isMgmt = false; let isDataPlane = false; + // Step 1: Initial guess based on file path (fast, works without parsing). + // Step 2 below overrides this with linter.extends from the parsed config, + // which is the authoritative plane indicator. if (file.includes(".Management/") || file.includes("/resource-manager/")) { isMgmt = true; } else { @@ -64,7 +67,7 @@ async function extractNamespaces(file, namespacesFound, artifactNames, core) { return { isMgmt, isDataPlane }; } - // Linter extends is the most reliable plane indicator — override path-based guess + // Step 2: Linter extends is the authoritative plane indicator — overrides path-based guess const linter = /** @type {Record | undefined} */ (config.linter); if (linter) { const linterExtends = /** @type {string[] | undefined} */ (linter.extends); diff --git a/.github/workflows/src/namespace-approval/labels.js b/.github/workflows/src/namespace-approval/labels.js new file mode 100644 index 000000000000..086e2e5dedbe --- /dev/null +++ b/.github/workflows/src/namespace-approval/labels.js @@ -0,0 +1,24 @@ +/** + * Remove a label from an issue/PR, ignoring 404 (label not present). + * + * @param {import("@actions/github-script").AsyncFunctionArguments["github"]} github + * @param {string} owner + * @param {string} repo + * @param {number} issueNumber + * @param {string} label + */ +export async function removeLabelIfPresent(github, owner, repo, issueNumber, label) { + try { + await github.rest.issues.removeLabel({ + owner, + repo, + issue_number: issueNumber, + name: label, + }); + } catch (error) { + if (error instanceof Error && "status" in error && error.status === 404) { + return; + } + throw error; + } +} diff --git a/.github/workflows/src/namespace-approval/post-results.js b/.github/workflows/src/namespace-approval/post-results.js index caec41cd04bb..e5a5a588b2b9 100644 --- a/.github/workflows/src/namespace-approval/post-results.js +++ b/.github/workflows/src/namespace-approval/post-results.js @@ -6,6 +6,7 @@ import { PER_PAGE_MAX } from "../../../shared/src/github.js"; import { commentOrUpdate, parseExistingComments } from "../comment.js"; import { extractInputs } from "../context.js"; import { loadApproversConfig } from "./approvers.js"; +import { removeLabelIfPresent } from "./labels.js"; const FormatValidationResultSchema = z.object({ valid: z.boolean(), @@ -77,29 +78,6 @@ async function downloadNamespaceResults(github, core, owner, repo, runId) { } } -/** - * @param {import("@actions/github-script").AsyncFunctionArguments["github"]} github - * @param {string} owner - * @param {string} repo - * @param {number} issueNumber - * @param {string} label - */ -async function removeLabelIfPresent(github, owner, repo, issueNumber, label) { - try { - await github.rest.issues.removeLabel({ - owner, - repo, - issue_number: issueNumber, - name: label, - }); - } catch (error) { - if (error instanceof Error && "status" in error && error.status === 404) { - return; - } - throw error; - } -} - /** * @param {import("./approvers.js").ApproversConfig} approversConfig * @param {boolean} isMgmt diff --git a/.github/workflows/src/namespace-approval/validate-approval.js b/.github/workflows/src/namespace-approval/validate-approval.js index ee0984f2931c..376b6bb0d8b1 100644 --- a/.github/workflows/src/namespace-approval/validate-approval.js +++ b/.github/workflows/src/namespace-approval/validate-approval.js @@ -1,5 +1,6 @@ import { extractInputs } from "../context.js"; import { loadApproversConfig } from "./approvers.js"; +import { removeLabelIfPresent } from "./labels.js"; /** * @typedef {Object} ValidateContext @@ -166,16 +167,6 @@ async function handleLabeled({ // Add approved label before removing pending to avoid status check timing gap (#4) if (targetLabel === "namespace-approved-all") { const approvedLabel = `namespace-${lang}-approved`; - try { - await github.rest.issues.getLabel({ owner, repo, name: approvedLabel }); - } catch { - await github.rest.issues.createLabel({ - owner, - repo, - name: approvedLabel, - color: "22c55e", - }); - } await github.rest.issues.addLabels({ owner, repo, @@ -184,16 +175,7 @@ async function handleLabeled({ }); } - try { - await github.rest.issues.removeLabel({ - owner, - repo, - issue_number: prNumber, - name: `namespace-${lang}-pending`, - }); - } catch { - // label may not exist - } + await removeLabelIfPresent(github, owner, repo, prNumber, `namespace-${lang}-pending`); } const { data: currentPR } = await github.rest.pulls.get({ @@ -236,26 +218,7 @@ async function handleLabeled({ if (pendingLabels.length === 0) { core.info("All namespaces approved!"); - try { - await github.rest.issues.removeLabel({ - owner, - repo, - issue_number: prNumber, - name: "namespace-review-required", - }); - } catch { - // label may not exist - } - try { - await github.rest.issues.getLabel({ owner, repo, name: "namespace-approved" }); - } catch { - await github.rest.issues.createLabel({ - owner, - repo, - name: "namespace-approved", - color: "22c55e", - }); - } + await removeLabelIfPresent(github, owner, repo, prNumber, "namespace-review-required"); await github.rest.issues.addLabels({ owner, repo, From b430ea09fd74406c13b8c18dde8fb8c49d2d969b Mon Sep 17 00:00:00 2001 From: Sameeksha Vaity Date: Tue, 7 Jul 2026 10:44:12 -0700 Subject: [PATCH 22/23] Fix NSTM: clarify namespace-approved-all is mgmt-plane only Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- .github/workflows/src/summarize-checks/labelling.js | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/.github/workflows/src/summarize-checks/labelling.js b/.github/workflows/src/summarize-checks/labelling.js index 7293ba21ec06..a3dbe54b4b59 100644 --- a/.github/workflows/src/summarize-checks/labelling.js +++ b/.github/workflows/src/summarize-checks/labelling.js @@ -1171,8 +1171,8 @@ const rulesPri1Namespace = [ "namespace approval from language architects before it can be merged.
" + "Check the Namespace Review Required comment on this PR for a detailed status table " + "showing which languages are pending and who can approve.
" + - "Architects: apply namespace-<language>-approved labels for each language, " + - "or namespace-approved-all to approve all at once.", + "Architects: apply namespace-<language>-approved labels for each language. " + + "For management-plane PRs, namespace-approved-all can be used to approve all languages at once.", }, ]; From c890e3e0951b18ea2507964637fa0ce17fc0d39e Mon Sep 17 00:00:00 2001 From: Mike Harder Date: Wed, 8 Jul 2026 12:56:24 -0700 Subject: [PATCH 23/23] changed-files helper cleanup --- .github/shared/src/changed-files.js | 47 ++++++++++++----------- .github/shared/test/changed-files.test.js | 24 ++++++++---- 2 files changed, 41 insertions(+), 30 deletions(-) diff --git a/.github/shared/src/changed-files.js b/.github/shared/src/changed-files.js index b479d9a42053..0c3edc073e61 100644 --- a/.github/shared/src/changed-files.js +++ b/.github/shared/src/changed-files.js @@ -217,6 +217,30 @@ export function readme(file) { return typeof file === "string" && file.toLowerCase().endsWith("readme.md"); } +/** + * @param {string} file + * @returns {boolean} + */ +export function tsp(file) { + return typeof file === "string" && file.toLowerCase().endsWith(".tsp"); +} + +/** + * @param {string} file + * @returns {boolean} + */ +export function tspconfig(file) { + return typeof file === "string" && file.toLowerCase().endsWith("tspconfig.yaml"); +} + +/** + * @param {string} file + * @returns {boolean} + */ +export function typespec(file) { + return typeof file === "string" && (tsp(file) || tspconfig(file)); +} + /** * @param {string} [file] * @returns {boolean} @@ -270,29 +294,6 @@ export function example(file) { ); } -/** - * @param {string} file - * @returns {boolean} - */ -export function typespec(file) { - return ( - typeof file === "string" && - (file.toLowerCase().endsWith(".tsp") || file.toLowerCase().endsWith("tspconfig.yaml")) - ); -} - -/** - * @param {string} file - * @returns {boolean} - */ -export function tspconfig(file) { - return ( - typeof file === "string" && - file.startsWith("specification/") && - file.endsWith("/tspconfig.yaml") - ); -} - /** * @param {string} [file] * @returns {boolean} diff --git a/.github/shared/test/changed-files.test.js b/.github/shared/test/changed-files.test.js index 1a4c2d352ad5..a574c53c5195 100644 --- a/.github/shared/test/changed-files.test.js +++ b/.github/shared/test/changed-files.test.js @@ -24,6 +24,7 @@ import { scenario, stable, swagger, + tsp, tspconfig, typespec, } from "../src/changed-files.js"; @@ -172,21 +173,30 @@ describe("changedFiles", () => { expect(filesResolved.filter(readme)).toEqual(expected.map((f) => resolve(f))); }); - it("filter:typespec", () => { + it("filter:tsp", () => { const expected = [ "not-spec/contosowidgetmanager/Contoso.Management/main.tsp", - "not-spec/contosowidgetmanager/Contoso.Management/tspconfig.yaml", "specification/contosowidgetmanager/Contoso.Management/main.tsp", - "specification/contosowidgetmanager/Contoso.Management/tspconfig.yaml", ]; - expect(files.filter(typespec)).toEqual(expected); + expect(files.filter(tsp)).toEqual(expected); }); it("filter:tspconfig", () => { - const expected = ["specification/contosowidgetmanager/Contoso.Management/tspconfig.yaml"]; - + const expected = [ + "not-spec/contosowidgetmanager/Contoso.Management/tspconfig.yaml", + "specification/contosowidgetmanager/Contoso.Management/tspconfig.yaml", + ]; expect(files.filter(tspconfig)).toEqual(expected); - expect(filesResolved.filter(tspconfig)).toEqual([]); + }); + + it("filter:typespec", () => { + const expected = [ + "not-spec/contosowidgetmanager/Contoso.Management/main.tsp", + "not-spec/contosowidgetmanager/Contoso.Management/tspconfig.yaml", + "specification/contosowidgetmanager/Contoso.Management/main.tsp", + "specification/contosowidgetmanager/Contoso.Management/tspconfig.yaml", + ]; + expect(files.filter(typespec)).toEqual(expected); }); it("filter:data-plane", () => {