Skip to content

fix(ensureSCCoreDevApproval): re-evaluate core-dev-approval on PR lifecycle events (draft-skip bypass)#2035

Open
0xDEnYO wants to merge 3 commits into
mainfrom
fix/core-dev-approval-draft-skip
Open

fix(ensureSCCoreDevApproval): re-evaluate core-dev-approval on PR lifecycle events (draft-skip bypass)#2035
0xDEnYO wants to merge 3 commits into
mainfrom
fix/core-dev-approval-draft-skip

Conversation

@0xDEnYO

@0xDEnYO 0xDEnYO commented Jul 10, 2026

Copy link
Copy Markdown
Contributor

Which Linear task belongs to this PR?

No Linear task yet — direct follow-up to the PR #2010 merge-gate incident (core-dev approval check bypassed).

Why did I implement it this way?

Incident: #2010 merged with only a lifi-qa-agent approval. The bot approved while the PR was a draft, so the core-dev-approval job (job-level if: draft == false) was skipped — and GitHub counts a skipped check run as passing a required status check. The workflow's only trigger was pull_request_review: submitted, so marking the PR ready never re-ran it; the stale skipped verdict on the head SHA satisfied branch protection and auto-merge fired.

Fix: add pull_request lifecycle triggers (opened, reopened, ready_for_review, synchronize) and pull_request_review: dismissed:

  • Any path to a mergeable PR now forces a real evaluation on the head commit: leaving draft fires ready_for_review, which replaces the skipped check run on the same SHA with a genuine pass/fail; pushes re-run it (closing the gap where dismiss_stale_reviews invalidated the approval but not the green check).
  • The job now runs unconditionally, including on drafts (decision: Daniel, 2026-07-10): the check can never conclude skipped, so the skipped-counts-as-passing failure mode is structurally impossible rather than dependent on trigger choreography staying correct. A red check on an unapproved draft is honest signal; it goes green the moment a core dev approves.
  • Added a per-PR concurrency group so rapid review/push events don't stack duplicate runs.

Note for reviewer: bot approvals still satisfy GitHub's native 1-approval rule by design — the decision (Daniel, 2026-07-10) is that this hardened core-dev check is the enforcement layer, so it must never be bypassable.

Checklist before requesting a review

Checklist for reviewer (DO NOT DEPLOY and contracts BEFORE CHECKING THIS!!!)

  • I have checked that any arbitrary calls to external contracts are validated and or restricted
  • I have checked that any privileged calls (i.e. storage modifications) are validated and or restricted
  • I have ensured that any new contracts have had AT A MINIMUM 1 preliminary audit conducted on by <company/auditor>

…e events so draft-time skips cannot satisfy branch protection

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
@coderabbitai

coderabbitai Bot commented Jul 10, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

Walkthrough

The Smart Contract Core Dev approval workflow now runs on more pull request and review events, cancels superseded runs for the same pull request, and executes for draft pull requests instead of skipping them.

Changes

Smart Contract Core Dev approval workflow

Layer / File(s) Summary
Lifecycle triggers and run concurrency
.github/workflows/ensureSCCoreDevApproval.yml
The workflow responds to pull request opening, reopening, readiness, synchronization, review submission, and review dismissal, while canceling older runs for the same pull request.
Unconditional approval check
.github/workflows/ensureSCCoreDevApproval.yml
The job-level draft restriction is removed so the required check does not skip draft pull requests.

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

Possibly related PRs

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly names the workflow and the main fix to re-run core-dev approval on PR lifecycle events.
Description check ✅ Passed The description covers the Linear task, rationale, checklist items, and reviewer notes required by the template.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
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.
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch fix/core-dev-approval-draft-skip

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

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

@lifi-action-bot
lifi-action-bot marked this pull request as draft July 10, 2026 03:36
@0xDEnYO
0xDEnYO marked this pull request as ready for review July 10, 2026 03:38
…eck can never conclude skipped

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
@0xDEnYO
0xDEnYO enabled auto-merge (squash) July 10, 2026 04:06

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🧹 Nitpick comments (1)
.github/workflows/ensureSCCoreDevApproval.yml (1)

18-20: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick win

Move permissions to per-job scope with workflow-level default-deny.

The path instruction requires permissions: {} at the workflow level (default-deny) and minimal scopes granted per-job, each with an inline "why". Currently, pull-requests: read and contents: read are granted at the workflow level instead.

As per path instructions, .cursor/rules/500-github-actions.mdc requires: "Use least-privilege permissions: set permissions: {} at the workflow level (default-deny) and grant minimal scopes per-job, each with an inline 'why'."

🔒 Proposed refactor for least-privilege permissions
 permissions:
-  pull-requests: read # required to read PR reviews and metadata
-  contents: read # required to read local workspace contents (e.g. access tmp file)
+  {}
 
 concurrency:
   group: core-dev-approval-${{ github.event.pull_request.number }}
   cancel-in-progress: true
 
 jobs:
   core-dev-approval:
+    permissions:
+      pull-requests: read # required to read PR reviews and metadata
+      contents: read # required to read local workspace contents (e.g. access tmp file)
     # no draft condition on purpose — a skipped run counts as passing for branch protection
     runs-on: ubuntu-latest
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In @.github/workflows/ensureSCCoreDevApproval.yml around lines 18 - 20, Set the
workflow-level permissions to an empty mapping for default-deny, then add a
permissions block under the relevant job with only pull-requests: read and
contents: read, preserving inline comments explaining why each scope is
required.

Source: Path instructions

🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Nitpick comments:
In @.github/workflows/ensureSCCoreDevApproval.yml:
- Around line 18-20: Set the workflow-level permissions to an empty mapping for
default-deny, then add a permissions block under the relevant job with only
pull-requests: read and contents: read, preserving inline comments explaining
why each scope is required.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Repository UI

Review profile: CHILL

Plan: Pro

Run ID: 7c9e2a78-c5a6-4df0-a6e6-8b7b26d80535

📥 Commits

Reviewing files that changed from the base of the PR and between 8a150a1 and 2955984.

📒 Files selected for processing (1)
  • .github/workflows/ensureSCCoreDevApproval.yml

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants