ci: use shared PR validation action from shared-ai-standards#15145
ci: use shared PR validation action from shared-ai-standards#15145andreiships-bot wants to merge 4 commits intoanomalyco:devfrom
Conversation
|
Hey! Your PR title Please update it to start with one of:
Where See CONTRIBUTING.md for details. |
|
This PR doesn't fully meet our contributing guidelines and PR template. What needs to be fixed:
Please edit this PR description to address the above within 2 hours, or it will be automatically closed. If you believe this was flagged incorrectly, please let a maintainer know. |
andreiships-bot
left a comment
There was a problem hiding this comment.
Clean extraction of the title regex into the shared action. The c.body?.includes null-safety fix is correct. Three items worth addressing before merge:
| id: title-check | ||
| continue-on-error: true | ||
| uses: andreiships/shared-ai-standards/.github/actions/pr-validation@main | ||
| with: |
There was a problem hiding this comment.
Behavior change: regex is now stricter.
The old local regex was:
/^(feat|fix|...)\s*(\([a-zA-Z0-9-]+\))?\s*:/
The shared action's default is:
/^(type)(\([\w-]+\))?!?:\s+.+$/
Differences:
- Requires a non-empty subject after
:(old allowed barefeat:) - Disallows space before colon (old allowed
feat :) - Adds
!(breaking-change marker) support \win scope also matches digits/underscore, while old was[a-zA-Z0-9-]
This is a good tightening but is an undocumented behavior change for contributors. Consider adding a comment here noting the regex change, or a CHANGELOG entry.
.github/workflows/pr-standards.yml
Outdated
| - name: Validate PR Title | ||
| id: title-check | ||
| continue-on-error: true | ||
| uses: andreiships/shared-ai-standards/.github/actions/pr-validation@main |
There was a problem hiding this comment.
skip-users input is unused — dual maintenance risk.
The shared action accepts a skip-users input that performs the same exclusion as the job-level if condition. Currently, the skip list (andreiships, opencode-agent[bot]) is maintained only in the if condition. If a future maintainer adds a user to skip-users: here without also updating the if, the job will still run but the action will skip internally — silently diverging.
Recommendation: either document that skip-users is intentionally unused (the job-level if is the single source of truth), or migrate to skip-users input and remove the job-level condition.
|
|
||
| See [CONTRIBUTING.md](../blob/dev/CONTRIBUTING.md#pr-titles) for details.`); | ||
| core.setFailed('PR title does not follow conventional commit format'); | ||
| return; |
There was a problem hiding this comment.
core.setFailed() step name is misleading on failure.
When title validation fails, the step that appears failed in the GitHub Actions UI will be Manage PR labels and check linked issue, not something that says PR title invalid. This is confusing for PR authors trying to understand why their PR check failed.
The shared action step name Validate PR Title would have shown the failure clearly if continue-on-error were not set. Consider renaming this step to Validate PR and manage labels or similar so the failure surface is more descriptive.
Replace inline title regex check with shared `andreiships/shared-ai-standards/.github/actions/pr-validation@main` composite action. Linked issue check and label management remain local.
6d0418d to
7f6d297
Compare
Gemini Deep ReviewSummaryThis PR refactors the PR standards workflow to use a shared GitHub action for title validation, which is a good improvement for maintainability. However, it also introduces an undocumented policy change requiring Findings[gemini-1] issue: P1 | .github/workflows/pr-standards.yml:110 | The logic for skipping linked issue checks was changed, but the PR description doesn't mention this new policy. Previously, [gemini-2] nit: P2 | .github/workflows/pr-standards.yml:109 | The comment describing which PR types skip the issue check is now out of sync with the code. The comment says PR MetadataSuggested Description Update: Please add a section to the description explaining that this PR also introduces a new requirement for Questions
Recommendation[ ] Approve | [X] Approve with changes | [ ] Request changes |
Codex ReviewSummaryThe PR is small and focused (<400 changed lines) and the intent is good, but it introduces a high-risk CI security change and one behavior regression risk. Requesting changes. Findings[FINDING-1] blocking: P0 | pr-standards.yml:17 | [FINDING-2] issue: P1 | pr-standards.yml:53 | Title validity is inferred from [FINDING-3] nit: P2 | pr-standards.yml:122 | Log text says docs/refactor/feat are skipped, but regex now skips only docs/refactor. Code Quality
Architecture
PR MetadataSuggested PR Title: Recommendation[ ] Approve | [ ] Approve with changes | [x] Request changes Escalate to Gemini?[x] Yes - Security-sensitive CI workflow change ( |
Resolution SummaryResolving findings from Codex Review, Gemini Review:
|
Reviewer Question ResponseQ (Gemini): The change to require linked issues for A: Yes, this is intentional. The |
Codex ReviewSummaryTwo of the prior findings are fixed (immutable SHA pin and corrected skip log text). One blocker remains: title-validation runtime failures are still treated as invalid titles, so reviewer guidance can still be wrong. Findings[FINDING-1] issue: P1 | /private/var/folders/z5/6g4cdcjx0054hsw8p848t4k40000gn/T/cr-anomalyco-opencode-02a78b1/.github/workflows/pr-standards.yml:53 | Previous finding not fully addressed: Code Quality
Architecture
PR MetadataSuggested PR Title: Recommendation[ ] Approve | [ ] Approve with changes | [x] Request changes Escalate to Gemini?[ ] Yes - [reason] | [x] No |
Gemini Deep ReviewSummaryThis PR refactors the PR standards workflow to use a shared GitHub action for title validation and tightens the policy around linked issues. While the move to a shared action is a good goal, using an action from a personal repository introduces a security risk. Additionally, a significant change to the contribution policy for Findings[gemini-1] blocking: P0 | .github/workflows/pr-standards.yml:18 | The workflow uses a GitHub Action from a personal, unverified repository ( [gemini-2] issue: P1 | .github/workflows/pr-standards.yml:115 | This PR changes the contribution policy by requiring PR MetadataSuggested Description Update: Questions
Recommendation[ ] Approve | [ ] Approve with changes | [X] Request changes |
Resolution SummaryResolving findings from Codex Review, Gemini Review:
|
Reviewer Question Response (Round 2)Q (Gemini): Is there a plan to move the shared action to an organization-owned repository? A: The shared action currently lives at — is a GitHub organization (not a personal account), so it is already organization-owned. The action is pinned to a specific commit SHA () to prevent mutable-ref supply chain attacks. For this PR's scope, the current setup is sufficient. A future improvement could be forking or vendoring the action into if the project wants to eliminate the external dependency entirely. |
Dual-Review Summary (Round 2)
Convergence: ❌ Not achieved (3 blocking findings) Rounds: 2 Findings[Codex #1] P1 -
[Gemini #1] P0 -
[Gemini #2] P1 -
|
Summary
Replace inline title regex check with the shared
andreiships/shared-ai-standards/.github/actions/pr-validation@maincomposite action. Linked issue check and label management (needs:title,needs:issue) remain as local steps.Changes
Validate PR Titlestep using shared composite action (pinned to SHA) withcontinue-on-error: truec.body.includes()→c.body?.includes()(optional chaining)ifcondition for upstream maintainer skip listpull_request_targettrigger (fork security)Policy change:
featPRs now require a linked issuePreviously,
feat,docs, andrefactorPRs were exempt from the linked-issue requirement. This PR removesfeatfrom the exemption list —featPRs must now reference an issue viaFixes #NorCloses #N.Rationale: Features should always trace to a tracked issue. The
featexemption was unintentional (added in the fork's history and not present in upstream). This aligns with the upstream policy.Regex change: stricter title validation
The shared action uses a stricter regex than the removed local one:
:(e.g.,feat:alone is now rejected)feat :is rejected)!breaking-change marker support (e.g.,feat!: major changeis now valid)Test Plan
needs:titlelabel + commentneeds:titlelabelneeds:issuelabel + commentif)Related