ci: add path-based conditional check execution#2673
Conversation
|
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
![pullfrog[bot]](https://avatars.githubusercontent.com/in/1768019?s=60&v=4){kind=small}
There was a problem hiding this comment.
The path-based filtering approach is sound and the implementation is straightforward. There are a few issues worth addressing — the most significant being missing path coverage and service container waste.
| filters: | | ||
| backend: | ||
| - 'agents-api/**' | ||
| - 'packages/**' | ||
| - 'agents-cli/**' | ||
| - 'agents-work-apps/**' | ||
| frontend: | ||
| - 'agents-manage-ui/**' | ||
| docs: | ||
| - 'agents-docs/**' | ||
| lockfile: | ||
| - 'pnpm-lock.yaml' |
There was a problem hiding this comment.
Missing paths in filters. Changes to .github/workflows/ci.yml, .github/composite-actions/**, or root config files (turbo.json, tsconfig.json, biome.json) won't trigger the E2E jobs. If someone modifies the CI workflow itself or a composite action used by this job, the tests silently skip. Consider adding a ci or infra filter that covers .github/** to the condition, or at minimum add the workflow file and composite actions directory to the backend filter.
Same applies to the cypress.yml changes job — changes to .github/workflows/cypress.yml or .github/composite-actions/cypress-e2e/ wouldn't trigger Cypress tests.
| fi | ||
|
|
||
| create-agents-e2e: | ||
| needs: [changes] |
There was a problem hiding this comment.
Service containers still provision when steps are skipped. GitHub Actions spins up services: at job start, before steps execute. When should_run is false, the doltgres and postgres containers (and the ubuntu-32gb runner) are still allocated — you just don't use them.
I understand the step-level if approach is intentional so the job still reports a passing check (satisfying required status checks). But paying for an ubuntu-32gb runner + 2 database containers to run zero steps is a meaningful cost.
Alternative: use a job-level if and configure branch protection to not require these checks, or use dorny/paths-filter's status check integration. Another option is a separate lightweight "gate" job that downstream required-check tooling references.
| - uses: actions/checkout@v4 | ||
| - uses: dorny/paths-filter@v3 |
There was a problem hiding this comment.
Unpinned actions. Uses actions/checkout@v4 (floating tag) while the rest of the workflow pins to a SHA (actions/checkout@de0fac2e4500dabe0009e67214ff5f5447ce83dd # v6). Pin both actions/checkout and dorny/paths-filter to specific SHAs for supply-chain safety and consistency. Also note this is v4 vs v6 used elsewhere.
| filters: | | ||
| frontend: | ||
| - 'agents-manage-ui/**' | ||
| lockfile: | ||
| - 'pnpm-lock.yaml' |
There was a problem hiding this comment.
Frontend filter may be too narrow. The Cypress E2E tests need the full backend running (the composite action builds and starts the API). Changes in packages/** — shared types, core library, validation schemas — flow into agents-manage-ui but wouldn't trigger these tests. Consider adding packages/** here, or at minimum the packages that the UI directly depends on.
| - uses: actions/checkout@v4 | ||
| - uses: dorny/paths-filter@v3 |
There was a problem hiding this comment.
Same pinning issue as ci.yml — pin actions/checkout and dorny/paths-filter to specific SHAs.
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
PR Review Summary
(7) Total Issues | Risk: Medium
🟠⚠️ Major (2) 🟠⚠️
Inline Comments:
- 🟠 Major:
.github/workflows/ci.yml:37—github.event.beforeis undefined formerge_groupevents, causing incorrect path comparison - 🟠 Major:
.github/workflows/cypress.yml:31— Samemerge_groupbase SHA issue
🟡 Minor (2) 🟡
🟡 1) create-agents-template/**, agents-cookbook/template-projects/** Template paths not included in backend filter
Issue: The create-agents-e2e tests use create-agents-template/ and agents-cookbook/template-projects/ for scaffolding, but these paths aren't included in the backend filter. Changes to templates won't trigger E2E tests.
Why: If someone modifies the quickstart template (e.g., changing dependencies or configuration), the E2E tests that validate template scaffolding won't run, potentially allowing broken templates to merge.
Fix: Consider adding to the backend filter:
backend:
- 'agents-api/**'
- 'packages/**'
- 'agents-cli/**'
- 'agents-work-apps/**'
- 'create-agents-template/**'
- 'agents-cookbook/template-projects/**'Refs:
- quickstart.test.ts:45-46 — test uses these directories
🟡 2) scope No skip indication in job summary for early-exit runs
Issue: When E2E steps are skipped due to path filtering, the job completes successfully but there's no visible indication distinguishing "tests passed" from "tests were skipped." Developers may assume tests ran when they didn't.
Why: This could mask regressions if a developer sees a green check and assumes their changes were validated when the tests actually early-exited.
Fix: Add a conditional step that runs when should_run == 'false' to output a notice:
- name: Skip notice
if: steps.should-run.outputs.should_run != 'true'
run: echo "::notice::Skipped E2E tests - no relevant path changes detected"Refs:
🕐 Pending Recommendations (5)
Issues already raised by prior reviewers. Links to original threads:
- 🟠
.github/workflows/ci.yml:33— Unpinned actions (actions/checkout@v4,dorny/paths-filter@v3) should use SHA pinning for supply-chain safety - 🟠
.github/workflows/cypress.yml:27— Same pinning issue in cypress.yml - 🟠
.github/workflows/ci.yml:49— Missing paths in filters (workflow files, composite actions, root config) - 🟡
.github/workflows/cypress.yml:36— Frontend filter may be too narrow (Cypress tests exercise full stack, consider addingpackages/**) - 🟡
.github/workflows/ci.yml:226— Service containers still provision when steps are skipped (known trade-off for maintaining required status checks)
🚫 REQUEST CHANGES
Summary: The path-based filtering approach is sound and will meaningfully reduce CI costs for docs-only and scoped PRs. However, the merge_group event handling has a bug (github.event.before is undefined for merge queue runs) that could cause incorrect test skipping in the merge queue. This should be fixed before merge. The pending recommendations from the prior review (SHA pinning, path coverage) should also be addressed.
Discarded (5)
| Location | Issue | Reason Discarded |
|---|---|---|
ci.yml:231-259 |
Service containers run even when skipped | Intentional trade-off documented in PR; already raised by prior reviewer |
cypress.yml:45-73 |
Same service container issue | Same as above |
multi-file |
Duplicate filter definitions could drift | Acceptable duplication for 2 files; extracting to reusable workflow adds complexity |
ci.yml:38-49 |
agents-work-apps not in frontend filter | agents-work-apps is backend, current categorization is correct |
multi-file |
Filter definitions not DRY | Same as duplicate filter finding above |
Reviewers (3)
| Reviewer | Returned | Main Findings | Consider | While You're Here | Inline Comments | Pending Recs | Discarded |
|---|---|---|---|---|---|---|---|
pr-review-devops |
8 | 1 | 0 | 0 | 0 | 4 | 3 |
pr-review-sre |
6 | 1 | 0 | 0 | 2 | 1 | 2 |
pr-review-consistency |
4 | 0 | 0 | 0 | 0 | 2 | 2 |
| Total | 18 | 2 | 0 | 0 | 2 | 5 | 5 |
Note: Multiple reviewers identified overlapping issues (SHA pinning, service container waste) which were deduplicated and routed to Pending Recommendations since they were already raised by prior review.
| id: filter | ||
| with: | ||
| # For push/merge_group events, compare against parent commit | ||
| base: ${{ (github.event_name == 'push' || github.event_name == 'merge_group') && github.event.before || '' }} |
There was a problem hiding this comment.
🟠 MAJOR: github.event.before is undefined for merge_group events
Issue: The base parameter uses github.event.before for merge queue events, but merge_group events don't populate this field — they use github.event.merge_group.base_sha instead.
Why: When the merge queue runs, github.event.before evaluates to empty/undefined, causing dorny/paths-filter to potentially compare against the wrong base or fall back to comparing all files. This could result in tests being skipped when they should run, or vice versa.
Fix:
| base: ${{ (github.event_name == 'push' || github.event_name == 'merge_group') && github.event.before || '' }} | |
| base: ${{ github.event_name == 'push' && github.event.before || github.event_name == 'merge_group' && github.event.merge_group.base_sha || '' }} |
Refs:
| id: filter | ||
| with: | ||
| # For push/merge_group events, compare against parent commit | ||
| base: ${{ (github.event_name == 'push' || github.event_name == 'merge_group') && github.event.before || '' }} |
There was a problem hiding this comment.
🟠 MAJOR: Same merge_group base SHA issue
Issue: Same issue as ci.yml — github.event.before is undefined for merge_group events.
Fix:
| base: ${{ (github.event_name == 'push' || github.event_name == 'merge_group') && github.event.before || '' }} | |
| base: ${{ github.event_name == 'push' && github.event.before || github.event_name == 'merge_group' && github.event.merge_group.base_sha || '' }} |
Refs:
Skip expensive E2E jobs (create-agents-e2e, cypress-e2e) when irrelevant paths haven't changed, using dorny/paths-filter. Combines with existing changeset-check early-exit pattern so both conditions are evaluated in a single should-run step. - create-agents-e2e skips when no backend or lockfile changes - cypress-e2e skips when no frontend or lockfile changes - ci job unchanged (turbo handles its own caching) Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
amikofalvy
force-pushed
the
prd-6283-path-based-ci
branch
from
a085f12 to
5aef88e
Compare
There was a problem hiding this comment.
PR Review Summary
(10) Total Issues | Risk: Medium
🟠⚠️ Major (2) 🟠⚠️
These issues were identified in prior reviews and remain unresolved. See Pending Recommendations below for links.
The two blocking Major issues are:
merge_groupbase SHA handling —github.event.beforeis undefined for merge queue events- Unpinned third-party actions —
dorny/paths-filter@v3andactions/checkout@v4should use SHA pinning
🟡 Minor (2) 🟡
Inline Comments:
- 🟡 Minor:
.github/workflows/ci.yml:22— Missing timeout on path detection job - 🟡 Minor:
.github/workflows/cypress.yml:18— Missing timeout on path detection job
🕐 Pending Recommendations (8)
Issues raised in prior reviews that remain unresolved:
Critical/Major:
- 🟠
.github/workflows/ci.yml:37—github.event.beforeis undefined formerge_groupevents, causing incorrect path comparison - 🟠
.github/workflows/cypress.yml:31— Samemerge_groupbase SHA issue - 🟠
.github/workflows/ci.yml:33— Unpinned actions (actions/checkout@v4,dorny/paths-filter@v3) should use SHA pinning - 🟠
.github/workflows/cypress.yml:27— Same pinning issue in cypress.yml
Minor:
- 🟡
.github/workflows/ci.yml:49— Missing paths in filters (workflow files, composite actions, root config) - 🟡
.github/workflows/cypress.yml:36— Frontend filter may be too narrow (consider addingpackages/**) - 🟡
template paths— Template paths (create-agents-template/**) not included in backend filter - 🟡
.github/workflows/ci.yml:226— Service containers still provision when steps are skipped (known trade-off)
🚫 REQUEST CHANGES
Summary: The path-based conditional execution approach is sound and will meaningfully reduce CI costs for docs-only and scoped PRs. However, the merge_group event handling has a critical bug (github.event.before is undefined for merge queue runs) that could cause incorrect test skipping in the merge queue. Additionally, the third-party actions should be SHA-pinned per repository conventions. These issues were identified in the prior review and remain unresolved in this commit — please address them before merge.
Discarded (6)
| Location | Issue | Reason Discarded |
|---|---|---|
ci.yml:21-50 |
Service containers start regardless of filter | Duplicate of pullfrog finding; routed to Pending |
ci.yml:39-44 |
Path filter missing workflow changes | Duplicate of pullfrog finding; routed to Pending |
cypress.yml:33-35 |
Path filter missing Cypress config | Duplicate of pullfrog finding; routed to Pending |
multi-file |
Step ID naming divergence | INFO level; intentional design difference between ci job and E2E jobs |
ci.yml:32 |
Inconsistent checkout version | Subset of SHA pinning finding; already in Pending |
multi-file |
Action versions use tags | Subset of SHA pinning finding; already in Pending |
Reviewers (3)
| Reviewer | Returned | Main Findings | Consider | While You're Here | Inline Comments | Pending Recs | Discarded |
|---|---|---|---|---|---|---|---|
pr-review-devops |
7 | 0 | 0 | 0 | 0 | 5 | 2 |
pr-review-sre |
5 | 0 | 0 | 0 | 2 | 2 | 1 |
pr-review-consistency |
4 | 0 | 0 | 0 | 0 | 1 | 3 |
| Total | 16 | 0 | 0 | 0 | 2 | 8 | 6 |
Note: Most findings were already raised in prior reviews (pullfrog, claude) and routed to Pending Recommendations to avoid duplication.
|
|
||
| jobs: | ||
| changes: | ||
| runs-on: ubuntu-latest |
There was a problem hiding this comment.
🟡 Minor: Missing timeout on path detection job
Issue: The changes job has no timeout-minutes specified. While it uses ubuntu-latest (not the expensive 32gb runner) and should complete quickly, a hung paths-filter action could block dependent jobs indefinitely.
Why: The E2E jobs would wait forever for the changes job outputs, blocking the CI pipeline. This is a low-probability scenario but easy to guard against.
Fix: Add a reasonable timeout:
| runs-on: ubuntu-latest | |
| changes: | |
| runs-on: ubuntu-latest | |
| timeout-minutes: 5 |
Refs:
|
|
||
| jobs: | ||
| changes: | ||
| runs-on: ubuntu-latest |
There was a problem hiding this comment.
🟡 Minor: Missing timeout on path detection job
Issue: Same as ci.yml — the changes job should have an explicit timeout to prevent indefinite blocking if the path filter action hangs.
Fix: Add a reasonable timeout:
| runs-on: ubuntu-latest | |
| changes: | |
| runs-on: ubuntu-latest | |
| timeout-minutes: 5 |
Refs:
|
This pull request has been automatically marked as stale because it has not had recent activity. If this PR is still relevant:
Thank you for your contributions! |
|
This pull request has been automatically closed due to inactivity. If you'd like to continue working on this, please:
Thank you for your understanding! |
1 participant
Summary
dorny/paths-filter@v3to detect which files changed in each PRcreate-agents-e2ejob now early-exits when no backend (agents-api/,packages/,agents-cli/,agents-work-apps/) or lockfile changes are detectedcypress-e2ejob now early-exits when no frontend (agents-manage-ui/) or lockfile changes are detectedTest plan
create-agents-e2eandcypress-e2epass quickly (early exit)create-agents-e2eruns fully,cypress-e2eexits earlycypress-e2eruns fully,create-agents-e2eexits earlypnpm-lock.yaml→ verify both E2E jobs run fullycijob always runs fully regardless of paths changed🤖 Generated with Claude Code