Skip to content

Fail the check when a pull request removes the pnpm project - #7

Merged
silveltman merged 4 commits into
mainfrom
fix/check-fail-closed
Oct 2, 2026
Merged

silveltman merged 4 commits into
mainfrom
fix/check-fail-closed

Conversation

@silveltman

@silveltman silveltman commented Oct 2, 2026 •

Copy link
Copy Markdown
Contributor

What changed

The required Check workflow no longer passes silently when a pull request deletes package.json, pnpm-lock.yaml, or the check script.

  • The checkout now uses fetch-depth: 2, so the merge commit's parents are available. The first parent is the base branch.
  • "Find the check script" compares the base branch with the pull request's merge result:
    1. Merge result has all three: run the check (unchanged).
    2. Base branch has all three, merge result does not: fail with a message naming what was removed.
    3. Neither has all three: pass with a notice (unchanged for non-pnpm repositories).
    4. The checkout is not a merge commit: fail.
  • Everything else is unchanged: SHA pins, permissions, persist-credentials: false, the frozen install with --ignore-scripts --ignore-pnpmfile, the env vars, the timeout, and the CloudCannon auto-merge job.

Why the base branch, not an allowlist

The base branch cannot be changed by the pull request, and it only got its contents through PRs that passed this check, so it is trusted input. This design needs no list to maintain: a repository locks in as soon as its main branch has all three, and a new non-pnpm repository passes without an edit here. An allowlist would also publish private client repository names in this public repository.

How it was verified

  1. Evaluated the logic against the default branch of all 51 non-archived repositories: 41 run their check, 10 pass with a notice (.github, agents, chaletsinfriesland, liquid, overnachtenzuidlaren, positiefboekhouden, docduc, gregoplatform, mandendeco, vanrupsnaarvlinder lack one of the three). None would fail.
  2. Evaluated all 51 open pull requests: none removes package.json, pnpm-lock.yaml, or the check script, so none would fail.
  3. Confirmed on live PR merge commits that the first parent is the base branch and the second is the PR head.
  4. actionlint 1.7.12 with shellcheck 0.11.0: clean.
  5. Ran the step script locally against 14 git fixtures (Node 20 and 24): removing the lockfile, package.json, the check script, or breaking package.json fails; README changes, non-pnpm bases, partial setups, and adding a full pnpm project behave as before.

🤖 Generated with Claude Code

Summary by CodeRabbit

  • CI Updates
    • Pull requests are now checked against their base branch to determine whether the project’s check should run.
    • If the base branch has a check configured but the pull request is missing required project files or the check script, the workflow fails and reports what is missing.
    • If neither branch has the required setup, the check is skipped with a notice.

@coderabbitai

coderabbitai Bot commented Oct 2, 2026 •

Copy link
Copy Markdown

Review in Change Stack →

Navigate logical layers of code changes, visualize relationships, and explore their blast radius.

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration
  • Configuration used: Organization UI
  • Review profile: CHILL
  • Plan: Advanced
  • Run ID: 3d382e76-922b-461a-b8ea-a85041bb5b4b
📥 Commits

Reviewing files that changed from the base of the PR and between 5d22144 and c6a551b.

📒 Files selected for processing (2)
  • .github/workflows/check.yml
  • README.md

Limit details: You’ve used all 10 included reviews currently available.


📝 Walkthrough

Walkthrough

The workflow now compares the pull request’s required files and check script with those in its base commit. It runs the check when the pull request has all prerequisites. If the base has all prerequisites but the pull request does not, the workflow fails and reports what is missing. If neither commit has all prerequisites, it skips with a notice. The README describes these conditions.

Priority: ➖ Normal

Estimated code review effort: 2 (Simple) | ~10 minutes

Change: Bug fix

Merge Risk: ⚪ Minimal · up to c6a55

The check remains ready to merge: it runs for complete pnpm projects, fails when a pull request removes required setup, and skips repositories without that setup.

Architecture Summary

Architecture risk: 🔵 Low · up to c6a55

The change affects 1 system.

Changed systems: README.md

Architecture concerns
No architecture-level concerns identified.

Review details

Systems and components

  • observed — README.md (service) was modified; 1 changed file maps to changed impact.

Before / after behavior

  • observed — Modified behavior in README.md: The README changes the failure condition from lacking a pnpm project or check script to removing package.json, pnpm-lock.yaml, or the check script when all three are present on the base branch; a base branch that is not a pnpm project with a check script passes with a notice.
  • observed — Modified behavior in .github/workflows/check.yml: The workflow comment now describes the base-branch requirement: repositories whose base has the manifest, lockfile, and check script fail if the pull request removes any of them; repositories without that base setup pass with a notice.
  • observed — Modified behavior in .github/workflows/check.yml: Checkout now fetches two commits, enabling the following step to inspect the pull request’s base.
  • observed — Modified behavior in .github/workflows/check.yml: The detection step adds a helper that checks a commit for package.json, pnpm-lock.yaml, and a check script, and reports each missing prerequisite.
🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check. Docstring coverage is scoped to functions touched by this diff. Analyzed 0 functions across 0…
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
Title check ✅ Passed The title clearly summarizes the main change: the workflow fails when a pull request removes the pnpm project.
Description check ✅ Passed The description accurately explains the workflow changes, comparison logic, preserved behavior, and verification steps.
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Commit to this branch
  • Create a new PR
  • Autopilot · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

Autopilot is currently an internal CodeRabbit preview.


Comment @coderabbitai help to get the list of available commands.

silveltman and others added 4 commits October 3, 2026 00:44
The organization check used to pass with "Nothing to run" when package.json,
pnpm-lock.yaml, or the check script was missing, so a pull request that
deleted one of them silently disabled validation.

The base branch now decides: when it has all three, the pull request's merge
result must keep all three or the check fails. The checkout fetches depth 2
so the step can read the merge commit's first parent, which is the base
branch and cannot be changed by the pull request. Repositories that are not
pnpm projects with a check script on the base branch still pass.

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
@silveltman
silveltman force-pushed the fix/check-fail-closed branch from c6a551b to a16fb0b Compare October 2, 2026 22:44
@silveltman
silveltman merged commit fae5230 into main Oct 2, 2026
4 checks passed
@silveltman
silveltman deleted the fix/check-fail-closed branch October 2, 2026 22:44
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