From 2f19d56e4c858c304cb4f0bd3bf74017325bb609 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Thu, 20 Nov 2025 17:58:53 +0000 Subject: [PATCH 1/6] Initial plan From 8cc3634c4fcc36caa4bb8047dd18fe7b6ae81212 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Thu, 20 Nov 2025 18:06:37 +0000 Subject: [PATCH 2/6] Add GitHub Action workflow to block strict version pins without architect approval Co-authored-by: l0lawrence <100643745+l0lawrence@users.noreply.github.com> --- .github/CODEOWNERS | 5 + .github/scripts/check_strict_pins.py | 289 ++++++++++++++++++ .../workflows/check-strict-version-pins.yml | 107 +++++++ 3 files changed, 401 insertions(+) create mode 100755 .github/scripts/check_strict_pins.py create mode 100644 .github/workflows/check-strict-version-pins.yml diff --git a/.github/CODEOWNERS b/.github/CODEOWNERS index fef97554f044..50118481b8a9 100644 --- a/.github/CODEOWNERS +++ b/.github/CODEOWNERS @@ -871,3 +871,8 @@ /pylintrc @l0lawrence @scbedd @mccoyp /sdk/**/ci.yml @msyyc @lmazuel @scbedd + +# Require architect approval for dependency changes (setup.py and pyproject.toml) +# to prevent strict version pins without proper review +/sdk/**/setup.py @kashifkhan @annatisch @johanste +/sdk/**/pyproject.toml @kashifkhan @annatisch @johanste diff --git a/.github/scripts/check_strict_pins.py b/.github/scripts/check_strict_pins.py new file mode 100755 index 000000000000..d481e9a439a1 --- /dev/null +++ b/.github/scripts/check_strict_pins.py @@ -0,0 +1,289 @@ +#!/usr/bin/env python3 +""" +Script to detect strict version pins (==) in Python package dependencies. + +This script checks for new strict version pins introduced in setup.py and pyproject.toml +files within the sdk directory, focusing on main runtime dependencies (install_requires +for setup.py, [project] dependencies for pyproject.toml). + +It ignores: +- dev/test/extras dependencies +- comments +- Changes outside of main dependency sections +""" + +import os +import re +import subprocess +import sys +from typing import Dict, List, Set, Tuple +import json +import urllib.request +import urllib.parse + + +def run_git_command(cmd: List[str]) -> str: + """Run a git command and return its output.""" + try: + result = subprocess.run( + cmd, + capture_output=True, + text=True, + check=True + ) + return result.stdout + except subprocess.CalledProcessError as e: + print(f"Error running git command: {e}") + print(f"stderr: {e.stderr}") + return "" + + +def get_changed_files(base_ref: str, head_ref: str) -> List[str]: + """Get list of changed setup.py and pyproject.toml files in sdk directory.""" + diff_output = run_git_command([ + "git", "diff", "--name-only", "--diff-filter=AM", + base_ref, head_ref + ]) + + files = [] + for line in diff_output.strip().split('\n'): + if line: + if (line.startswith('sdk/') and + (line.endswith('/setup.py') or line.endswith('/pyproject.toml'))): + files.append(line) + + return files + + +def extract_strict_pins_from_setup_py_diff(diff_content: str) -> List[str]: + """ + Extract strict version pins from a setup.py diff. + Only considers additions in install_requires section. + """ + strict_pins = [] + in_install_requires = False + bracket_depth = 0 + + for line in diff_content.split('\n'): + # Skip the +++ file marker + if line.startswith('+++') or line.startswith('---'): + continue + + # Process all lines to track context, but only extract from added lines + actual_line = line[1:].strip() if (line.startswith('+') or line.startswith('-') or line.startswith(' ')) else line.strip() + + # Detect start of install_requires in any line + if 'install_requires' in actual_line and '=' in actual_line: + in_install_requires = True + bracket_depth = 0 + # Check if the array starts on the same line + if '[' in actual_line: + bracket_depth = actual_line.count('[') - actual_line.count(']') + + # Detect end of install_requires or start of other sections + if in_install_requires: + if 'extras_require' in actual_line or 'tests_require' in actual_line: + in_install_requires = False + continue + + # Track brackets in all lines + bracket_depth += actual_line.count('[') - actual_line.count(']') + + # If we close all brackets, we're done with install_requires + if bracket_depth <= 0 and (']' in actual_line or '),' in actual_line): + # Check current line before exiting if it's an added line + if line.startswith('+') and '==' in actual_line and not actual_line.strip().startswith('#'): + match = re.search(r'["\']([^"\']+==[\d\.]+[^"\']*)["\']', actual_line) + if match: + strict_pins.append(match.group(1)) + in_install_requires = False + continue + + # Look for strict pins in added lines within install_requires + if in_install_requires and line.startswith('+'): + if '==' in actual_line and not actual_line.strip().startswith('#'): + # Match package==version pattern + match = re.search(r'["\']([^"\']+==[\d\.]+[^"\']*)["\']', actual_line) + if match: + strict_pins.append(match.group(1)) + + return strict_pins + + +def extract_strict_pins_from_pyproject_diff(diff_content: str) -> List[str]: + """ + Extract strict version pins from a pyproject.toml diff. + Only considers additions in [project] dependencies section. + """ + strict_pins = [] + in_project_dependencies = False + in_other_section = False + + for line in diff_content.split('\n'): + # Skip the +++ and --- file markers + if line.startswith('+++') or line.startswith('---'): + continue + + # Process all lines to track context + actual_line = line[1:].strip() if (line.startswith('+') or line.startswith('-') or line.startswith(' ')) else line.strip() + + # Detect [project] section markers in any line (context or changes) + if actual_line.startswith('['): + if actual_line.startswith('[project]'): + in_other_section = False + elif actual_line.startswith('['): + in_other_section = True + in_project_dependencies = False + + # Detect start of dependencies in [project] section + if not in_other_section and 'dependencies' in actual_line and '=' in actual_line: + if not ('optional-dependencies' in actual_line or + 'dev-dependencies' in actual_line): + in_project_dependencies = True + continue + + # Detect end of dependencies array + if in_project_dependencies and ']' in actual_line and '==' not in actual_line: + in_project_dependencies = False + continue + + # Look for strict pins in added lines within [project] dependencies + if in_project_dependencies and line.startswith('+'): + if '==' in actual_line and not actual_line.strip().startswith('#'): + # Match package==version pattern + match = re.search(r'["\']([^"\']+==[\d\.]+[^"\']*)["\']', actual_line) + if match: + strict_pins.append(match.group(1)) + + return strict_pins + + +def check_file_for_strict_pins(filepath: str, base_ref: str, head_ref: str) -> List[str]: + """Check a specific file for new strict version pins.""" + # Get the diff for this file + diff_output = run_git_command([ + "git", "diff", base_ref, head_ref, "--", filepath + ]) + + if not diff_output: + return [] + + if filepath.endswith('setup.py'): + return extract_strict_pins_from_setup_py_diff(diff_output) + elif filepath.endswith('pyproject.toml'): + return extract_strict_pins_from_pyproject_diff(diff_output) + + return [] + + +def check_architect_approval(pr_number: str, repo: str, github_token: str) -> bool: + """ + Check if any of the architects have approved the PR. + Architects: kashifkhan, annatisch, johanste + """ + architects = {'kashifkhan', 'annatisch', 'johanste'} + + # GitHub API to get PR reviews + url = f"https://api.github.com/repos/{repo}/pulls/{pr_number}/reviews" + + headers = { + 'Authorization': f'token {github_token}', + 'Accept': 'application/vnd.github.v3+json' + } + + try: + req = urllib.request.Request(url, headers=headers) + with urllib.request.urlopen(req) as response: + reviews = json.loads(response.read()) + + for review in reviews: + if review['state'] == 'APPROVED': + reviewer = review['user']['login'] + if reviewer in architects: + print(f"✅ Architect {reviewer} has approved this PR") + return True + + print("❌ No architect approval found") + return False + + except Exception as e: + print(f"Error checking PR reviews: {e}") + # In case of error, we should fail open to not block legitimate PRs + # if the API is down + return False + + +def set_output(name: str, value: str): + """Set GitHub Actions output.""" + github_output = os.getenv('GITHUB_OUTPUT') + if github_output: + with open(github_output, 'a') as f: + # Escape newlines and special characters + escaped_value = value.replace('%', '%25').replace('\n', '%0A').replace('\r', '%0D') + f.write(f"{name}={escaped_value}\n") + else: + print(f"::set-output name={name}::{value}") + + +def main(): + base_ref = os.getenv('BASE_REF', 'origin/main') + head_ref = os.getenv('HEAD_REF', 'HEAD') + pr_number = os.getenv('PR_NUMBER') + repo = os.getenv('REPO') + github_token = os.getenv('GITHUB_TOKEN') + + print(f"Checking for strict version pins...") + print(f"Base: {base_ref}, Head: {head_ref}") + + # Get changed files + changed_files = get_changed_files(base_ref, head_ref) + + if not changed_files: + print("No relevant files changed") + set_output('strict_pins_found', 'false') + set_output('architect_approved', 'false') + set_output('strict_pins_details', '') + return 0 + + print(f"Checking {len(changed_files)} file(s):") + for f in changed_files: + print(f" - {f}") + + # Check each file for strict pins + all_strict_pins = {} + for filepath in changed_files: + strict_pins = check_file_for_strict_pins(filepath, base_ref, head_ref) + if strict_pins: + all_strict_pins[filepath] = strict_pins + + if not all_strict_pins: + print("✅ No new strict version pins found") + set_output('strict_pins_found', 'false') + set_output('architect_approved', 'false') + set_output('strict_pins_details', '') + return 0 + + # Format the findings + details = [] + for filepath, pins in all_strict_pins.items(): + details.append(f"{filepath}:") + for pin in pins: + details.append(f" - {pin}") + + details_str = '\n'.join(details) + print(f"\n⚠️ Strict version pins found:\n{details_str}") + + # Check for architect approval + architect_approved = False + if pr_number and repo and github_token: + architect_approved = check_architect_approval(pr_number, repo, github_token) + + set_output('strict_pins_found', 'true') + set_output('architect_approved', 'true' if architect_approved else 'false') + set_output('strict_pins_details', details_str) + + return 0 + + +if __name__ == '__main__': + sys.exit(main()) diff --git a/.github/workflows/check-strict-version-pins.yml b/.github/workflows/check-strict-version-pins.yml new file mode 100644 index 000000000000..3ca15093eb3f --- /dev/null +++ b/.github/workflows/check-strict-version-pins.yml @@ -0,0 +1,107 @@ +name: Check for Strict Version Pins + +on: + pull_request: + paths: + - 'sdk/**/setup.py' + - 'sdk/**/pyproject.toml' + +permissions: + pull-requests: read + contents: read + +jobs: + check-strict-pins: + runs-on: ubuntu-latest + steps: + - name: Checkout repository + uses: actions/checkout@v4 + with: + fetch-depth: 0 + + - name: Set up Python + uses: actions/setup-python@v5 + with: + python-version: '3.11' + + - name: Fetch base branch + run: | + git fetch origin ${{ github.base_ref }} + + - name: Check for strict version pins + id: check-pins + env: + GITHUB_TOKEN: ${{ secrets.GITHUB_TOKEN }} + PR_NUMBER: ${{ github.event.pull_request.number }} + REPO: ${{ github.repository }} + BASE_REF: origin/${{ github.base_ref }} + HEAD_REF: ${{ github.sha }} + run: | + python .github/scripts/check_strict_pins.py + + - name: Comment on PR if strict pins found + if: steps.check-pins.outputs.strict_pins_found == 'true' + uses: actions/github-script@v7 + with: + script: | + const strictPins = `${{ steps.check-pins.outputs.strict_pins_details }}`; + const architectApproved = `${{ steps.check-pins.outputs.architect_approved }}`; + + let message = '## ⚠️ Strict Version Pin Detected\n\n'; + message += 'This PR introduces one or more strict version pins (`==`) in main runtime dependencies:\n\n'; + message += '```\n' + strictPins + '\n```\n\n'; + + if (architectApproved === 'true') { + message += '✅ An architect has approved this PR.\n'; + } else { + message += '❌ **This PR requires approval from one of the following architects:**\n'; + message += '- @kashifkhan\n'; + message += '- @annatisch\n'; + message += '- @johanste\n\n'; + message += 'Please request a review from one of these architects before merging.\n'; + } + + // Check if comment already exists + const { data: comments } = await github.rest.issues.listComments({ + owner: context.repo.owner, + repo: context.repo.repo, + issue_number: context.issue.number, + }); + + const botComment = comments.find(comment => + comment.user.type === 'Bot' && + comment.body.includes('Strict Version Pin Detected') + ); + + if (botComment) { + await github.rest.issues.updateComment({ + owner: context.repo.owner, + repo: context.repo.repo, + comment_id: botComment.id, + body: message + }); + } else { + await github.rest.issues.createComment({ + owner: context.repo.owner, + repo: context.repo.repo, + issue_number: context.issue.number, + body: message + }); + } + + - name: Block merge if architect approval required + if: steps.check-pins.outputs.strict_pins_found == 'true' && steps.check-pins.outputs.architect_approved == 'false' + run: | + echo "::error::Strict version pins detected without architect approval" + echo "This PR introduces strict version pins (==) in main runtime dependencies." + echo "Approval required from: kashifkhan, annatisch, or johanste" + exit 1 + + - name: Success - No strict pins or approved + if: steps.check-pins.outputs.strict_pins_found == 'false' || steps.check-pins.outputs.architect_approved == 'true' + run: | + if [ "${{ steps.check-pins.outputs.strict_pins_found }}" == "true" ]; then + echo "✅ Strict version pins detected but architect has approved" + else + echo "✅ No new strict version pins detected in main runtime dependencies" + fi From d0e5840954f3034f69e98776ac03360db1accc74 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Thu, 20 Nov 2025 18:08:52 +0000 Subject: [PATCH 3/6] Add comprehensive documentation for strict version pin workflow Co-authored-by: l0lawrence <100643745+l0lawrence@users.noreply.github.com> --- .../workflows/README-strict-version-pins.md | 154 ++++++++++++++++++ 1 file changed, 154 insertions(+) create mode 100644 .github/workflows/README-strict-version-pins.md diff --git a/.github/workflows/README-strict-version-pins.md b/.github/workflows/README-strict-version-pins.md new file mode 100644 index 000000000000..52f87e7becc9 --- /dev/null +++ b/.github/workflows/README-strict-version-pins.md @@ -0,0 +1,154 @@ +# Strict Version Pin Check Workflow + +## Overview + +This GitHub Actions workflow enforces a policy that requires architect approval for any pull requests that introduce strict version pins (`==`) in main runtime dependencies of Python packages within the `sdk/` directory. + +## Purpose + +Strict version pins can cause dependency conflicts and limit flexibility in package management. This workflow ensures that any such pins in main runtime dependencies are reviewed and approved by designated architects before merging. + +## How It Works + +### Trigger +The workflow runs on pull requests that modify: +- `sdk/**/setup.py` +- `sdk/**/pyproject.toml` + +### Detection Logic + +The workflow analyzes the diff to detect: +- **New strict version pins**: Dependencies newly added with `==` operator +- **Modified pins**: Dependencies changed from flexible constraints (e.g., `>=`, `~=`) to strict `==` pins + +The detection is **scope-aware** and only considers: +- `install_requires` in `setup.py` +- `dependencies` under `[project]` in `pyproject.toml` + +The following are **ignored**: +- Dev dependencies (`extras_require`, `[project.optional-dependencies]`) +- Test dependencies (`tests_require`) +- Comments +- Build dependencies + +### Approval Requirements + +If strict version pins are detected, the workflow: +1. Posts a comment on the PR listing the detected pins +2. Checks if any of the following architects have approved the PR: + - `@kashifkhan` + - `@annatisch` + - `@johanste` +3. **Blocks merging** if no architect approval is found (workflow fails with exit code 1) +4. **Allows merging** if an architect has approved + +### CODEOWNERS Integration + +The `.github/CODEOWNERS` file has been updated to require reviews from the architects for: +- `/sdk/**/setup.py` +- `/sdk/**/pyproject.toml` + +This provides a secondary enforcement mechanism through branch protection rules. + +## Examples + +### ✅ Allowed (No Strict Pins) +```python +# setup.py +install_requires=[ + "azure-core>=1.30.0", + "requests>=2.21.0", +] +``` + +### ⚠️ Requires Architect Approval +```python +# setup.py +install_requires=[ + "azure-core==1.30.0", # Strict pin detected! + "requests>=2.21.0", +] +``` + +### ✅ Allowed (Strict Pin in Dev Dependencies) +```python +# setup.py +install_requires=[ + "azure-core>=1.30.0", +], +extras_require={ + "dev": ["pytest==7.0.0"] # OK - this is a dev dependency +} +``` + +## Testing + +The detection logic has been validated with comprehensive test cases covering: +- Adding new strict pins +- Changing from flexible to strict constraints +- Ignoring dev/test dependencies +- Ignoring optional dependencies in pyproject.toml + +Run tests locally: +```bash +python /tmp/test_strict_pins.py +``` + +## Files Modified + +1. **`.github/workflows/check-strict-version-pins.yml`** + - Main workflow definition + - Triggers on PR events + - Runs detection and enforcement logic + +2. **`.github/scripts/check_strict_pins.py`** + - Python script that analyzes git diffs + - Detects strict version pins in appropriate sections + - Checks for architect approvals via GitHub API + +3. **`.github/CODEOWNERS`** + - Added architect requirements for setup.py and pyproject.toml + +## Branch Protection + +To fully enforce this policy, ensure branch protection rules are configured to: +- Require status checks to pass before merging +- Require the "check-strict-pins" workflow to succeed +- Require review from code owners + +## Troubleshooting + +### Workflow Not Running +- Verify the PR modifies files matching `sdk/**/setup.py` or `sdk/**/pyproject.toml` +- Check workflow runs in the Actions tab + +### False Positives +If the workflow incorrectly flags a dependency: +- Verify the dependency is in the main runtime dependencies section +- Check if comments are interfering with detection +- File an issue with the specific case + +### Override Process +If a strict pin is necessary: +1. Document the justification in the PR description +2. Request review from one of the architects: + - @kashifkhan + - @annatisch + - @johanste +3. Architect provides approval review +4. Workflow will pass and allow merge + +## Maintenance + +### Adding/Removing Architects +To modify the list of architects: +1. Update the `architects` set in `.github/scripts/check_strict_pins.py` +2. Update the CODEOWNERS entries in `.github/CODEOWNERS` +3. Update documentation in this README + +### Extending Detection +To add support for additional dependency formats: +1. Add extraction function in `check_strict_pins.py` +2. Update `check_file_for_strict_pins()` to handle new file types +3. Add corresponding test cases +4. Update workflow triggers if needed From c2684befe9d98967404982f0bae8b0ac0f85d268 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Thu, 20 Nov 2025 20:59:27 +0000 Subject: [PATCH 4/6] Fix CI errors: add pull-requests write permission and move README out of workflows directory Co-authored-by: l0lawrence <100643745+l0lawrence@users.noreply.github.com> --- .github/{workflows => }/README-strict-version-pins.md | 0 .github/workflows/check-strict-version-pins.yml | 2 +- 2 files changed, 1 insertion(+), 1 deletion(-) rename .github/{workflows => }/README-strict-version-pins.md (100%) diff --git a/.github/workflows/README-strict-version-pins.md b/.github/README-strict-version-pins.md similarity index 100% rename from .github/workflows/README-strict-version-pins.md rename to .github/README-strict-version-pins.md diff --git a/.github/workflows/check-strict-version-pins.yml b/.github/workflows/check-strict-version-pins.yml index 3ca15093eb3f..afec2732526b 100644 --- a/.github/workflows/check-strict-version-pins.yml +++ b/.github/workflows/check-strict-version-pins.yml @@ -7,7 +7,7 @@ on: - 'sdk/**/pyproject.toml' permissions: - pull-requests: read + pull-requests: write contents: read jobs: From 45517b796b7e7f5f4845a7da8273d8475cbc5933 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Thu, 20 Nov 2025 21:46:42 +0000 Subject: [PATCH 5/6] Remove codeowner names from README documentation Co-authored-by: l0lawrence <100643745+l0lawrence@users.noreply.github.com> --- .github/README-strict-version-pins.md | 10 ++-------- 1 file changed, 2 insertions(+), 8 deletions(-) diff --git a/.github/README-strict-version-pins.md b/.github/README-strict-version-pins.md index 52f87e7becc9..6fcdb50e845b 100644 --- a/.github/README-strict-version-pins.md +++ b/.github/README-strict-version-pins.md @@ -35,10 +35,7 @@ The following are **ignored**: If strict version pins are detected, the workflow: 1. Posts a comment on the PR listing the detected pins -2. Checks if any of the following architects have approved the PR: - - `@kashifkhan` - - `@annatisch` - - `@johanste` +2. Checks if any of the designated architects have approved the PR 3. **Blocks merging** if no architect approval is found (workflow fails with exit code 1) 4. **Allows merging** if an architect has approved @@ -131,10 +128,7 @@ If the workflow incorrectly flags a dependency: ### Override Process If a strict pin is necessary: 1. Document the justification in the PR description -2. Request review from one of the architects: - - @kashifkhan - - @annatisch - - @johanste +2. Request review from one of the designated architects 3. Architect provides approval review 4. Workflow will pass and allow merge From 67cc77265986c90eface658173806944fcb3bc9e Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Thu, 20 Nov 2025 23:11:27 +0000 Subject: [PATCH 6/6] Add .github/scripts/** to cspell ignorePaths Co-authored-by: l0lawrence <100643745+l0lawrence@users.noreply.github.com> --- .vscode/cspell.json | 1 + 1 file changed, 1 insertion(+) diff --git a/.vscode/cspell.json b/.vscode/cspell.json index dc421e3d78e9..c35d53023c78 100644 --- a/.vscode/cspell.json +++ b/.vscode/cspell.json @@ -14,6 +14,7 @@ ".devcontainer/**", "eng/**", ".github/CODEOWNERS", + ".github/scripts/**", "scripts/**", "**/tests/recordings/**", ".vscode/cspell.json",