Skip to content

fix(review-mcp-update): check CI status first as hard gate #395

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.

Sign up for GitHub

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

Closed
JAORMX wants to merge 1 commit into from
Closed

fix(review-mcp-update): check CI status first as hard gate #395

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
76 changes: 46 additions & 30 deletions .claude/skills/review-mcp-update/SKILL.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@ license: Apache-2.0
compatibility: Requires GitHub MCP server for API access
metadata:
author: stacklok
version: "1.0"
version: "1.1"
---

# MCP Server Update PR Review Skill
Expand All @@ -26,15 +26,44 @@ Use this skill when:

## Review Process

### Step 1: Identify the PR and Changed Files
### Step 1: Verify CI Status (HARD GATE — Do This First)

First, get the PR details and identify what changed:
**Always check CI before any other analysis. If CI is red, stop — do not approve.**

**IMPORTANT:** Do NOT use `mcp__github__pull_request_read` with method `get_status` — it only returns legacy commit statuses and misses GitHub Actions check runs entirely (returns `total_count: 0`).
Copy link
Member

@aponcedeleonch aponcedeleonch Feb 13, 2026

Choose a reason for hiding this comment

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

I would remove this part on what not to do. Indicating what to do is better, which is done right below


**Use this `gh` command via Bash to get actual check run results:**
```bash
gh pr list --repo {owner}/{repo} --state open --json number,title,statusCheckRollup \
--jq '.[] | {number, title, checks: [.statusCheckRollup[]? | {name: .name, status: .status, conclusion: .conclusion}]}'
```

When reviewing a single PR, add `| select(.number == {PR_NUMBER})` to the jq filter.

**Required checks — FAILURE on any of these blocks the PR:**
| Check | FAILURE | SUCCESS | NEUTRAL | SKIPPED |
|-------|---------|---------|---------|---------|
| `mcp-security-scan` | BLOCK | OK | OK | OK |
| `build-containers` | BLOCK | OK | — | OK |
| `Build` | BLOCK | OK | — | — |
| `Lint` | BLOCK | OK | — | — |
Comment on lines +44 to +49
Copy link
Member

@aponcedeleonch aponcedeleonch Feb 13, 2026

Choose a reason for hiding this comment

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

not sure if the table is needed here. Probably a list would be more than enough. IMO it's not adding any new information. The distinction is already clear that these CI checks should fail and the ones below are informational


**Informational checks (do not block merge):**
- `verify-provenance` — note regressions but don't block
- `Trivy` — NEUTRAL is normal; review only if FAILURE
- `summary`, `discover-configs`, `save-pr-number` — infrastructure, ignore

**HARD GATE:** If ANY required check has `conclusion: "FAILURE"`, do NOT approve or recommend merging that PR. Report the failure and stop the review for that PR. Continue reviewing other PRs.

### Step 2: Identify the PR and Changed Files

Get the PR details and identify what changed:

1. Use `mcp__github__pull_request_read` with method `get` to get PR details
2. Use `mcp__github__pull_request_read` with method `get_files` to see changed files
3. Look for changes to `spec.yaml` files in `npx/`, `uvx/`, or `go/` directories
Comment on lines 62 to 64
Copy link
Member

@aponcedeleonch aponcedeleonch Feb 13, 2026

Choose a reason for hiding this comment

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

Better to be consistent, either the skill should always use gh or always use the MCP, not a mixture. Up in the prompt it is mentioned gh is preferred so this should change


### Step 2: Analyze Version Changes
### Step 3: Analyze Version Changes

For each changed `spec.yaml`:

Expand All @@ -51,24 +80,7 @@ For each changed `spec.yaml`:
- Authentication requirements
- API endpoints

### Step 3: Verify CI Status

Check that all required CI checks pass:

1. Use `mcp__github__pull_request_read` with method `get_status` to check CI status
2. **Required checks:**
- `MCP Security Scan` - Must pass (blocks merge if failed)
- `Verify Provenance` - Informational, check for regressions
- `Build Containers` - Must pass
- `Trivy Vulnerability Scan` - Review findings

3. **If MCP Security Scan fails**, check for:
- Prompt injection risks (AITech-1.1)
- Data exfiltration (AITech-8.2) and system manipulation (AITech-9.1) flows
- Tool exploitation (AITech-12.1)
- Whether a security allowlist entry is needed in `spec.yaml`

### Step 3a: Investigating Security Scan Failures
### Step 3a: Investigating Security Scan Failures (when mcp-security-scan fails)

When the MCP Security Scan fails, determine if issues are **real security concerns** or **false positives** before adding allowlist entries:

Expand All @@ -89,7 +101,7 @@ When the MCP Security Scan fails, determine if issues are **real security concer

See [references/INVESTIGATING_SECURITY_ISSUES.md](references/INVESTIGATING_SECURITY_ISSUES.md) for detailed investigation procedures, issue code meanings, and examples of identifying false positives.

### Step 4: Evaluate Upstream Repository Health
### Step 4: Evaluate Upstream Repository Health (for new servers or major updates)

For new servers or major version updates, evaluate against ToolHive registry criteria:

Expand Down Expand Up @@ -151,25 +163,29 @@ After completing the review, provide a structured summary:
```markdown
## MCP Server Update Review: [Server Name]

### CI Status (checked first)
- Build: [Pass/Fail]
- Lint: [Pass/Fail]
- MCP Security Scan: [Pass/Fail/Skipped]
- Container Build: [Pass/Fail/Skipped]
- Provenance Verification: [Pass/Fail] (informational)
- Trivy: [Neutral/Findings] (informational)

> If any required check FAILED, stop here: **DO NOT MERGE**

### Change Summary
- **Package:** [package name]
- **Version:** [old version] → [new version]
- **Change Type:** [Patch/Minor/Major]
- **Protocol:** [npx/uvx/go]

### CI Status
- MCP Security Scan: [Pass/Fail/Pending]
- Provenance Verification: [Verified/Signatures/None]
- Container Build: [Pass/Fail/Pending]
- Vulnerability Scan: [Clean/Findings]

### Breaking Changes
[List any breaking changes from release notes]

### Security Considerations
[Any new security allowlist entries or concerns]

### Upstream Health (for major updates)
### Upstream Health (for major updates only)
- License: [license type]
- Recent Activity: [active/stale]
- Open Issues: [count]
Expand Down
Loading