feat(flags): Implement codelyze flags#826
Conversation
There was a problem hiding this comment.
Pull request overview
Adds support for “flags” to the Codelyze coverage GitHub Action so teams can upload/report multiple coverage streams (e.g., unit vs e2e) independently, including per-flag commit statuses and a PR coverage summary comment.
Changes:
- Introduces a new
flagaction input and passes it through to the backend coverage upload. - Posts per-flag commit statuses by fetching flag summaries from the backend.
- Adds PR comment upsert logic to show overall + per-flag coverage in a single comment.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| src/main.ts | Reads new flag input and forwards it into coverage processing. |
| src/coverage.ts | Sends flag/parentShas to backend; adds per-flag statuses + PR comment upsert. |
| src/comment.ts | New module to fetch flag summaries and upsert a PR comment with a marker. |
| src/codelyze.ts | Extends upload payload to include flag and parentShas. |
| action.yml | Documents and exposes the new flag input. |
| README.md | Documents how to use flags and adds required permissions for PR comments. |
| .github/workflows/ci.yml | Updates workflow permissions and example usage to include flag. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // GITHUB_REF=refs/pull/123/merge or GITHUB_EVENT_PATH payload | ||
| const ref = process.env.GITHUB_REF ?? '' | ||
| const match = ref.match(/refs\/pull\/(\d+)\//) | ||
| return match ? Number(match[1]) : undefined |
There was a problem hiding this comment.
The comment says this supports reading the PR number from GITHUB_EVENT_PATH, but the implementation only parses GITHUB_REF. Either update the comment to match reality or implement the GITHUB_EVENT_PATH fallback to avoid confusion for future maintainers.
| const params = new URLSearchParams({ token, commit, branch }) | ||
| const res = await fetch( | ||
| `https://api.codelyze.com/v1/projects/coverage/flags?${params}` |
There was a problem hiding this comment.
fetchFlagSummaries puts the codelyze upload token into the URL query string. Query strings are more likely to be captured in intermediary/proxy logs and error reports than headers or POST bodies. Prefer sending the token in an Authorization header (or request body) if the API supports it, and consider adding a request timeout (e.g., via AbortSignal.timeout) so the action can’t hang indefinitely on a stalled network call.
| const params = new URLSearchParams({ token, commit, branch }) | |
| const res = await fetch( | |
| `https://api.codelyze.com/v1/projects/coverage/flags?${params}` | |
| const params = new URLSearchParams({ commit, branch }) | |
| const res = await fetch( | |
| `https://api.codelyze.com/v1/projects/coverage/flags?${params}`, | |
| { | |
| headers: { | |
| Authorization: `Bearer ${token}` | |
| }, | |
| signal: AbortSignal.timeout(10000) | |
| } |
| const rows = flags | ||
| .map((f) => { | ||
| const rate = f.linesFound > 0 ? f.linesHit / f.linesFound : 0 | ||
| const cf = f.carryforward ? ' _(cf)_' : '' | ||
| return `| \`${f.flagName}\`${cf} | ${f.linesHit}/${f.linesFound} | ${percentString(rate)} |` | ||
| }) |
There was a problem hiding this comment.
flagName is interpolated directly into a Markdown table cell. If a flag name contains characters like | or backticks/newlines, it can break the table formatting and allows Markdown injection (e.g., unwanted mentions/links) in the PR comment. Consider escaping Markdown-sensitive characters (at least |, `, and newlines) before rendering, or validate/sanitize flag names to a safe character set.
| await upsertPrComment({ | ||
| octokit, | ||
| context, | ||
| token, | ||
| commit: sha, | ||
| branch: ref?.replace('refs/heads/', '') ?? '', | ||
| overallHit: summary.lines.hit, | ||
| overallFound: summary.lines.found, | ||
| diff | ||
| }).catch((err) => core.debug(`PR comment failed: ${err}`)) | ||
|
|
There was a problem hiding this comment.
This new block adds external API calls (fetchFlagSummaries) and PR comment upserts. The repo has Jest coverage for coverage.ts, but there are no tests asserting this new behavior (and the unmocked fetch/Octokit calls can introduce network dependency or unexpected throws during tests). Please add/update tests to mock the comment module and verify: (1) per-flag statuses are posted with the expected contexts, and (2) PR comment upsert is invoked only when appropriate.
| await upsertPrComment({ | |
| octokit, | |
| context, | |
| token, | |
| commit: sha, | |
| branch: ref?.replace('refs/heads/', '') ?? '', | |
| overallHit: summary.lines.hit, | |
| overallFound: summary.lines.found, | |
| diff | |
| }).catch((err) => core.debug(`PR comment failed: ${err}`)) | |
| const isPullRequestEvent = | |
| github.context.eventName === 'pull_request' || | |
| github.context.eventName === 'pull_request_target' | |
| const hasPullRequest = Boolean( | |
| (github.context.payload as { pull_request?: unknown }).pull_request | |
| ) | |
| if (isPullRequestEvent && hasPullRequest) { | |
| await upsertPrComment({ | |
| octokit, | |
| context, | |
| token, | |
| commit: sha, | |
| branch: ref?.replace('refs/heads/', '') ?? '', | |
| overallHit: summary.lines.hit, | |
| overallFound: summary.lines.found, | |
| diff | |
| }).catch((err) => core.debug(`PR comment failed: ${err}`)) | |
| } |
| const patchThreshold = | ||
| Number.parseFloat(core.getInput('patch-threshold')) || 0 | ||
| const emptyPatch = core.getBooleanInput('skip-empty-patch') ?? false | ||
| const flag = core.getInput('flag') || undefined |
There was a problem hiding this comment.
flag is read without trimming whitespace. Since this value becomes part of the upload payload/status context, leading/trailing spaces can easily produce unexpected “different” flags. Consider using core.getInput('flag', { trimWhitespace: true }) and/or normalizing to a safe identifier format before passing it through.
| const comments = await octokit.rest.issues.listComments({ | ||
| owner: context.owner, | ||
| repo: context.repo, | ||
| issue_number: prNumber | ||
| }) | ||
|
|
||
| const existing = comments.data.find((c) => c.body?.includes(MARKER)) |
There was a problem hiding this comment.
listComments only returns the first page (30 comments by default). On PRs with lots of discussion, the existing codelyze marker comment may be on a later page, causing this to create duplicate comments instead of updating. Consider using octokit.paginate (or explicitly paginating with per_page: 100 and page iteration) when searching for the marker comment.
| const comments = await octokit.rest.issues.listComments({ | |
| owner: context.owner, | |
| repo: context.repo, | |
| issue_number: prNumber | |
| }) | |
| const existing = comments.data.find((c) => c.body?.includes(MARKER)) | |
| const comments = await octokit.paginate(octokit.rest.issues.listComments, { | |
| owner: context.owner, | |
| repo: context.repo, | |
| issue_number: prNumber, | |
| per_page: 100 | |
| }) | |
| const existing = comments.find((c) => c.body?.includes(MARKER)) |
Coverage report for f1ebe7c
cf = carried forward from a previous commit |
97b1aa3 to
f1ebe7c
Compare
No description provided.