-
Notifications
You must be signed in to change notification settings - Fork 1.4k
Consolidate CI workflows into a single entry point #836
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
- Add ci-all.yml to orchestrate all CI workflows on push to main - Update individual workflows to use workflow_call for reusability - Remove redundant push triggers from individual test workflows - Update release.yml to trigger on CI All workflow completion - Auto-release on version bump commits after CI passes Co-Authored-By: Claude <noreply@anthropic.com> Claude-Generated-By: Claude Code (cli/claude-opus-4-5=100%) Claude-Steers: 8 Claude-Permission-Prompts: 1 Claude-Escapes: 0
| if: | | ||
| github.event_name == 'workflow_dispatch' || | ||
| (github.event.workflow_run.conclusion == 'success' && | ||
| startsWith(github.event.workflow_run.head_commit.message, 'chore: bump Claude Code to')) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
SECURITY: The workflow_run trigger has a potential security vulnerability. The current condition only checks the commit message, which is insufficient.
Attack vector: A malicious PR could craft a commit message starting with "chore: bump Claude Code to" and potentially trigger a release if the branch filter is bypassed.
Recommendation: Add explicit branch and event type verification:
| if: | | |
| github.event_name == 'workflow_dispatch' || | |
| (github.event.workflow_run.conclusion == 'success' && | |
| startsWith(github.event.workflow_run.head_commit.message, 'chore: bump Claude Code to')) | |
| if: | | |
| github.event_name == 'workflow_dispatch' || | |
| (github.event.workflow_run.conclusion == 'success' && | |
| github.event.workflow_run.head_branch == 'main' && | |
| github.event.workflow_run.event == 'push' && | |
| startsWith(github.event.workflow_run.head_commit.message, 'chore: bump Claude Code to')) |
This adds defense-in-depth by verifying:
- The workflow ran on the
mainbranch (not just filtered to it) - The triggering event was a
push(not apull_request)
| branches: | ||
| - main | ||
| pull_request: | ||
| workflow_dispatch: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
SECURITY: Missing explicit permission declarations. When no permissions are declared, workflows get default read-all permissions for GITHUB_TOKEN.
Recommendation: Add explicit minimal permissions:
| workflow_dispatch: | |
| workflow_dispatch: | |
| permissions: | |
| contents: read |
This follows the principle of least privilege and ensures called workflows don't have more permissions than necessary.
Test Coverage ConcernsMissing test coverage for the workflow_run auto-trigger logic The most critical change—auto-triggering releases based on commit message matching—has no automated tests. This is a high-risk change that could silently fail or trigger unintended releases. Edge cases not covered in the test plan:
Recommendation: Add validation tests for the commit message matching logic, or at minimum expand the test plan to include these negative test cases. workflow_dispatch inputs and workflow_call incompatibility In
Is this intentional? If yes, it should be documented. If no, consider harmonizing the workflows. |
| @@ -0,0 +1,32 @@ | |||
| name: CI All | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Consider adding a brief header comment explaining the orchestrator role:
| name: CI All | |
| # Orchestrates all CI workflows - runs on PRs, pushes to main, and manual dispatch | |
| # Individual test workflows are called as reusable workflows | |
| name: CI All |
This helps future maintainers understand the purpose of this workflow at a glance.
| uses: ./.github/workflows/ci.yml | ||
|
|
||
| test-base-action: | ||
| uses: ./.github/workflows/test-base-action.yml |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Consider adding a brief comment explaining why secrets: inherit is necessary:
| uses: ./.github/workflows/test-base-action.yml | |
| test-base-action: | |
| uses: ./.github/workflows/test-base-action.yml | |
| secrets: inherit # Required for ANTHROPIC_API_KEY |
This clarifies the security implications and dependencies.
General ObservationsStrengths:
Performance:
Documentation:
|
Summary
ci-all.ymlworkflow that orchestrates all CI workflows (ci, test-base-action, test-custom-executables, test-mcp-servers, test-settings, test-structured-output)workflow_callfor reuse and removes redundantpushtriggersrelease.ymlto auto-trigger on successfulCI Allruns for version bump commitsTest plan
CI Allworkflow triggers on PRs and runs all sub-workflowsCI Allworkflow triggers on pushes to mainworkflow_dispatchCI Allon version bump commitsworkflow_dispatchfor releases still worksChangelog
🤖 Generated with Claude Code (100% 10-shotted by claude-opus-4-5)