Conversation
There was a problem hiding this comment.
Pull request overview
This PR aims to prevent concurrent XLS number assignment collisions by serializing the assignment workflow and reserving numbers across open PRs via a bot comment marker + label.
Changes:
- Added a workflow-level
concurrencygroup for the XLS number assignment workflow. - Updated the workflow to always upsert the assignment comment and to add a
has-xls-numberlabel. - Extended the assignment script to look up reserved XLS numbers from other open PRs via the GitHub REST API and reuse a previously reserved number for the current PR.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| .github/workflows/assign-xls-number.yml | Adds concurrency control, updates comment upsert behavior, and labels PRs to support reservation tracking. |
| .github/scripts/assign_xls_number.py | Adds GitHub API querying + comment-marker parsing to treat numbers as reserved across open PRs. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| - name: Assign XLS number | ||
| if: steps.check-drafts.outputs.has_drafts == 'true' | ||
| id: assign-number | ||
| env: | ||
| REPO_ROOT: ${{ github.workspace }} | ||
| run: | | ||
| echo ${{ steps.check-drafts.outputs.draft_files }} | ||
| python base-repo/.github/scripts/assign_xls_number.py ${{ steps.check-drafts.outputs.draft_files }} | ||
|
|
There was a problem hiding this comment.
The Python script now queries the GitHub API for reserved XLS numbers via GITHUB_TOKEN, but this workflow step doesn’t pass GITHUB_TOKEN into the script environment. In GitHub Actions, secrets.GITHUB_TOKEN isn’t automatically exposed as an env var named GITHUB_TOKEN, so the reservation logic will be skipped and concurrency issues can still occur. Export GITHUB_TOKEN (e.g., from ${{ github.token }} or ${{ secrets.GITHUB_TOKEN }}) for the Assign XLS number step.
| for comment in comments: | ||
| if comment.get("user", {}).get("login") != "github-actions[bot]": | ||
| continue | ||
| body = comment.get("body") or "" | ||
| match = re.search(r"<!--\s*XLS_NUMBER:(\\d+)\s*-->", body) | ||
| if match: | ||
| try: | ||
| return int(match.group(1)) | ||
| except ValueError: |
There was a problem hiding this comment.
The regex used to extract the reserved number from the bot comment is incorrectly escaped: r"...(\\d+)" matches a literal \d instead of digits, so this will never detect the XLS number marker and reservations won’t work. Use r"...(\d+)" (single backslash in the raw string) to match digits.
| all_numbers.add(assigned_number) | ||
| new_dir_name = re.sub(r"^xls-draft-", f"xls-{assigned_number:04d}-", draft_dir.lower()) | ||
| assignments.append({ |
There was a problem hiding this comment.
new_dir_name is forced to lowercase (draft_dir.lower()), producing names like xls-0096-..., but the repository’s existing XLS directories are XLS-####-... and get_existing_xls_numbers() only detects the uppercase form. If editors follow the workflow comment and rename to lowercase, future runs won’t treat those directories as “existing” and may reassign duplicate numbers. Make directory naming consistent (either output XLS-####-... or update the scanner to accept both cases).
| curl -sS \ | ||
| -H "Authorization: Bearer $GITHUB_TOKEN" \ | ||
| -H "Accept: application/vnd.github+json" \ | ||
| -X POST \ | ||
| -d '{"labels":["has-xls-number"]}' \ | ||
| "https://api.github.com/repos/${{ github.repository }}/issues/${{ github.event.pull_request.number }}/labels" |
There was a problem hiding this comment.
This curl call won’t fail the step on HTTP errors (e.g., insufficient permissions / invalid repo / API error) because it doesn’t use --fail/--fail-with-body or check the response status. That can leave PRs unlabeled while the workflow still succeeds, breaking the reservation mechanism. Make the step fail on non-2xx (or explicitly handle and log failures) by checking the HTTP status code.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| for comment in comments: | ||
| if comment.get("user", {}).get("login") != "github-actions[bot]": | ||
| continue | ||
| body = comment.get("body") or "" | ||
| match = re.search(r"<!--\s*XLS_NUMBER:(\\d+)\s*-->", body) | ||
| if match: | ||
| try: | ||
| return int(match.group(1)) | ||
| except ValueError: | ||
| continue |
There was a problem hiding this comment.
The reserved-number extraction regex is incorrect: the pattern uses \\d+ inside a raw string, which matches a literal "\d" instead of digits. As a result, the script will never find <!-- XLS_NUMBER:... --> markers and will treat all open PRs as having no reserved number. Change the regex to use \d+ (single backslash in a raw string).
| def github_api_request(path: str, token: str, params: dict | None = None): | ||
| """Small helper to call the GitHub REST API. | ||
|
|
||
| Returns parsed JSON or an empty list/dict on failure. | ||
| """ | ||
|
|
||
| base_url = "https://api.github.com" | ||
| url = f"{base_url}{path}" | ||
| if params: | ||
| query = parse.urlencode(params) | ||
| url = f"{url}?{query}" | ||
|
|
||
| headers = { | ||
| "Accept": "application/vnd.github+json", | ||
| "User-Agent": "xrpl-standards-xls-number-bot", | ||
| } | ||
| if token: | ||
| headers["Authorization"] = f"Bearer {token}" | ||
|
|
||
| req = request.Request(url, headers=headers) | ||
|
|
||
| try: | ||
| with request.urlopen(req) as resp: | ||
| data = resp.read() | ||
| return json.loads(data.decode("utf-8")) | ||
| except error.HTTPError as e: | ||
| print(f"Warning: GitHub API HTTP error {e.code} for {url}: {e.reason}") | ||
| except error.URLError as e: | ||
| print(f"Warning: GitHub API URL error for {url}: {e.reason}") |
There was a problem hiding this comment.
github_api_request() uses request.urlopen(req) without a timeout. Since this runs in CI, a stalled network call can hang the entire workflow indefinitely. Pass an explicit timeout to urlopen (and consider handling json.JSONDecodeError as well) so failures are bounded and reported cleanly.
| - name: Checkout repository | ||
| uses: actions/checkout@v6 | ||
| with: | ||
| fetch-depth: 0 | ||
| ref: ${{ github.event.pull_request.head.sha }} | ||
|
|
||
| - name: Checkout script from base branch | ||
| uses: actions/checkout@v6 | ||
| with: | ||
| ref: ${{ github.event.pull_request.base.ref }} | ||
| sparse-checkout: | | ||
| .github/scripts/assign_xls_number.py | ||
| sparse-checkout-cone-mode: false | ||
| path: base-repo | ||
|
|
There was a problem hiding this comment.
The script derives existing_numbers by scanning the checked-out working tree at REPO_ROOT, but this workflow checks out the PR head SHA (not the base branch). If the PR branch is behind master, the scan can miss recently-merged XLS-<NNNN>-... directories and suggest a number that’s already taken on the base branch. Consider scanning against the base ref (e.g., checkout the base branch to a separate path and point REPO_ROOT there, or query the tree at github.event.pull_request.base.sha).
High Level Overview of Change
Context of Change
Type of Change