exclude docs prs from e2e test check but make build check req#2213
exclude docs prs from e2e test check but make build check req#2213Gaurav-Narayan-Varma wants to merge 1 commit intomainfrom
Conversation
|
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
PR Review Summary
(2) Total Issues | Risk: Low
🟡 Minor (2) 🟡
Inline Comments:
- 🟡 Minor:
ci.yml:151Missing timeout-minutes onchangesjob - 🟡 Minor:
ci.yml:169Missing timeout-minutes ondocs-buildjob
💭 Consider (2) 💭
💭 1) ci.yml:157 Third-party action version pinning
Issue: The dorny/paths-filter@v3 action is pinned to a mutable tag rather than a SHA.
Why: SHA pinning protects against supply chain attacks where upstream repos are compromised. However, this repo consistently uses version tags for all actions (including pnpm/action-setup@v4, actions/cache@v4, etc.), so this is more of a repo-wide policy consideration than a PR-specific issue.
Fix: If the team wants to improve supply chain security, consider SHA-pinning third-party actions repo-wide. For this PR specifically, no change required to maintain consistency with existing patterns.
Refs:
💭 2) ci.yml:212 Branch protection compatibility with skipped jobs
Issue: When create-agents-e2e is skipped for docs-only PRs, the job reports as "skipped" rather than "success".
Why: If create-agents-e2e is configured as a required status check in branch protection rules, skipped jobs may block merge depending on GitHub's branch protection settings.
Fix: Verify branch protection rules handle skipped jobs correctly. Options if this becomes an issue:
- Configure branch protection to allow skipped jobs
- Use
paths-ignoreat workflow level in a separate file - Add a no-op success job that runs when e2e is skipped
Refs:
💡 APPROVE WITH SUGGESTIONS
Summary: Good optimization to avoid running expensive E2E tests for docs-only PRs. The path filtering logic is correct and the new docs-build job ensures docs still get validated. The two minor items (missing timeouts) are quick fixes that improve CI reliability. The consider items are lower priority but worth noting for operational awareness.
Discarded (2)
| Location | Issue | Reason Discarded |
|---|---|---|
ci.yml:156 |
actions/checkout not SHA-pinned | Pre-existing repo-wide pattern — all actions use version tags, not specific to this PR |
ci.yml:160-165 |
Filter logic for mixed PRs | Intended behavior — mixed PRs correctly trigger both e2e and docs-build |
Reviewers (1)
| Reviewer | Returned | Main Findings | Consider | While You're Here | Inline Comments | Pending Recs | Discarded |
|---|---|---|---|---|---|---|---|
pr-review-devops |
6 | 0 | 2 | 0 | 2 | 0 | 2 |
| Total | 6 | 0 | 2 | 0 | 2 | 0 | 2 |
| fi | ||
|
|
||
| changes: | ||
| runs-on: ubuntu-latest |
There was a problem hiding this comment.
🟡 Minor: Missing timeout-minutes on changes job
Issue: The new changes job lacks a timeout-minutes declaration. While this job is lightweight, adding an explicit timeout prevents runner hang scenarios.
Why: The existing ci job sets timeout-minutes: 30 (line 23). For consistency and CI reliability, new jobs should follow this pattern.
Fix: Add a short timeout since this job should complete quickly:
| runs-on: ubuntu-latest | |
| changes: | |
| runs-on: ubuntu-latest | |
| timeout-minutes: 5 |
Refs:
|
|
||
| docs-build: | ||
| runs-on: ubuntu-latest | ||
| name: Docs Build |
There was a problem hiding this comment.
🟡 Minor: Missing timeout-minutes on docs-build job
Issue: The new docs-build job lacks a timeout-minutes declaration.
Why: Docs builds can occasionally hang due to dependency issues or build tool problems. An explicit timeout ensures failed builds don't consume runner minutes indefinitely.
Fix: Add a reasonable timeout for a docs build:
| name: Docs Build | |
| name: Docs Build | |
| needs: changes | |
| timeout-minutes: 15 |
Refs:
1 participant
No description provided.