Skip to content

ci: enforce changesets for affected packages on PRs#49

Merged
mgabor3141 merged 1 commit into
mainfrom
ci-changeset-check
May 2, 2026
Merged

ci: enforce changesets for affected packages on PRs#49
mgabor3141 merged 1 commit into
mainfrom
ci-changeset-check

Conversation

@mgabor3141
Copy link
Copy Markdown
Owner

@mgabor3141 mgabor3141 commented May 2, 2026

Why

Several recent PRs (e.g. #41) modified publishable packages without a corresponding changeset, which means those fixes never get released. Catching this in code review is unreliable; let's enforce it in CI.

What this does

Adds a Changeset check workflow that runs on every PR to main and fails if any affected publishable package is missing a changeset entry.

Affected = a release-worthy file under packages/<pkg>/ was added/modified/renamed/deleted in the PR diff. Excluded:

  • **/test/**, **/__tests__/**, **/*.test.{ts,tsx,js,jsx}
  • tsup.config.*, vitest.config.*, tsconfig*, biome*
  • CHANGELOG.md (regenerated by changesets)
  • packages with "private": true or no name

Covered = a .changeset/*.md file (added or modified in this PR, excluding README.md) has a frontmatter line naming the package at any bump level. Modifications are accepted so a PR amending an earlier changeset on the same branch still passes.

The auto-generated changeset-release/* PR opened by the changesets action is skipped via the workflow if:.

The workflow checks out the PR head SHA explicitly (not the synthetic merge commit) so HEAD and pull_request.head.sha agree, and uses fetch-depth: 0 so the merge-base with base.sha is reachable. The script reads changeset content with git show ${HEAD_SHA}:<path> so it's robust to checkout strategy.

Verification

Replayed the script against historical PRs:

PR Outcome Expected
#41 (budget-model fix, no changeset) fail ✅ would have caught
#39 (enclave feat + changeset) pass
#36 (bash-bg README fix + changeset) pass
#35 (root README only) skipped (no package files)
#33 (jujutsu fix + changeset) pass

Plus a synthetic two-package scenario verifying multi-package detection and the amended-changeset path.

Notes

  • Plain Node, no extra deps; no yarn install needed in the job, so it's fast.
  • Not adding unit tests for the script: the meaningful behaviour is git-driven (diff parsing against real changeset files), which is best exercised by the historical replays above plus the live PR feedback loop. A mocked unit test would test the mock, not the script.

@greptile-apps
Copy link
Copy Markdown

greptile-apps Bot commented May 2, 2026

Greptile Summary

This PR adds a Changeset check GitHub Actions workflow and a companion Node.js script that fail a PR if any modified publishable package is missing a changeset entry. The implementation is well-considered: it uses execFileSync throughout (no shell injection), correctly excludes test/config/spec files, handles amended changesets, and skips the auto-generated release PR.

Confidence Score: 4/5

Safe to merge; the one identified gap (entire-package deletion not detected) is a rare edge case that does not affect normal PR workflows.

No P0 or P1 findings. One P2 logic gap: when a PR removes an entire package directory, pkgNameByDir is built from HEAD where that directory is already absent, so the deleted package never enters the affected set and no changeset is required. All previously flagged issues (shell injection, spec-file exclusion) are correctly addressed in this version.

.github/scripts/check-changesets.mjs — package-deletion edge case around lines 38-46.

Important Files Changed

Filename Overview
.github/scripts/check-changesets.mjs New CI script that detects affected publishable packages and verifies changeset coverage; uses execFileSync throughout (no shell injection), excludes test/config files and spec-style tests correctly; edge case: entire package removal is not detected because pkgNameByDir is built from HEAD where the directory is already gone.
.github/workflows/changeset-check.yml New workflow triggering on PRs to main; correctly skips release PRs, checks out the PR head SHA (not the merge commit), uses fetch-depth: 0 for full history, and passes base/head SHAs via env vars.

Flowchart

%%{init: {'theme': 'neutral'}}%%
flowchart TD
    A[PR opened / updated] --> B{head_ref starts with\nchangeset-release/?}
    B -- yes --> C[Skip job]
    B -- no --> D[Checkout PR head SHA\nfetch-depth: 0]
    D --> E[git diff baseSha...headSha\n--diff-filter=ACMRTD]
    E --> F[Build pkgNameByDir\nfrom packages/*/package.json at HEAD]
    F --> G[For each changed file:\npackageForFile filter]
    G --> H{Release-worthy file\nin a publishable package?}
    H -- no: test/config/CHANGELOG/private --> I[Skip file]
    H -- yes --> J[Add to affected set]
    J --> K[git diff --diff-filter=AM\n.changeset/*.md]
    K --> L[Parse frontmatter\nfrom each changeset file]
    L --> M[Build covered set\nof package names]
    M --> N{affected - covered = missing?}
    N -- empty --> O[exit 0 ✓]
    N -- non-empty --> P[Print missing packages\nexit 1 ✗]
Loading
Prompt To Fix All With AI
Fix the following 1 code review issue. Work through them one at a time, proposing concise fixes.

---

### Issue 1 of 1
.github/scripts/check-changesets.mjs:38-46
**Entire-package deletion silently bypasses the check**

`pkgNameByDir` is built by scanning the filesystem at checkout HEAD, so when a PR removes an entire package directory, that directory no longer exists on disk. Any deleted files from that package pass through `packageForFile` and return `null` (line 54 — package not found in the map), so the package never enters `affected`. A PR that fully removes a publishable package would pass the check without a changeset.

The fix is to also diff `packages/*/package.json` at the base ref to find packages that existed then but are absent now:

```js
// Also capture packages deleted entirely in this PR.
const deletedPkgFiles = git("diff", "--name-only", "--diff-filter=D", diffRange, "--", "packages/*/package.json")
  .split("\n")
  .filter(Boolean);
for (const f of deletedPkgFiles) {
  const m = f.match(/^packages\/([^/]+)\/package\.json$/);
  if (!m) continue;
  try {
    const pj = JSON.parse(git("show", `${baseSha}:${f}`));
    if (!pj.private && pj.name) affected.add(pj.name);
  } catch { /* file gone at both refs — skip */ }
}
```

Reviews (3): Last reviewed commit: "ci: enforce changesets for affected pack..." | Re-trigger Greptile

Comment thread .github/scripts/check-changesets.mjs Outdated
Comment thread .github/scripts/check-changesets.mjs Outdated
@mgabor3141 mgabor3141 force-pushed the ci-changeset-check branch 2 times, most recently from 4d570d1 to 5b3680e Compare May 2, 2026 10:26
Adds a Changeset check workflow that runs on every PR and fails when a
publishable package was modified without a corresponding new (or amended)
.changeset/*.md entry. Test files, build/test config, CHANGELOG.md, and
private packages are excluded. The auto-generated changeset-release/* PR
is skipped via the workflow if condition.

The check is implemented as a small Node script (no extra deps) that
reads added/modified changeset files via git show, so it works regardless
of whether checkout uses the merge commit or the head ref.

address review: avoid shell, exclude .spec files, drop dead fallback

- Switch all git invocations from execSync to execFileSync so paths from
  git diff output cannot be interpreted as shell metacharacters. Verified
  with a malicious .changeset/$(...).md filename: old version executed
  the embedded command, new version parses the file safely.
- Exclude .spec.[jt]sx? in addition to .test.[jt]sx? to cover the other
  common test naming convention.
- Remove the shallow-fetch fallback: the workflow always uses
  fetch-depth: 0 so both SHAs are reachable; the fallback was dead code.
@mgabor3141 mgabor3141 force-pushed the ci-changeset-check branch from 5b3680e to 3abe493 Compare May 2, 2026 10:32
@mgabor3141
Copy link
Copy Markdown
Owner Author

mgabor3141 commented May 2, 2026

[AGENT] Addressed greptile's review in 3abe493:

P1 shell injection — fixed. All git calls now go through execFileSync (no shell). Verified the exploit reproduces against the old version and is neutralised by the new one: a malicious .changeset/$(touch PWNED).md filename executed touch under execSync but is treated as a literal path argument under execFileSync.

.spec. files not excluded — fixed. Pattern is now /\.(test|spec)\.[jt]sx?$/.

Redundant shallow-fetch fallback — removed. The workflow uses fetch-depth: 0 unconditionally, so the git cat-file / git fetch --depth=1 block was dead code; a stale comment now makes the assumption explicit.

@mgabor3141 mgabor3141 merged commit 6ac0d74 into main May 2, 2026
2 checks passed
@mgabor3141 mgabor3141 deleted the ci-changeset-check branch May 2, 2026 10:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant