Skip to content

ci: per-commit fast-check gate (catch broken intermediate commits under rebase-merge)#1190

Open
vringar wants to merge 3 commits into
masterfrom
ci/per-commit-checks
Open

ci: per-commit fast-check gate (catch broken intermediate commits under rebase-merge)#1190
vringar wants to merge 3 commits into
masterfrom
ci/per-commit-checks

Conversation

@vringar

@vringar vringar commented Jun 15, 2026

Copy link
Copy Markdown
Contributor

Motivation

OpenWPM uses rebase-and-merge, so every commit in a PR lands on master
individually. But CI only tests the PR tip (and the merge-queue candidate
tip). That means a commit which is broken in isolation — e.g. it introduces a
syntax error, an undefined name, a bad import, or a type/lint regression that a
later commit in the same PR happens to fix — can still ship to master as an
individual broken commit. Anyone who later bisects, checks out, or builds from
that commit hits the breakage.

This adds a lightweight gate, per-commit-checks.yaml, that runs the fast
static checks on every commit
in the PR / merge-queue range, so an
intermediate commit that's broken on its own can't slip through. It's the cheap
80/20: it reuses the existing ./.github/actions/setup composite action (conda
env + built extension) and finishes quickly because it skips the expensive part.

What it catches (per commit)

  • Syntax errors / undefined names / bad importspython -c "import openwpm"
    plus pytest --collect-only exercise import of the package and the whole test
    tree (this also covers cross-file breaks that mypy's changed-files-only view
    can miss, e.g. a symbol deleted in one module but still referenced by an
    untouched one).
  • Type errors & lintpre-commit run --from-ref <c~1> --to-ref <c> runs
    black / isort / mypy / actionlint on the files that commit changed. mypy is
    the load-bearing check here: it flags [name-defined] (undefined name) and
    type errors in changed files.
  • Extension TypeScript buildnpm run build runs only when the commit
    touched Extension/, catching broken extension sources.

What it intentionally does NOT do

  • It does not run the browser/Selenium test suite. Runtime and
    browser-behavior regressions stay covered by the existing tests job on the
    PR tip + the merge queue, plus author discipline. Running the full suite on
    every commit would be far too expensive for the marginal value.

Notes

  • This job only gates merges once it's added as a required status check
    in branch protection. Until then it runs informationally.
  • Opened as a draft for design review — feedback welcome on scope (which
    checks belong in the per-commit tier) and on the merge-queue handling.
  • The workflow self-tests by running on its own PR (pull_request trigger).
    The merge_group trigger only fires inside a real merge queue, so that path
    can't be exercised from the PR itself; the field names
    (merge_group.base_sha / merge_group.head_sha) were validated against the
    published webhook payload schema.

How the commit range is computed

  • pull_request: base = pull_request.base.sha, head = pull_request.head.sha
    — the PR's own commits (not the synthetic merge commit). fetch-depth: 0 plus
    explicit-SHA git checkout in the loop makes this independent of which ref
    actions/checkout materialized.
  • merge_group: base = merge_group.base_sha (the group's parent commit),
    head = merge_group.head_sha (the group tip) — exactly the candidate commits.

The loop uses set -uo pipefail (not set -e) and || { ...; fail=1; } so it
visits every commit and reports all failing ones, then fails the job.

@vringar vringar force-pushed the ci/per-commit-checks branch from f361334 to 1041f4f Compare June 15, 2026 22:14
@codecov

codecov Bot commented Jun 15, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 62.12%. Comparing base (315e667) to head (c4533be).
⚠️ Report is 1 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #1190      +/-   ##
==========================================
- Coverage   62.14%   62.12%   -0.03%     
==========================================
  Files          40       40              
  Lines        3918     3918              
==========================================
- Hits         2435     2434       -1     
- Misses       1483     1484       +1     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

…er rebase-merge)

Add a lightweight CI job that runs the fast static checks on every commit in
a PR / merge-queue candidate, not just the tip. Because the project uses
rebase-and-merge, every commit lands on master individually, but the existing
tests job only validates the tip — a commit that is broken in isolation can
reach master. This gate closes that gap cheaply: per commit it runs the
pre-commit hooks (black/isort/mypy/actionlint) on the changed files, rebuilds
the extension when its sources changed, imports the package, and collects the
test suite. It intentionally does not run the browser test suite — runtime and
browser-behavior regressions stay covered by the tests job on the tip plus the
merge queue.
@vringar vringar marked this pull request as ready for review June 19, 2026 23:03
Copilot AI review requested due to automatic review settings June 19, 2026 23:03
@vringar vringar force-pushed the ci/per-commit-checks branch from 1041f4f to 24076a0 Compare June 19, 2026 23:04

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Adds a new GitHub Actions workflow to gate rebase-and-merge PRs by running a fast static-check suite on every individual commit in the PR (and in merge-queue merge_group candidates), preventing “broken intermediate commits” from landing on master.

Changes:

  • Introduces per-commit-checks.yaml, triggered on pull_request and merge_group.
  • Computes a commit range and iterates commit-by-commit, running pre-commit (changed-files), conditional Extension build, import openwpm, and pytest --collect-only.
  • Aggregates failures across commits (doesn’t stop at first failure) and fails the job at the end if any commit failed.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread .github/workflows/per-commit-checks.yaml
Comment thread .github/workflows/per-commit-checks.yaml
vringar added 2 commits June 20, 2026 09:08
The per-commit gate reused the node_modules installed by the setup
action on the tip ref. A commit that changed Extension/package*.json
without a matching node_modules would run its build against the wrong
dependency set. Run npm ci (lockfile-driven, fast) before the build
when this commit touched the manifest or lockfile.
The per-commit gate checks out each PR commit but kept the conda env from
the tip, so an intermediate commit that changes deps was validated against
the wrong environment (stale-deps false negative).

Mirror the existing Extension-lockfile guard: track a sha256 baseline of
the environment.yaml the env was built from, and re-run install.sh to
recreate the env only when a commit's environment.yaml differs from that
baseline, updating the baseline afterward. Conda setup re-runs only on
actual dep changes (rare), preserving full correctness at minimal cost.
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.

2 participants