Skip to content

Conversation

@chambridge
Copy link
Collaborator

Description

The workflow_run event's pull_requests array is empty for fork PRs due to a known GitHub limitation. This prevented coverage comments from being posted on fork PRs.

Type of Change

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • Documentation update
  • Refactoring (no functional changes)
  • Performance improvement
  • Test coverage improvement

Related Issues

Relates to https://github.com/orgs/community/discussions/25220

Changes Made

Replace the JavaScript-based pull_requests array lookup with gh pr view command, which reliably finds PRs from both forks and the same repo. Add conditional logic to use the correct branch query format:

  • Fork PRs: "owner:branch" format
  • Same-repo PRs: "branch" format

Testing

Checklist

  • My code follows the project's code style
  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • My changes generate no new warnings
  • I have added tests that prove my fix is effective or that my feature works
  • New and existing unit tests pass locally with my changes
  • Any dependent changes have been merged and published

Screenshots (if applicable)

Additional Notes

The workflow_run event's pull_requests array is empty for fork PRs due
to a known GitHub limitation. This prevented coverage comments from
being posted on fork PRs.

Replace the JavaScript-based pull_requests array lookup with gh pr view
command, which reliably finds PRs from both forks and the same repo.
Add conditional logic to use the correct branch query format:
- Fork PRs: "owner:branch" format
- Same-repo PRs: "branch" format

See: https://github.com/orgs/community/discussions/25220

Co-Authored-By: Claude <noreply@anthropic.com>
Signed-off-by: Chris Hambridge <chambrid@redhat.com>
@github-actions
Copy link
Contributor

AgentReady Code Review: PR #253

Overview

PR: fix(ci): use gh pr view for fork PR number lookup in coverage comment
Type: Bug Fix
Impact: Enables coverage comments on fork PRs


✅ Strengths

1. Problem Analysis (Excellent)

  • Root Cause Identified: Correctly identified that workflow_run.pull_requests array is empty for fork PRs due to GitHub API limitation
  • Evidence-Based: References official GitHub community discussion (#25220)
  • Testing Validation: Tested both fork PR and same-repo PR scenarios

2. Security (Strong)

  • Maintains Security Model: Preserves existing PR number validation at .github/workflows/coverage-comment.yml:84-86
  • Trusted Source: Uses github.event.workflow_run context (trusted) rather than potentially malicious fork PR context
  • No New Attack Surface: Replacement approach does not introduce new security risks

3. Code Quality (Good)

  • Clear Logic: Branch query format selection is readable and maintainable
  • Error Handling: Gracefully handles PR lookup failures with descriptive error messages
  • Documentation: Inline comments explain the fork/same-repo distinction

⚠️ Issues & Recommendations

Critical Issues

1. Shell Quoting Best Practice (Medium Priority)

Location: .github/workflows/coverage-comment.yml:54

Issue: The stderr redirect 2>/dev/null pattern could be improved per CLAUDE.md guidelines.

Current:

PR_NUMBER=$(gh pr view \
  --repo "$TARGET_REPO" \
  "$BRANCH_QUERY" \
  --json number --jq .number 2>/dev/null || echo "")

Recommended: Use combined redirect for efficiency (per CLAUDE.md: "Use proper shell quoting and combined redirects for efficiency")

Note: Current implementation is functional; this is a style improvement aligned with repository standards.

2. Error Message Could Be More Actionable (Low Priority)

Location: .github/workflows/coverage-comment.yml:57-58

Current:

echo "::error::Could not find PR for ${BRANCH_QUERY} in ${TARGET_REPO}"

Suggested Enhancement:

echo "::error::Could not find PR for ${BRANCH_QUERY} in ${TARGET_REPO}. Verify the branch exists and has an open PR."

Minor Observations

3. Missing Actionlint Validation Evidence

Requirement: CLAUDE.md states "ALWAYS run actionlint and fix any issues before pushing changes"

Status: PR description mentions "actionlint validation: Passed with no errors" but does not show output or CI job evidence.

Recommendation: Include actionlint output in PR description or ensure CI includes actionlint check.

4. Incomplete Test Checklist

Location: PR Description

Several test checklist items are unchecked:

  • Unit tests pass (pytest)
  • My code follows the project's code style

Note: These may not apply to workflow-only changes, but clarifying "N/A - Workflow only" would be clearer.


📊 AgentReady Attribute Mapping

Attribute Status Score Impact Notes
CI/CD Pipeline ✅ Improved +1 Fixes fork PR coverage reporting
Pre-commit Hooks ⚠️ Partial 0 actionlint validation claimed but not evidenced
Code Documentation ✅ Pass 0 Inline comments explain logic
Security Practices ✅ Pass 0 Maintains PR number validation
Error Handling ✅ Pass 0 Graceful failure with error messages
Test Coverage ⚠️ N/A 0 Workflow changes (no unit tests applicable)

Net Impact: +0.5 to 1.0 points (minor improvement in CI/CD robustness)


🔒 Security Analysis

No New Vulnerabilities Introduced ✅

  1. PR Number Validation: Existing security check at line 84-86 prevents cross-PR comment injection
  2. Trusted Context: Uses github.event.workflow_run (trusted) for HEAD_REPO/HEAD_BRANCH
  3. No User Input: BRANCH_QUERY constructed from GitHub context, not user-controlled
  4. gh CLI: Uses official GitHub CLI with proper authentication via GH_TOKEN

Potential Edge Cases to Consider

  1. Branch Names with Special Characters: The ${HEAD_REPO%%/*} expansion assumes owner is before first /. Multi-level org paths could cause issues, but this is low risk (GitHub does not support multi-level orgs in full_name).

  2. Rate Limiting: gh pr view calls GitHub API. Multiple concurrent workflow runs could hit rate limits, but risk is low (workflow_run triggers are typically sequential and already uses github.token which has higher rate limits).


🎯 Best Practices Alignment

Follows AgentReady Principles ✅

  • Graceful Degradation: Falls back to error message if PR not found
  • Clear Error Messages: Uses ::error:: syntax for GitHub Actions
  • Documentation: Comments explain GitHub API limitation
  • Minimal Change: Surgical fix without over-engineering

📝 Recommendations Summary

Must Fix (Before Merge)

  • None (PR is functionally correct)

Should Fix (Improve Quality)

  1. Update stderr redirect to use combined redirect pattern per CLAUDE.md (optional style improvement)
  2. Add actionlint output evidence to PR description or CI

Consider for Future

  1. Add integration test for fork PR scenario
  2. Document branch name assumptions
  3. Add metrics/notices to track fork vs same-repo query usage

Final Verdict

Approval Status: ✅ LGTM with Minor Suggestions

Reasoning:

  • Correctly solves the fork PR coverage comment problem
  • Maintains security properties
  • No breaking changes or regressions
  • Minor shell best practice improvements suggested but not blocking

Suggested Action: Merge as-is or after addressing shell redirect pattern (2-minute fix).


Review conducted by AgentReady-aware Claude Code Agent
Date: 2026-01-15
AgentReady Version: 2.22.0

@jeremyeder jeremyeder merged commit 1688362 into ambient-code:main Jan 15, 2026
10 checks passed
github-actions bot pushed a commit that referenced this pull request Jan 15, 2026
## [2.22.2](v2.22.1...v2.22.2) (2026-01-15)

### Bug Fixes

* **ci:** use gh pr view for fork PR number lookup in coverage comment ([#253](#253)) ([1688362](1688362))
@github-actions
Copy link
Contributor

🎉 This PR is included in version 2.22.2 🎉

The release is available on GitHub release

Your semantic-release bot 📦🚀

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

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants