Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
30 commits
Select commit Hold shift + click to select a range
c79e985
Remove unused .husky folder
vvolkgang Nov 11, 2025
1e58893
Label PR script and workflow - first draft
vvolkgang Nov 11, 2025
d204171
invert loops
vvolkgang Nov 11, 2025
125a2f6
Add catch-all label
vvolkgang Nov 11, 2025
d3a3285
Re-inverted loops, it's less verbose
vvolkgang Nov 11, 2025
025a994
Implement PR Title labeling
vvolkgang Nov 11, 2025
8cfa32d
Check for conventional commit format
vvolkgang Nov 11, 2025
dbccb55
Add script docs and cleanup
vvolkgang Nov 11, 2025
18a4838
Implement argparse, refactor arguments and add dry-run option
vvolkgang Nov 11, 2025
104d510
Workflow - Add workflow_dispatch. Removing pull_request for now aheadโ€ฆ
vvolkgang Nov 11, 2025
b6aae4d
Workflow - determine label mode for pull requests and use the new scrโ€ฆ
vvolkgang Nov 11, 2025
e2279b3
Address zizmor feedback
vvolkgang Nov 11, 2025
bd15be2
Workflow - support retrieving PR data on workflow_dispatch triggers
vvolkgang Nov 11, 2025
9e3c584
Fix script typo
vvolkgang Nov 11, 2025
470158c
Cleanup
vvolkgang Nov 11, 2025
51c539e
json config draft
vvolkgang Nov 11, 2025
3578b08
Invert mode check to use the default mode
vvolkgang Nov 11, 2025
352a2e4
Move arg parsing to it's own method and
vvolkgang Nov 11, 2025
bcf6981
Move config file validation to the file loading method
vvolkgang Nov 11, 2025
d339d5d
Add comment
vvolkgang Nov 11, 2025
a3b7def
Add workflow logs
vvolkgang Nov 11, 2025
043021f
Merge branch 'main' into vvolkgang/label-prs
vvolkgang Nov 11, 2025
99983ca
Invert default mode check
vvolkgang Nov 11, 2025
c6505e3
Return empty list / string on error
vvolkgang Nov 11, 2025
625d9a7
Improve error logs
vvolkgang Nov 12, 2025
7b32be4
Add new line to changed files
vvolkgang Nov 12, 2025
ee72ba2
fix: Script error when $_DRY_RUN is empty
vvolkgang Nov 12, 2025
701c404
Add minimum python version comment
vvolkgang Nov 12, 2025
1e39e26
Add enhancement label
vvolkgang Nov 12, 2025
51677ba
Merge branch 'main' into vvolkgang/label-prs
vvolkgang Nov 17, 2025
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
49 changes: 49 additions & 0 deletions .github/label-pr.json
Original file line number Diff line number Diff line change
@@ -0,0 +1,49 @@
{
"catch_all_label": "t:misc",
"title_patterns": {
"t:new-feature": ["feat", "feature"],
"t:enhancement": ["enhancement", "enh", "impr"],
"t:bug": ["fix", "bug", "bugfix"],
"t:tech-debt": ["refactor", "chore", "cleanup", "revert", "debt", "test", "perf"],
"t:docs": ["docs"],
"t:ci": ["ci", "build", "chore(ci)"],
"t:deps": ["deps"],
"t:breaking-change": ["breaking", "breaking-change"],
"t:misc": ["misc"]
},
"path_patterns": {
"app:shared": [
"annotation/",
"core/",
"data/",
"network/",
"ui/",
"authenticatorbridge/",
"gradle/"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

๐Ÿ’ญ Pattern overlap: gradle/ appears in both app:shared and t:deps

Details

The gradle/ directory pattern is defined in:

  • app:shared (line 22): Triggers app:password-manager + app:authenticator labels
  • t:deps (line 43): Triggers t:deps label

When a PR modifies gradle/ files, it will receive three labels: app:password-manager, app:authenticator, and t:deps.

This might be intentional (gradle changes affect both apps and are dependency-related), but worth confirming this is the desired behavior. If gradle changes should only be labeled as t:deps, remove it from the app:shared list.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

๐Ÿ’ญ Pattern overlap: gradle/ appears in both app:shared and t:deps.

Details

The gradle/ directory pattern is defined in:

  • app:shared (line 22): Triggers expansion to app:password-manager + app:authenticator labels
  • t:deps (line 43): Triggers t:deps label

When a PR modifies gradle/ files, it will receive three labels: app:password-manager, app:authenticator, and t:deps.

This might be intentional (gradle changes affect both apps and are dependency-related), but worth confirming this is the desired behavior. If gradle changes should only be labeled as t:deps, remove it from the app:shared list.

],
"app:password-manager": [
"app/",
"cxf/"
],
"app:authenticator": [
"authenticator/"
],
"t:ci": [
".github/",
"scripts/",
"fastlane/",
".gradle/",
".claude/",
"detekt-config.yml"
],
"t:docs": [
"docs/"
],
"t:deps": [
"gradle/"
],
"t:misc": [
"keystore/"
]
}
}
238 changes: 238 additions & 0 deletions .github/scripts/label-pr.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,238 @@
#!/usr/bin/env python3
# Requires Python 3.9+
"""
Label pull requests based on changed file paths and PR title patterns (conventional commit format).

Usage:
python label-pr.py <pr-number> [-a|--add|-r|--replace] [-d|--dry-run] [-c|--config CONFIG]

Arguments:
pr-number: The pull request number
-a, --add: Add labels without removing existing ones (default)
-r, --replace: Replace all existing labels
-d, --dry-run: Run without actually applying labels
-c, --config: Path to JSON config file (default: .github/label-pr.json)

Examples:
python label-pr.py 1234
python label-pr.py 1234 -a
python label-pr.py 1234 --replace
python label-pr.py 1234 -r -d
python label-pr.py 1234 --config custom-config.json
"""

import argparse
import json
import os
import subprocess
import sys

DEFAULT_MODE = "add"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

โš ๏ธ Documentation contradicts implementation regarding default mode.

Details

The constant DEFAULT_MODE = "add" and documentation at lines 11-12 state that --add is the default:

-a, --add: Add labels without removing existing ones (default)
-r, --replace: Replace all existing labels

However, the implementation at line 202 defaults to "add" only when --add is explicitly provided, otherwise it uses "replace":

mode = "replace" if args.replace else "add"

This means when neither flag is provided, the mode is "add", which actually matches the documentation. However, the workflow at .github/workflows/sdlc-label-pr.yml:68-69 uses --replace as the default for normal PRs, creating confusion about intended behavior.

Recommendation: Clarify whether the default should be "add" or "replace" and update either the documentation or the workflow accordingly for consistency.

DEFAULT_CONFIG_PATH = ".github/label-pr.json"

def load_config_json(config_file: str) -> dict:
"""Load configuration from JSON file."""
if not os.path.exists(config_file):
print(f"โŒ Config file not found: {config_file}")
sys.exit(1)

try:
with open(config_file, 'r') as f:
config = json.load(f)
print(f"โœ… Loaded config from: {config_file}")

valid_config = True
if not config.get("catch_all_label"):
print("โŒ Missing 'catch_all_label' in config file")
valid_config = False
if not config.get("title_patterns"):
print("โŒ Missing 'title_patterns' in config file")
valid_config = False
if not config.get("path_patterns"):
print("โŒ Missing 'path_patterns' in config file")
valid_config = False

if not valid_config:
print("::error::Invalid label-pr.json config file, exiting...")
sys.exit(1)

return config
except json.JSONDecodeError as e:
print(f"โŒ JSON deserialization error in label-pr.json config: {e}")
sys.exit(1)
except Exception as e:
print(f"โŒ Unexpected error loading label-pr.json config: {e}")
sys.exit(1)

def gh_get_changed_files(pr_number: str) -> list[str]:
"""Get list of changed files in a pull request."""
try:
result = subprocess.run(
["gh", "pr", "diff", pr_number, "--name-only"],
capture_output=True,
text=True,
check=True
)
changed_files = result.stdout.strip().split("\n")
return list(filter(None, changed_files))
except subprocess.CalledProcessError as e:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

โš ๏ธ Silent failure: Returns empty list on error without clear indication

Details

When gh pr diff fails, the function prints an error message but returns an empty list [], which is indistinguishable from a PR with no changed files. This could mask genuine API failures.

The caller at line 212 doesn't distinguish between:

  1. API failure (should potentially fail the workflow)
  2. Legitimately no files changed (should continue with title-based labeling)

Current behavior:

changed_files = gh_get_changed_files(pr_number)  # Returns [] on error
print("๐Ÿ‘€ Changed files:\n" + "\n".join(changed_files) + "\n")  # Prints empty list

Recommendation:
Either:

  1. Exit the script on critical failures:
def gh_get_changed_files(pr_number: str) -> list[str]:
    try:
        # ... existing code ...
    except subprocess.CalledProcessError as e:
        print(f"::error::Error getting changed files: {e}")
        sys.exit(1)  # Critical failure
  1. Or add explicit validation after the call:
changed_files = gh_get_changed_files(pr_number)
if changed_files is None:  # Return None on error instead of []
    print("::error::Failed to retrieve changed files")
    sys.exit(1)

Same applies to gh_get_pr_title() at line 93.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

โš ๏ธ Silent failure returns empty list on error, masking API failures.

Details

When gh pr diff fails, the function prints an error but returns an empty list [], which is indistinguishable from a PR with no changed files. This could mask genuine API failures.

The caller at line 212 cannot distinguish between:

  1. API failure (should potentially fail the workflow)
  2. Legitimately no files changed (should continue with title-based labeling)

Current behavior:

changed_files = gh_get_changed_files(pr_number)  # Returns [] on error
print("๐Ÿ‘€ Changed files:\n" + "\n".join(changed_files) + "\n")  # Prints empty list

Recommendation: Either exit on critical failures or return None on error and handle it explicitly:

def gh_get_changed_files(pr_number: str) -> list[str] | None:
    try:
        # ... existing code ...
    except subprocess.CalledProcessError as e:
        print(f"::error::Error getting changed files: {e}")
        return None  # Signal error explicitly

# At call site:
changed_files = gh_get_changed_files(pr_number)
if changed_files is None:
    print("::error::Failed to retrieve changed files")
    sys.exit(1)

Same issue exists in gh_get_pr_title() at line 92.

print(f"::error::Error getting changed files: {e}")
return []

def gh_get_pr_title(pr_number: str) -> str:
"""Get the title of a pull request."""
try:
result = subprocess.run(
["gh", "pr", "view", pr_number, "--json", "title", "--jq", ".title"],
capture_output=True,
text=True,
check=True
)
return result.stdout.strip()
except subprocess.CalledProcessError as e:
print(f"::error::Error getting PR title: {e}")
return ""

def gh_add_labels(pr_number: str, labels: list[str]) -> None:
"""Add labels to a pull request (doesn't remove existing labels)."""
gh_labels = ','.join(labels)
subprocess.run(
["gh", "pr", "edit", pr_number, "--add-label", gh_labels],
check=True
)

def gh_replace_labels(pr_number: str, labels: list[str]) -> None:
"""Replace all labels on a pull request with the specified labels."""
payload = json.dumps({"labels": labels})
subprocess.run(
["gh", "api", "repos/{owner}/{repo}/issues/" + pr_number, "-X", "PATCH", "--silent", "--input", "-"],
input=payload,
text=True,
check=True
)

def label_filepaths(changed_files: list[str], path_patterns: dict) -> list[str]:
"""Check changed files against path patterns and return labels to apply."""
if not changed_files:
return []

labels_to_apply = set() # Use set to avoid duplicates

for label, patterns in path_patterns.items():
for file in changed_files:
if any(file.startswith(pattern) for pattern in patterns):
print(f"๐Ÿ‘€ File '{file}' matches pattern for label '{label}'")
labels_to_apply.add(label)
break

if "app:shared" in labels_to_apply:
labels_to_apply.add("app:password-manager")
labels_to_apply.add("app:authenticator")
labels_to_apply.remove("app:shared")

if not labels_to_apply:
print("::warning::No matching file paths found, no labels applied.")

return list(labels_to_apply)

def label_title(pr_title: str, title_patterns: dict) -> list[str]:
"""Check PR title against patterns and return labels to apply."""
if not pr_title:
return []

labels_to_apply = set()
title_lower = pr_title.lower()
for label, patterns in title_patterns.items():
for pattern in patterns:
# Check for pattern with : or ( suffix (conventional commits format)
if f"{pattern}:" in title_lower or f"{pattern}(" in title_lower:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

๐ŸŽจ Title pattern matching could be more precise

Details

Current logic checks if pattern appears anywhere in the title with : or ( suffix:

if f"{pattern}:" in title_lower or f"{pattern}(" in title_lower:

This could produce false positives. Examples:

  • Pattern "fix" matches title "Add prefix: bug" (contains "fix:" in "prefix:")
  • Pattern "feat" matches title "defeat: the enemy" (contains "feat:")

Recommendation:
Use more precise matching to ensure pattern is at start of title or after whitespace:

# Check for pattern at beginning or after whitespace
import re
pattern_regex = rf"(^|\s){re.escape(pattern)}[:(]"
if re.search(pattern_regex, title_lower):
    print(f"๐Ÿ“ Title matches pattern '{pattern}' for label '{label}'")
    labels_to_apply.add(label)
    break

Or simpler string-based approach:

# Check pattern is at start or preceded by space
if title_lower.startswith(f"{pattern}:") or title_lower.startswith(f"{pattern}(") or \
   f" {pattern}:" in title_lower or f" {pattern}(" in title_lower:

Current implementation works for well-formatted conventional commits, but could be more robust.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

๐ŸŽจ Title pattern matching could produce false positives.

Details

Current logic checks if pattern appears anywhere in the title:

if f"{pattern}:" in title_lower or f"{pattern}(" in title_lower:

This could match incorrectly. Examples:

  • Pattern "fix" matches title "Add prefix: bug" (contains "fix:" within "prefix:")
  • Pattern "feat" matches title "defeat: the enemy" (contains "feat:" within "defeat:")

Recommendation: Use more precise matching to ensure pattern is at word boundary:

import re

# At function level, check pattern at start or after whitespace
pattern_regex = rf"(^|\s){re.escape(pattern)}[:(]"
if re.search(pattern_regex, title_lower):
    print(f"๐Ÿ“ Title matches pattern '{pattern}' for label '{label}'")
    labels_to_apply.add(label)
    break

Or simpler string-based approach:

# Check pattern is at start or preceded by space
if title_lower.startswith(f"{pattern}:") or title_lower.startswith(f"{pattern}(") or \
   f" {pattern}:" in title_lower or f" {pattern}(" in title_lower:

Current implementation works for well-formatted conventional commits, but could be more robust.

print(f"๐Ÿ“ Title matches pattern '{pattern}' for label '{label}'")
labels_to_apply.add(label)
break

if not labels_to_apply:
print("::warning::No matching title patterns found, no labels applied.")

return list(labels_to_apply)

def parse_args():
"""Parse command line arguments."""
parser = argparse.ArgumentParser(
description="Label pull requests based on changed file paths and PR title patterns."
)
parser.add_argument(
"pr_number",
help="The pull request number"
)

mode_group = parser.add_mutually_exclusive_group()
mode_group.add_argument(
"-a", "--add",
action="store_true",
help="Add labels without removing existing ones (default)"
)
mode_group.add_argument(
"-r", "--replace",
action="store_true",
help="Replace all existing labels"
)

parser.add_argument(
"-d", "--dry-run",
action="store_true",
help="Run without actually applying labels"
)

parser.add_argument(
"-c", "--config",
default=DEFAULT_CONFIG_PATH,
help=f"Path to JSON config file (default: {DEFAULT_CONFIG_PATH})"
)
args, unknown = parser.parse_known_args() # required to handle --dry-run passed as an empty string ("") by the workflow
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

๐Ÿ’ญ parse_known_args() is a workaround for shell quoting issue in workflow.

Details

The comment states this is "required to handle --dry-run passed as an empty string".

This happens because the workflow at .github/workflows/sdlc-label-pr.yml:76-79 passes $_DRY_RUN which can be an empty string when dry-run is false:

_DRY_RUN: ${{ inputs.dry-run == true && '--dry-run' || '' }}

When _DRY_RUN="", the shell command becomes:

python3 .github/scripts/label-pr.py "$_PR_NUMBER" "$_LABEL_MODE" ""

The empty string is passed as a positional argument, which argparse treats as unknown.

Recommendation: Fix at the workflow level instead of working around in Python:

run: |
  if [ -n "$_DRY_RUN" ]; then
    python3 .github/scripts/label-pr.py "$_PR_NUMBER" "$_LABEL_MODE" "$_DRY_RUN"
  else
    python3 .github/scripts/label-pr.py "$_PR_NUMBER" "$_LABEL_MODE"
  fi

This eliminates the need for parse_known_args() and makes the interface cleaner.

return args

def main():
args = parse_args()
config = load_config_json(args.config)
CATCH_ALL_LABEL = config["catch_all_label"]
LABEL_TITLE_PATTERNS = config["title_patterns"]
LABEL_PATH_PATTERNS = config["path_patterns"]

pr_number = args.pr_number
mode = "replace" if args.replace else "add"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

โš ๏ธ Mode default behavior inconsistent with documentation

Details

The implementation defaults to "add" when neither flag is provided:

mode = "replace" if args.replace else "add"

However, the documentation at lines 11-12 states:

-a, --add: Add labels without removing existing ones (default)
-r, --replace: Replace all existing labels

This creates confusion about actual vs documented behavior. The workflow at .github/workflows/sdlc-label-pr.yml:68-69 also uses --replace as the default for normal PRs, suggesting replace should be the default.

Recommendation:
Update documentation to reflect actual behavior:

-a, --add: Add labels without removing existing ones
-r, --replace: Replace all existing labels (default for normal PRs)

Or update DEFAULT_MODE constant at line 30 to match documentation.


if args.dry_run:
print("๐Ÿ” DRY RUN MODE - Labels will not be applied")
print(f"๐Ÿ“Œ Labeling mode: {mode}")
print(f"๐Ÿ” Checking PR #{pr_number}...")

pr_title = gh_get_pr_title(pr_number)
print(f"๐Ÿ“‹ PR Title: {pr_title}\n")

changed_files = gh_get_changed_files(pr_number)
print("๐Ÿ‘€ Changed files:\n" + "\n".join(changed_files) + "\n")

filepath_labels = label_filepaths(changed_files, LABEL_PATH_PATTERNS)
title_labels = label_title(pr_title, LABEL_TITLE_PATTERNS)
all_labels = set(filepath_labels + title_labels)

if not any(label.startswith("t:") for label in all_labels):
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

๐Ÿ’ญ Catch-all label logic may produce unexpected results

Details

The catch-all label (t:misc) is added only when there are no t: prefixed labels:

if not any(label.startswith("t:") for label in all_labels):
    all_labels.add(CATCH_ALL_LABEL)

Scenario: A PR that:

  • Modifies app/MainActivity.kt โ†’ gets app:password-manager
  • Has title "Update MainActivity layout" โ†’ no title pattern match

Result: Only app:password-manager label, no type label. Then t:misc is added.

Question: Should the catch-all only apply when both filepath and title labeling fail to produce type labels, or is the current behavior (adding t:misc whenever no type label exists) correct?

Consider if this edge case needs explicit documentation or if the logic should check:

# Only add catch-all if BOTH filepath and title labeling found no type labels
if not title_labels and not any(label.startswith("t:") for label in filepath_labels):
    all_labels.add(CATCH_ALL_LABEL)

Current behavior seems reasonable, but worth confirming intent.

all_labels.add(CATCH_ALL_LABEL)

if all_labels:
labels_str = ', '.join(sorted(all_labels))
if mode == "add":
print(f"๐Ÿท๏ธ Adding labels: {labels_str}")
if not args.dry_run:
gh_add_labels(pr_number, list(all_labels))
else:
print(f"๐Ÿท๏ธ Replacing labels with: {labels_str}")
if not args.dry_run:
gh_replace_labels(pr_number, list(all_labels))
else:
print("โ„น๏ธ No matching patterns found, no labels applied.")

print("โœ… Done")

if __name__ == "__main__":
main()
80 changes: 80 additions & 0 deletions .github/workflows/sdlc-label-pr.yml
Original file line number Diff line number Diff line change
@@ -0,0 +1,80 @@
name: SDLC / Label PR by Files

on:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

๐Ÿ’ญ Workflow only supports manual dispatch - automated triggers not yet configured.

Details

The workflow currently only triggers via workflow_dispatch, requiring manual execution. The PR description mentions:

pull_request workflow target will be added in a future PR after testing with workflow_dispatch.

This is appropriate for initial testing. When adding automated triggers, consider:

on:
  pull_request:
    types: [opened, synchronize, reopened]
  pull_request_target:  # For fork PRs with write permissions
    types: [opened, synchronize, reopened]
  workflow_dispatch:
    # ... keep existing inputs for manual testing

Security consideration: When adding pull_request_target, ensure the workflow checks out the base branch (not PR branch) before running any code from the PR, or validate that the Python script execution is safe for untrusted PRs.

Current implementation is safe for testing phase.

workflow_dispatch:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

๐Ÿ’ญ Workflow only supports manual dispatch - PR events not yet configured

Details

The workflow currently only triggers via workflow_dispatch, requiring manual execution. The PR description mentions:

pull_request workflow target will be added in a future PR after testing with workflow_dispatch.

This is fine for initial testing, but worth noting that automated labeling won't occur until the trigger is updated to something like:

on:
  pull_request:
    types: [opened, synchronize, reopened]
  pull_request_target:  # For fork PRs
    types: [opened, synchronize, reopened]
  workflow_dispatch:
    # ... keep existing inputs for manual testing

Security consideration: When adding pull_request_target, ensure the workflow checks out the base branch (not PR branch) before running untrusted code, or limit to trusted users only.

Current implementation is safe for testing phase. Just flagging for future work.

inputs:
pr-number:
description: "Pull Request Number"
required: true
type: number
mode:
description: "Labeling Mode"
type: choice
options:
- add
- replace
default: add
dry-run:
description: "Dry Run - Don't apply labels"
type: boolean
default: false

jobs:
label-pr:
name: Label PR by Changed Files
runs-on: ubuntu-24.04
permissions:
pull-requests: write # required to update labels
contents: read

steps:
- name: Check out repository
uses: actions/checkout@08c6903cd8c0fde910a37f88322edcfb5dd907a8 # v5.0.0
with:
persist-credentials: false

- name: Determine label mode for Pull Request
id: label-mode
env:
GH_TOKEN: ${{ github.token }}
_PR_NUMBER: ${{ inputs.pr-number }}
_PR_USER: ${{ github.event.pull_request.user.login }}
_IS_FORK: ${{ github.event.pull_request.head.repo.fork }}
run: |
# Support workflow_dispatch testing by retrieving PR data
if [ -z "$_PR_USER" ]; then
echo "๐Ÿ‘€ PR User is empty, retrieving PR data for PR #$_PR_NUMBER..."
PR_DATA=$(gh pr view "$_PR_NUMBER" --json author,isCrossRepository)
_PR_USER=$(echo "$PR_DATA" | jq -r '.author.login')
_IS_FORK=$(echo "$PR_DATA" | jq -r '.isCrossRepository')
fi
echo "๐Ÿ“‹ PR User: $_PR_USER"
echo "๐Ÿ“‹ Is Fork: $_IS_FORK"
# Handle PRs with labels set by other automations by adding instead of replacing
if [ "$_IS_FORK" = "true" ]; then
echo "โžก๏ธ Fork PR ($_PR_USER). Label mode: --add"
echo "label_mode=--add" >> "$GITHUB_OUTPUT"
exit 0
fi
if [ "$_PR_USER" = "renovate[bot]" ] || [ "$_PR_USER" = "bw-ghapp[bot]" ]; then
echo "โžก๏ธ Bot PR ($_PR_USER). Label mode: --add"
echo "label_mode=--add" >> "$GITHUB_OUTPUT"
exit 0
fi
echo "โžก๏ธ Normal PR. Label mode: --replace"
echo "label_mode=--replace" >> "$GITHUB_OUTPUT"
- name: Label PR based on changed files
env:
GH_TOKEN: ${{ github.token }}
_PR_NUMBER: ${{ inputs.pr-number || github.event.pull_request.number }}
_LABEL_MODE: ${{ inputs.mode && format('--{0}', inputs.mode) || steps.label-mode.outputs.label_mode }}
_DRY_RUN: ${{ inputs.dry-run == true && '--dry-run' || '' }}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

โš ๏ธ Empty string passed as positional argument when dry-run is false

Details

When inputs.dry-run is false, the _DRY_RUN variable is set to an empty string '':

_DRY_RUN: ${{ inputs.dry-run == true && '--dry-run' || '' }}

This empty string is then passed as a positional argument to the Python script:

python3 .github/scripts/label-pr.py "$_PR_NUMBER" "$_LABEL_MODE" "$_DRY_RUN"

When _DRY_RUN="", the shell expands this to:

python3 .github/scripts/label-pr.py "1234" "--replace" ""

The Python script receives an empty string as the third argument, which argparse treats as an unknown argument. The script handles this with parse_known_args() at line 191, but this is a workaround for a shell quoting issue.

Recommendation:
Only pass _DRY_RUN if it's not empty:

- name: Label PR based on changed files
  env:
    GH_TOKEN: ${{ github.token }}
    _PR_NUMBER: ${{ inputs.pr-number || github.event.pull_request.number }}
    _LABEL_MODE: ${{ inputs.mode && format('--{0}', inputs.mode) || steps.label-mode.outputs.label_mode }}
    _DRY_RUN: ${{ inputs.dry-run == true && '--dry-run' || '' }}
  run: |
    if [ -n "$_DRY_RUN" ]; then
      python3 .github/scripts/label-pr.py "$_PR_NUMBER" "$_LABEL_MODE" "$_DRY_RUN"
    else
      python3 .github/scripts/label-pr.py "$_PR_NUMBER" "$_LABEL_MODE"
    fi

This eliminates the need for the parse_known_args() workaround.

run: |
echo "๐Ÿ” Labeling PR #$_PR_NUMBER with mode: $_LABEL_MODE and dry-run: $_DRY_RUN"
python3 .github/scripts/label-pr.py "$_PR_NUMBER" "$_LABEL_MODE" "$_DRY_RUN"
1 change: 0 additions & 1 deletion .husky/pre-commit

This file was deleted.

Loading