Skip to content

ci: allow benchmark/ branch name prefix#258

Merged
carlos-alm merged 1 commit intomainfrom
ci/allow-benchmark-branch
Mar 3, 2026
Merged

ci: allow benchmark/ branch name prefix#258
carlos-alm merged 1 commit intomainfrom
ci/allow-benchmark-branch

Conversation

@carlos-alm
Copy link
Contributor

Summary

Test plan

@carlos-alm
Copy link
Contributor Author

I have read the CLA Document and I hereby sign the CLA

@claude
Copy link

claude bot commented Mar 3, 2026

Claude finished @carlos-alm's task —— View job


PR Review: ci: allow benchmark/ branch name prefix

Todo List

  • Read and analyze the changed file (.github/workflows/commitlint.yml)
  • Understand the current branch naming validation rules
  • Assess root cause and justification for the change
  • Provide comprehensive review feedback

Root Cause Analysis

Problem Being Solved: The automated benchmark workflow in .github/workflows/benchmark.yml creates branches with benchmark/ prefixes (like benchmark/build-dev-20260303-142233, benchmark/embedding-v2.4.0-20260303, etc.), but the commitlint workflow rejects these branch names because benchmark/ wasn't in the allowed prefixes list.

Why This Approach: This is a legitimate fix addressing a real infrastructure issue. The benchmark workflow has been creating benchmark/ prefixed branches for automated performance testing, but the branch name validation was preventing these legitimate automation branches from being processed.

Risk Assessment: LOW RISK - This is adding a specific, well-scoped branch prefix that aligns with existing automation patterns. The benchmark/ prefix is:

  1. Already used by existing automation (8 instances in benchmark.yml)
  2. Follows the same pattern as other automation prefixes like dependabot/
  3. Semantically appropriate for performance testing branches

Critical Concerns

None identified - This is a straightforward infrastructure fix.

Technical Analysis

.github/workflows/commitlint.yml:35-38: The change correctly:

  • ✅ Adds benchmark to the regex pattern alongside other valid prefixes
  • ✅ Updates the error message to include the new prefix
  • ✅ Maintains alphabetical ordering in the error message
  • ✅ Uses consistent formatting and syntax

Evidence of Legitimate Use: The benchmark workflow creates 8 different benchmark/ branch patterns:

  • benchmark/build-dev-* and benchmark/build-v*-* (lines 133, 135)
  • benchmark/embedding-dev-* and benchmark/embedding-v*-* (lines 278, 280)
  • benchmark/query-dev-* and benchmark/query-v*-* (lines 409, 411)
  • benchmark/incremental-dev-* and benchmark/incremental-v*-* (lines 540, 542)

Alignment with Project Conventions: The CONTRIBUTING.md file lists allowed prefixes but is now outdated and should be updated to include benchmark/ for consistency.

Minor Suggestions

  1. Documentation sync: Update CONTRIBUTING.md:70-77 to include benchmark/ in the branch naming convention documentation
  2. Consider automation prefix grouping: The current list mixes feature prefixes (feat/, fix/) with automation prefixes (dependabot/, benchmark/) - consider documenting this distinction

Final Assessment

## Root Cause Analysis
**Problem Being Solved**: Automated benchmark workflow creates `benchmark/` prefixed branches that fail branch name validation
**Why This Approach**: Legitimate infrastructure fix - these are real automation branches that need to pass validation
**Risk Assessment**: LOW - Well-scoped addition that aligns with existing automation patterns

## Critical Concerns
None

## Final Recommendation
- Rating: ⭐⭐⭐⭐☆ (4/5)
- Action: APPROVE
- Reasoning: This is a clean, justified fix for a real infrastructure issue. The `benchmark/` prefix is already used by automation and belongs in the allowed list. The implementation is correct and low-risk.

Recommendation: Approve this PR. Consider updating CONTRIBUTING.md in a follow-up to keep documentation in sync.

@greptile-apps
Copy link
Contributor

greptile-apps bot commented Mar 3, 2026

Greptile Summary

Added benchmark/ to the list of allowed branch name prefixes in the commitlint workflow to fix failing branch validation checks on benchmark-related PRs. The change updates both the regex pattern and the error message to include the new prefix, maintaining consistency with existing validation rules.

Confidence Score: 5/5

  • This PR is safe to merge with no risks
  • Simple configuration change that adds a single branch prefix to an existing validation pattern. The change is syntactically correct, consistently applied in both the pattern and error message, and addresses a real issue with benchmark branches failing validation.
  • No files require special attention

Important Files Changed

Filename Overview
.github/workflows/commitlint.yml Added benchmark/ to allowed branch name prefixes in validation pattern and error message

Last reviewed commit: 6e2a927

Copy link
Contributor

@greptile-apps greptile-apps bot left a comment

Choose a reason for hiding this comment

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

1 file reviewed, no comments

Edit Code Review Agent Settings | Greptile

@carlos-alm carlos-alm merged commit 50bb447 into main Mar 3, 2026
18 checks passed
@carlos-alm carlos-alm deleted the ci/allow-benchmark-branch branch March 3, 2026 00:37
@github-actions github-actions bot locked and limited conversation to collaborators Mar 3, 2026
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant