feat: add diff-cover-check composite action#2
Conversation
Composite action for enforcing patch coverage threshold on changed lines. Supports coverage-override label bypass, CODEOWNERS approval, and optional Axiom telemetry. Inputs: threshold Coverage threshold % (default: 80) coverage-file Path to LCOV file (default: coverage/lcov.info) compare-branch Git branch to diff against (required) axiom-token Axiom API token for telemetry (optional)
andreiships-bot
left a comment
andreiships-bot
left a comment
There was a problem hiding this comment.
Single-pass Claude review — found 5 issues across and .
| - name: Install diff-cover | ||
| shell: bash | ||
| run: pip install -r "${{ github.action_path }}/requirements.txt" | ||
|
|
There was a problem hiding this comment.
The --json-report and --html-report output paths are hardcoded to coverage/diff-cover.json / coverage/diff-cover.html. Consider exposing these as action inputs (or deriving them from coverage-file) so callers can control output placement without monkey-patching the action.
| const matches = lineWithoutComment.matchAll(/@([\w.\-]+(?:\/[\w.\-]+)?)/g); | ||
| for (const match of matches) { | ||
| owners.add(match[1]); // Capture group contains owner without @ | ||
| } |
There was a problem hiding this comment.
parseCodeowners opens the file at a runner-relative path (.github/CODEOWNERS). When this action is consumed cross-repo (e.g. andreiships/shared-ai-standards/.github/actions/diff-cover-check@main), the runner workspace is the caller repo — so this works. However the path is silently wrong if the caller stores CODEOWNERS elsewhere. Consider accepting a codeowners-path input and passing it through so callers can override the default.
| - name: Install diff-cover | ||
| shell: bash | ||
| run: pip install -r "${{ github.action_path }}/requirements.txt" | ||
|
|
There was a problem hiding this comment.
Hardcoded report output paths: The --json-report and --html-report destinations (coverage/diff-cover.json, coverage/diff-cover.html) are hardcoded. Consider exposing these as action inputs or deriving them from the coverage-file input, so callers can control output placement when running multiple coverage checks in one workflow.
| const matches = lineWithoutComment.matchAll(/@([\w.\-]+(?:\/[\w.\-]+)?)/g); | ||
| for (const match of matches) { | ||
| owners.add(match[1]); // Capture group contains owner without @ | ||
| } |
There was a problem hiding this comment.
Hardcoded CODEOWNERS path: parseCodeowners reads from .github/CODEOWNERS relative to the runner's working directory. This works when consumed cross-repo (the caller's workspace is $GITHUB_WORKSPACE), but callers with non-standard CODEOWNERS locations (e.g. CODEOWNERS at repo root) will silently fall back to "fail-closed" (no override ever succeeds). Consider a codeowners-path action input, defaulting to .github/CODEOWNERS, passed into checkCodeownersApproval.
|
|
||
| const codeowners = parseCodeowners(); | ||
| const prAuthor = context.payload.pull_request?.user?.login; | ||
|
|
There was a problem hiding this comment.
Unsafe .number access: context.payload.pull_request.number (line 43) will throw TypeError: Cannot read properties of undefined if the action is triggered by a non-PR event (e.g. push, workflow_dispatch). Guard with optional chaining: context.payload.pull_request?.number — and bail early if pull_number is undefined rather than sending a bad API call.
| const prAuthorBase = prAuthor?.replace(/-bot$/, '') || ''; | ||
| const isSoleDev = codeowners.size === 1 && | ||
| (codeowners.has(prAuthor) || codeowners.has(prAuthorBase)); | ||
| if (isSoleDev) { |
There was a problem hiding this comment.
Incorrect bot suffix pattern: The solo-dev exception strips -bot$ from usernames (prAuthorBase = prAuthor?.replace(/-bot$/, '')). GitHub's actual bot convention is the [bot] suffix (e.g. dependabot[bot], github-actions[bot]). A -bot suffix is just a human naming convention. This means the intended bot-exception will silently not fire for real GitHub Apps/bots, while potentially misidentifying human accounts ending in -bot as bot equivalents of another user.
| } | ||
|
|
||
| for (const eventData of events) { | ||
| const payload = [{ |
There was a problem hiding this comment.
Telemetry dataset hardcoded: The Axiom ingest URL embeds ci-metrics as the dataset name (/v1/datasets/ci-metrics/ingest). Since this is a shared action used by multiple repos, the dataset name should be an input (e.g. axiom-dataset, defaulting to ci-metrics) so different consuming repos can route to their own datasets without forking the action.
Codex ReviewSummaryPR is focused and reasonably sized (310 LOC), but it introduces a coverage-gate bypass and a CODEOWNERS approval logic mismatch that can block valid overrides. Merge should be blocked until these are fixed. Findings[FINDING-1] blocking: P0 | .github/actions/diff-cover-check/action.yml:38 | Any [FINDING-2] issue: P1 | .github/actions/diff-cover-check/analyze-coverage-results.mjs:22,79 | Parser accepts Code Quality
Architecture
PR MetadataSuggested PR Title: Recommendation[ ] Approve | [ ] Approve with changes | [x] Request changes Escalate to Gemini?[x] Yes - override authorization/approval logic is security-sensitive and has edge-case risk (team CODEOWNERS + bypass paths) | [ ] No |
Gemini Deep ReviewSummaryThe PR introduces a robust Findings[gemini-1] blocking: P0 | .github/actions/diff-cover-check/action.yml:56-61 | Script Injection vulnerability in const { analyzeCoverageResults, sendTelemetry } = await import('${{ github.action_path }}/analyze-coverage-results.mjs');
const result = await analyzeCoverageResults({
reportPath: 'coverage/diff-cover.json',
threshold: parseInt('${{ inputs.threshold }}', 10),Fix: Pass all inputs (including [gemini-2] blocking: P0 | .github/actions/diff-cover-check/action.yml:45 | Permissive fallback logic allows coverage bypass echo '{"total_percent_covered": 100, "total_num_lines": 0, "total_num_lines_missed": 0, "src_stats": {}}' > coverage/diff-cover.jsonFix: The fallback should indicate an error state or a specific "crash" flag that the analysis script can use to fail the build unless an override is present. Never fake a 0-line report for failures. [gemini-3] issue: P1 | .github/actions/diff-cover-check/analyze-coverage-results.mjs:1 | Missing unit tests for critical analysis logic [gemini-4] issue: P1 | .github/actions/diff-cover-check/analyze-coverage-results.mjs:78-83 | Global CODEOWNERS approval is too permissive for (const [user, state] of latestReviews) {
if (state === 'APPROVED' && codeowners.has(user)) {
return true;
}
}Fix: Ideally, use the GitHub API to verify if the approver is an owner of the changed files. At minimum, document this "Global Owner" behavior as a known limitation. [gemini-5] nit: P2 | .github/actions/diff-cover-check/analyze-coverage-results.mjs:36 | Missing pagination in [gemini-6] nit: P2 | .github/actions/diff-cover-check/README.md | Missing documentation PR MetadataSuggested PR Title:
Questions
Recommendation[ ] Approve | [ ] Approve with changes | [x] Request changes |
- fix(P0): crash fallback now writes sentinel {crash_fallback:true} instead
of fake 0-line report; analyzer fails the build on crash rather than
silently passing (coverage bypass on diff-cover crash/LCOV error)
- fix(P0/P1): move threshold + action_path out of inline JS string into
env vars (COVERAGE_THRESHOLD, ACTION_PATH) to eliminate script injection
risk from direct ${{ inputs.threshold }} / ${{ github.action_path }}
interpolation in github-script
- fix(P1): parseCodeowners regex no longer matches org/team entries
(removed '/' from character class); team slugs can never match reviewer
logins so including them silently blocked valid team-owned overrides
- fix(P2): checkCodeownersApproval now uses github.paginate() for
listReviews so PRs with >30 reviews don't silently miss an approval
- fix(P2): guard pull_request.number access with optional chaining +
early return when action runs outside a pull_request event
Resolution SummaryResolving findings from Codex Review, Gemini Review:
|
Answers to Gemini's Review QuestionsQ: Is the Axiom dataset A: Q: Why was the fallback logic designed to fake a 0-line report instead of failing explicitly? A: The original intent was to handle kcov LCOV path-resolution crashes gracefully and let the diff-cover step succeed — the assumption was that a crash with 0 parseable diff lines meant no coverage-requiring code was changed. This was a design flaw: it conflated tool crashes with doc-only PRs. The dual-review correctly flagged this as P0. It has been fixed in commit 3dbe0d1 — the fallback now writes a crash sentinel ( |
Dual-Review Summary (Round 2)
Convergence: ❌ Not achieved (0 blocking findings) Rounds: 2 Findings[Codex #1] P2 - [Codex #2] P2 - [Gemini #1] P2 - [Gemini #2] P2 - [Gemini #3] P2 - [Gemini #4] P2 - [Gemini #5] P2 - [Gemini #6] P2 - |
2 participants
Changes
Adds
.github/actions/diff-cover-check/— a composite GitHub Action for enforcing patch coverage threshold on changed lines usingdiff-cover.Files:
action.yml— composite action definitionanalyze-coverage-results.mjs— threshold enforcement,coverage-overridelabel bypass, CODEOWNERS approval, Axiom telemetryrequirements.txt— pinneddiff-cover==10.2.0Callers (after this merges):
andreiships/pistachiorama— replace inline diff-cover steps with cross-repo referenceandreiships/opencode— replace local copy with cross-repo referenceSecurity Fixes Applied (dual-review)
crash_fallback:truesentinel; analyzer fails the build on diff-cover crashes instead of silently passing as doc-onlythresholdandaction_pathmoved to env vars (COVERAGE_THRESHOLD,ACTION_PATH); no${{ }}interpolation insidegithub-scriptJS string@org/teamentries; team slugs can never match reviewer loginscheckCodeownersApprovalusesgithub.paginate()forlistReviewspull_request.numberfor non-PR event contextsDeferred Items
analyze-coverage-results.mjs(no test infra in shared-standards yet)Test Plan
validateCI passesandreiships/shared-ai-standards/.github/actions/diff-cover-check@main