fix(ci): match all .changeset/ files for CI skip#2876
Conversation
|
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
TL;DR — Fixes the changeset-only CI skip logic to match all Key changes
Summary ｜ 4 files ｜ 5 commits ｜ base: Broader
|
There was a problem hiding this comment.
Two issues — one is an accidental file commit, the other is a stale SPEC that now contradicts the implementation.
Claude Opus ｜ 𝕏
| @@ -0,0 +1,289 @@ | |||
| --- | |||
There was a problem hiding this comment.
This is a generated local-review artifact (note the local:manual event, LOCAL review state, and stale SHA 7ad17dd). .claude/skills/pr-context/ is already in .gitignore but the .agents/skills/pr-context/ source directory isn't — so it slipped through.
Remove this file and add .agents/skills/pr-context/ to .gitignore alongside the existing .claude/ entry.
| - **Push to main:** Skip this check entirely — pushes to main should always run CI | ||
| - **Empty diff:** If `gh pr diff` returns nothing, do NOT skip CI (fail-safe) | ||
| - **Mixed changes:** If any file outside `.changeset/` is changed, run full CI | ||
| - **Only `.changeset/*.md` files skip CI:** Non-markdown files like `config.json` should still trigger CI |
There was a problem hiding this comment.
The SPEC still says .changeset/config.json changes should trigger CI, but the implementation now uses grep -v '^\.changeset/' which matches all files under .changeset/ — including config.json.
This contradicts acceptance criteria 5 (line 72: ".changeset/config.json changes are NOT treated as changeset-only"), test case 4 (line 81), and the code example on line 47 which uses grep -v '^\.changeset/.*\.md$'.
Since the intent of this PR is explicitly to widen the pattern, the SPEC should be updated to match: change the code sample, remove the config.json exception from edge cases/acceptance criteria, and update test case 4.
| fi | ||
|
|
||
| if [ -n "$CHANGED_FILES" ]; then | ||
| NON_CHANGESET=$(echo "$CHANGED_FILES" | grep -v '^\.changeset/' || true) |
There was a problem hiding this comment.
This pattern (grep -v '^\.changeset/') now matches any file under .changeset/, which is the stated intent. Worth noting that .changeset/config.json is a real configuration file — changes to it can affect how changesets are versioned and published. Skipping CI for config changes is a deliberate trade-off (low risk since the release workflow still runs on merge). Just flagging for awareness since this reverses the SPEC's original design choice.
There was a problem hiding this comment.
PR Review Summary
(2) Total Issues | Risk: Medium
🟠⚠️ Major (2) 🟠⚠️
Inline Comments:
- 🟠 Major:
specs/changeset-only-skip-ci/SPEC.md:72SPEC contradicts implementation — document statesconfig.jsonshould trigger CI but code skips ALL.changeset/files - 🟠 Major:
.agents/skills/pr-context/SKILL.md:25Auto-generated local review artifact accidentally committed
💭 Consider (1) 💭
💭 1) multi-file Extract duplicated changeset detection logic
Issue: The 22-line changeset-only detection block is copied identically across 3 locations: ci.yml (lines 61-82, 370-391) and cypress.yml (lines 79-100).
Why: Code duplication creates maintenance burden — any bug fix or behavior change must be applied to 3 locations. This PR demonstrates the risk since all 3 locations must stay in sync.
Fix: Consider extracting to a reusable composite action at .github/composite-actions/changeset-check/action.yml. The repo already uses composite actions (e.g., cypress-e2e/action.yml). This could be a follow-up improvement.
🚫 REQUEST CHANGES
Summary: The CI skip logic implementation looks correct and the error handling improvements are solid. However, two issues need attention before merge:
-
SPEC/implementation mismatch — The SPEC says
config.jsonchanges should trigger CI, but the implementation (intentionally) skips all.changeset/files. Either the SPEC needs updating or the implementation should be reverted to match the SPEC. -
Accidentally committed generated file — The
.agents/skills/pr-context/SKILL.mdfile contains auto-generated content from a local review session with embedded diffs and stale commit SHAs. This should be removed.
Reviewers (2)
| Reviewer | Returned | Main Findings | Consider | While You're Here | Inline Comments | Pending Recs | Discarded |
|---|---|---|---|---|---|---|---|
pr-review-devops |
6 | 0 | 1 | 0 | 2 | 0 | 3 |
pr-review-standards |
2 | 0 | 0 | 0 | 0 | 0 | 2 |
| Total | 8 | 0 | 1 | 0 | 2 | 0 | 5 |
Discarded (5)
| Location | Issue | Reason Discarded |
|---|---|---|
ci.yml:64-66 |
Error handling captures stderr to CHANGED_FILES | Not an issue — this is intentional and correct behavior for debugging |
auto-format.yml:136-139 |
Warning added when APP_TOKEN unavailable | Good defensive logging, not an issue |
pr-context/SKILL.md:1-289 |
File is generated artifact | Covered by Major inline comment |
SPEC.md:47 |
Pattern mismatch | Covered by Major inline comment |
SPEC.md:58 |
Edge case documentation mismatch | Covered by Major inline comment |
| 2. PRs that modify `.changeset/*.md` AND other files still run full CI | ||
| 3. The `changeset-release/main` branch detection continues to work as before | ||
| 4. Push events to main always run full CI | ||
| 5. `.changeset/config.json` changes are NOT treated as changeset-only |
There was a problem hiding this comment.
🟠 MAJOR: SPEC contradicts implementation
Issue: The SPEC states .changeset/config.json changes should NOT skip CI (line 72, also lines 47, 58), but the implementation uses grep -v '^\.changeset/' which matches all .changeset/ files including config.json.
Why: The PR description says this is intentional ("Restores the broader pattern to match all .changeset/ files (including config.json), per the original intent"). However, the SPEC document was not updated to reflect this decision change. Documentation drift creates confusion for future maintainers who will read the SPEC and expect different behavior.
Fix: Update the SPEC to match the implementation:
- Line 47: Change pattern from
'^\.changeset/.*\.md$'to'^\.changeset/' - Line 58: Remove "Non-markdown files like
config.jsonshould still trigger CI" - Line 72: Change to "All
.changeset/files (includingconfig.json) are treated as changeset-only" - Test case readme clarity #4 (line 81): Update to "PR with
.changeset/config.jsonchanged → CI skipped"
Refs:
| | **Author** | Andrew Mikofalvy | | ||
| | **Base** | `main` | | ||
| | **Repo** | inkeep/agents | | ||
| | **Head SHA** | `7ad17dd61c66ea12b129c2d1c72da22fd8797e49` | |
There was a problem hiding this comment.
🟠 MAJOR: Generated local review artifact accidentally committed
Issue: This file contains auto-generated content from a local review session, including:
- Specific commit SHA (
842304ff52ec4a5e62502c0eabe7c4fce4c78b42) - Embedded diff content from an earlier state of this PR
- References to "local review" mode that doesn't match the actual PR context
Why: Unlike other skills in .agents/skills/ which provide reusable guidance templates, this file contains ephemeral generated output. The content is already stale (references older commits) and will only become more so. Committing generated artifacts creates repository noise and confusion about what should be version-controlled.
Fix: Remove this file from the commit. If pr-context is intended as a skill, it should contain template/guidance content — not generated review output.
git rm .agents/skills/pr-context/SKILL.mdRefs:
- Compare to other skills like audience-impact/SKILL.md which contain reusable templates, not generated content
The review fixer narrowed the pattern to only .md files, but per user request all .changeset/ file changes (including config.json) should skip CI. Also keeps the improved error handling from the review fix. Co-Authored-By: Claude Opus 4.6 (1M context) <[email protected]>
Co-Authored-By: Claude Opus 4.6 (1M context) <[email protected]>
Co-Authored-By: Claude Opus 4.6 (1M context) <[email protected]>
Drop auto-format.yml and ci.yml App token warning changes that the review auto-fixer added — not part of the changeset-only skip feature. Co-Authored-By: Claude Opus 4.6 (1M context) <[email protected]>
225ecc2 to
d459ab7
Compare
There was a problem hiding this comment.
PR Review Summary
(5) Total Issues | Risk: Medium
🟠⚠️ Major (2) 🟠⚠️
Inline Comments:
- 🟠 Major:
SPEC.md:72Acceptance Criteria #5 contradicts Edge Cases (line 58) and implementation — saysconfig.jsonshould trigger CI - 🟠 Major:
SPEC.md:81Test Case #4 contradicts implementation — expectsconfig.jsonto trigger CI
🟡 Minor (3) 🟡
Inline Comments:
- 🟡 Minor:
SPEC.md:32Description mentions only.mdfiles but implementation matches all.changeset/files - 🟡 Minor:
SPEC.md:47Code example shows'^\.changeset/.*\.md$'pattern but implementation uses'^\.changeset/' - 🟡 Minor:
SPEC.md:68Acceptance Criteria #1 mentions only.mdfiles
💭 Consider (1) 💭
💭 1) multi-file Extract duplicated changeset detection logic
Issue: The 22-line changeset-only detection block is copied identically across 3 locations: ci.yml (lines 61-82, 368-389) and cypress.yml (lines 79-100).
Why: Code duplication creates maintenance burden — any bug fix or behavior change must be applied to 3 locations. This PR demonstrates the risk since all 3 locations must stay in sync.
Fix: Consider extracting to a reusable composite action at .github/composite-actions/changeset-check/action.yml. The repo already uses composite actions (e.g., cypress-e2e/action.yml). This could be a follow-up improvement.
✅ What's Good
- Error handling improvement — Capturing
gh pr difffailures with2>&1and emitting::warning::annotations is a solid fail-safe pattern - Shell logic is correct — The
grep -v '^\.changeset/'pattern, empty-check guards, and|| truefor grep are all properly implemented - PR_NUMBER env var — Correctly passed through for
pull_requestevents
🕐 Pending Recommendations (1)
- 🟠 Previous review flagged SPEC/implementation mismatch — partially addressed (line 58 updated) but lines 32, 47, 68, 72, 81 still contradict the implementation
📝 Note on Prior False Positive
The previous review flagged .agents/skills/pr-context/SKILL.md as accidentally committed. This was a false positive — that file exists on main branch and was not introduced by this PR. The changed files list correctly shows only 3 files: ci.yml, cypress.yml, and SPEC.md.
🚫 REQUEST CHANGES
Summary: The CI workflow changes (error handling, broader changeset pattern) are implemented correctly. However, the SPEC document has internal contradictions that need to be resolved before merge. The author updated line 58 to reflect the new "all .changeset/ files" behavior, but missed updating 5 other locations that still reference the old .md-only behavior. All fixes have 1-click suggestion blocks in the inline comments.
Reviewers (2)
| Reviewer | Returned | Main Findings | Consider | While You're Here | Inline Comments | Pending Recs | Discarded |
|---|---|---|---|---|---|---|---|
pr-review-devops |
2 | 0 | 1 | 0 | 5 | 0 | 0 |
pr-review-standards |
0 | 0 | 0 | 0 | 0 | 0 | 0 |
| Total | 2 | 0 | 1 | 0 | 5 | 0 | 0 |
Note: pr-review-standards returned no new findings — shell logic is correct, and SPEC issues were already covered by devops reviewer.
Discarded (1)
| Location | Issue | Reason Discarded |
|---|---|---|
.agents/skills/pr-context/SKILL.md |
Prior review flagged as accidentally committed | False positive — file exists on main branch, not introduced by this PR |
| 2. PRs that modify `.changeset/*.md` AND other files still run full CI | ||
| 3. The `changeset-release/main` branch detection continues to work as before | ||
| 4. Push events to main always run full CI | ||
| 5. `.changeset/config.json` changes are NOT treated as changeset-only |
There was a problem hiding this comment.
🟠 MAJOR: Acceptance Criteria contradicts Edge Cases and implementation
Issue: This line says config.json changes should NOT skip CI, but line 58 says "All .changeset/ files skip CI: Both .md changeset entries and config.json are changeset-only changes". The implementation (using grep -v '^\.changeset/') matches line 58's intent.
Why: Internal contradictions in specs create confusion about intended behavior. Future maintainers may "fix" the implementation to match this incorrect criterion.
Fix: (1-click apply)
| 5. `.changeset/config.json` changes are NOT treated as changeset-only | |
| 5. All `.changeset/` files (including `config.json`) ARE treated as changeset-only — CI skipped |
Refs:
| 1. **PR with only `.changeset/foo.md` added** -> CI skipped | ||
| 2. **PR with `.changeset/foo.md` + `src/index.ts` changed** -> CI runs | ||
| 3. **PR on `changeset-release/main` branch** -> CI skipped (existing behavior) | ||
| 4. **PR with `.changeset/config.json` changed** -> CI runs |
There was a problem hiding this comment.
🟠 MAJOR: Test case contradicts implementation
Issue: This test case expects config.json changes to trigger CI, but the implementation skips CI for all .changeset/ files.
Why: Test cases that contradict implementation will cause incorrect pass/fail judgments during manual testing and future CI validation.
Fix: (1-click apply)
| 4. **PR with `.changeset/config.json` changed** -> CI runs | |
| 4. **PR with `.changeset/config.json` changed** -> CI skipped |
Refs:
|
|
||
| Then check: | ||
| ```bash | ||
| NON_CHANGESET_FILES=$(echo "$CHANGED_FILES" | grep -v '^\.changeset/.*\.md$' || true) |
There was a problem hiding this comment.
🟡 Minor: Code example shows wrong grep pattern
Issue: The pattern '^\.changeset/.*\.md$' only matches .md files, but the implementation uses '^\.changeset/' to match all files in .changeset/.
Why: Code examples in specs should match the actual implementation to avoid confusion when developers reference this document.
Fix: (1-click apply)
| NON_CHANGESET_FILES=$(echo "$CHANGED_FILES" | grep -v '^\.changeset/.*\.md$' || true) | |
| NON_CHANGESET_FILES=$(echo "$CHANGED_FILES" | grep -v '^\.changeset/' || true) |
Refs:
|
|
||
| ### Enhanced Detection Logic | ||
|
|
||
| Add a second check after the existing one: if `IS_CHANGESET` is still `false` and the event is not a `push` to main, use the GitHub API to list changed files and check if ALL of them match `.changeset/*.md`. |
There was a problem hiding this comment.
🟡 Minor: Description mentions only .md files
Issue: This line says "check if ALL of them match .changeset/*.md" but the implementation matches all files in .changeset/, not just .md files.
Why: The prose should accurately describe what the implementation does.
Fix: (1-click apply)
| Add a second check after the existing one: if `IS_CHANGESET` is still `false` and the event is not a `push` to main, use the GitHub API to list changed files and check if ALL of them match `.changeset/*.md`. | |
| Add a second check after the existing one: if `IS_CHANGESET` is still `false` and the event is not a `push` to main, use the GitHub API to list changed files and check if ALL of them are under `.changeset/`. |
|
|
||
| ## Acceptance Criteria | ||
|
|
||
| 1. PRs that only add/modify `.changeset/*.md` files skip all CI steps in `ci.yml` and `cypress.yml` |
There was a problem hiding this comment.
🟡 Minor: Acceptance criteria mentions only .md files
Issue: This criterion says "PRs that only add/modify .changeset/*.md files" but the implementation skips CI for all .changeset/ files.
Why: Acceptance criteria should accurately describe the implemented behavior.
Fix: (1-click apply)
| 1. PRs that only add/modify `.changeset/*.md` files skip all CI steps in `ci.yml` and `cypress.yml` | |
| 1. PRs that only add/modify files under `.changeset/` skip all CI steps in `ci.yml` and `cypress.yml` |
Summary
.mdfiles.changeset/files (includingconfig.json), per the original intentgh pr difffailure logging)Test plan
.changeset/config.jsonchanged → CI skipped.changeset/*.mdfiles → CI skipped.changeset/+ code changes → CI runs🤖 Generated with Claude Code