feat(scorecard): add codecov backend module with 7 coverage metrics#3477
feat(scorecard): add codecov backend module with 7 coverage metrics#3477fullsend-ai-coder[bot] wants to merge 4 commits into
Conversation
fullsend-ai-coder
Bot
requested review from
a team,
Eswaraiahsapram,
christoph-jerolimov and
its-mitesh-kumar
as code owners
|
Important This PR includes changes that affect public-facing API. Please ensure you are adding/updating documentation for new features or behavior. Changed Packages
|
|
|
🤖 Finished Review · ✅ Success · Started 1:30 PM UTC · Completed 1:42 PM UTC |
ReviewFindingsHigh
Medium
Low
Previous runReviewFindingsHigh
Medium
Low
Info
Previous run (2)ReviewFindingsHigh
Medium
Low
Previous run (3)ReviewFindingsHigh
Medium
Low
Info
|
![fullsend-ai-review[bot]](https://avatars.githubusercontent.com/in/3478501?s=60&v=4){kind=small}
| updatestamp: string; | ||
| author: CodecovAuthor; | ||
| language: string; | ||
| branch: string; |
There was a problem hiding this comment.
[high] nil/null handling
The CodecovRepoResponse.totals field is typed as non-optional CodecovTotals, but the Codecov API returns totals: null for repositories that have no coverage data uploaded yet. Both calculateMetric and calculateMetrics access repoInfo.totals[field] without any null check, which will throw a TypeError at runtime.
Suggested fix: Change the type to totals: CodecovTotals | null and add a null guard in both calculateMetric and calculateMetrics.
| async getRepoInfo( | ||
| service: string, | ||
| owner: string, | ||
| repo: string, |
There was a problem hiding this comment.
[medium] error-handling-idiom
When a specific account name is requested but not found in configuration, the CodecovClient only logs a warning and returns undefined, silently falling through to an unauthenticated request. The SonarQube module throws an Error in the same scenario.
Suggested fix: Throw an Error when a specific accountName is provided but not found in configuration, matching the SonarQube pattern.
| { id: string; title: string; description: string } | ||
| > = { | ||
| coverage: { | ||
| id: 'codecov.coverage', |
There was a problem hiding this comment.
[low] api-contract
The coverage_trend metric is described as Code coverage trend for the last 7 days but maps to the totals.diff field which represents diff coverage, not a time-based trend.
| id: 'codecov.tracked_lines', | ||
| title: 'Codecov Tracked Lines', | ||
| description: 'Total lines of code tracked by Codecov.', | ||
| }, |
There was a problem hiding this comment.
[low] edge-case
Threshold rules for partial_lines and missed_lines use absolute counts which may not scale well across repos of different sizes.
| getCatalogFilter(): Record<string, string | symbol | (string | symbol)[]> { | ||
| return { | ||
| [`metadata.annotations.${CODECOV_REPO_ANNOTATION}`]: | ||
| CATALOG_FILTER_EXISTS, |
There was a problem hiding this comment.
[low] error-handling-gaps
calculateMetric and calculateMetrics have no additional error context (entity ref, metric ID) around the API call.
|
/fs-fix regenerate the api reports and commit the changes (or new files) |
|
🤖 Finished Fix · ✅ Success · Started 6:45 AM UTC · Completed 6:58 AM UTC |
fullsend-ai-coder
Bot
force-pushed
the
fs/3473-scorecard-codecov-module
branch
from
3cbc239 to
7669647
Compare
Add the missing report.api.md for the new scorecard-backend-module-codecov plugin. Addresses review feedback on #3477
🔧 Fix agent — iteration 1 (human-triggered)Generated the missing API report file (report.api.md) for the new codecov backend module plugin. Fixed (1):
Tests: passed Updated by fullsend fix agent |
|
🤖 Finished Review · ✅ Success · Started 7:01 AM UTC · Completed 7:14 AM UTC |
| private constructor() {} | ||
|
|
||
| static fromConfig(config: Config, logger: LoggerService): MetricProvider[] { | ||
| return CODECOV_METRICS.map(metricId => |
There was a problem hiding this comment.
[medium] efficiency
The factory creates 7 separate CodecovMetricProvider instances, each constructing its own CodecovClient. If the framework calls calculateMetric on individual providers (non-batch path), 7 separate HTTP requests will be made for the same repository data.
Suggested fix: Create a single CodecovClient instance in the factory and share it across all 7 providers.
| if (account) { | ||
| return account.getOptionalString('authToken'); | ||
| } | ||
|
|
There was a problem hiding this comment.
[medium] fail-open
When an entity codecov.io/account annotation references an account name not present in the config, resolveAuthToken() silently returns undefined and the request proceeds without authentication, silently degrading from authenticated to unauthenticated access.
Suggested fix: When an explicit accountName is provided but no matching account is found, throw an error rather than silently falling back to unauthenticated requests.
| }, | ||
| covered_lines: { | ||
| id: 'codecov.covered_lines', | ||
| title: 'Codecov Covered Lines', |
There was a problem hiding this comment.
[low] API-contract-accuracy
The coverage_trend metric description says Code coverage trend for the last 7 days but maps to the diff field, which represents coverage diff from the most recent comparison, not a 7-day window.
| * Represents the full response from the Codecov repos_retrieve API. | ||
| * @see https://docs.codecov.com/reference/repos_retrieve | ||
| */ | ||
| export interface CodecovRepoResponse { |
There was a problem hiding this comment.
[low] null-handling
The coverage field in CodecovTotals is typed as number, but the Codecov API may return coverage: null when there are tracked files but no actual coverage measurements.
| if (serviceAnnotation) { | ||
| service = serviceAnnotation; | ||
| } else if (entity.metadata.annotations?.[GITHUB_PROJECT_SLUG_ANNOTATION]) { | ||
| service = 'github'; |
There was a problem hiding this comment.
[low] insufficient-input-validation
The service value from the codecov.io/service annotation is used in the API URL without validation against known Codecov services. While encodeURIComponent prevents URL manipulation, allowlist validation would be defense-in-depth.
| * The default account name to use when the codecov.io/account annotation is not set. | ||
| * Defaults to "default". | ||
| * @visibility frontend | ||
| */ |
There was a problem hiding this comment.
[low] scope-alignment
The defaultAccount field is marked @visibility frontend, but this is a backend-only module with no frontend counterpart.
|
/fs-fix Add a readme, with installation instructions, notes on the necessary annotation and table with all metrics |
|
🤖 Finished Fix · ✅ Success · Started 8:03 AM UTC · Completed 8:07 AM UTC |
Add a new scorecard-backend-module-codecov plugin that integrates with the Codecov API to provide 7 code coverage metrics: coverage percentage, coverage trend, tracked files, tracked lines, covered lines, partial lines, and missed lines. The module supports multi-account configuration with optional auth tokens and resolves service/owner/repo from entity annotations with fallback logic. All 7 metrics share a single API call via the batch provider pattern. Closes #3473 Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Add the missing report.api.md for the new scorecard-backend-module-codecov plugin. Addresses review feedback on #3477
fullsend-ai-coder
Bot
force-pushed
the
fs/3473-scorecard-codecov-module
branch
from
baa2e8b to
331e7e5
Compare
…odule Add README.md with installation instructions, entity annotation documentation, metrics table, and multi-account configuration guide. Set package.json version to 0.0.0 for initial release. Add changeset for the new codecov module. Addresses review feedback on #3477
🔧 Fix agent — iteration 2 (human-triggered)Addressed all 3 human-requested changes: added comprehensive README with metrics table, installation instructions, and annotation docs; set package version to 0.0.0; added changeset. Fixed (3):
Tests: passed Updated by fullsend fix agent |
|
🤖 Finished Review · ✅ Success · Started 8:09 AM UTC · Completed 8:18 AM UTC |
| const { service, owner, repo, accountName } = | ||
| resolveCodecovEntityInfo(entity); | ||
| const repoInfo = await this.client.getRepoInfo( | ||
| service, |
There was a problem hiding this comment.
[high] nil-deref
Both calculateMetric and calculateMetrics access repoInfo.totals[field] without null-checking totals. The Codecov repos_retrieve API returns totals: null for repositories without coverage uploads. At runtime repoInfo.totals will be null and property access will throw TypeError.
Suggested fix: Change the totals field in CodecovRepoResponse to totals: CodecovTotals | null, and add a null guard in both calculateMetric and calculateMetrics.
| /** | ||
| * Maps scorecard metric IDs to the field in the Codecov API totals response. | ||
| */ | ||
| export const CODECOV_TOTALS_FIELD_MAP: Record< |
There was a problem hiding this comment.
[medium] api-contract
The coverage_trend metric is described as Code coverage trend for the last 7 days but maps to totals.diff which represents the coverage diff of the most recent commit, not a 7-day trend.
Suggested fix: Update the description to accurately reflect what totals.diff represents or use a different Codecov API endpoint for time-series data.
| `Codecov API error: ${response.status} ${response.statusText} for ${url}`, | ||
| ); | ||
| } | ||
| return response.json() as Promise<CodecovRepoResponse>; |
There was a problem hiding this comment.
[low] error-handling
getRepoInfo calls response.json() without a try-catch. A 200 response with non-JSON body will produce a confusing error.
Suggested fix: Wrap response.json() in a try-catch and throw a descriptive error.
| @@ -0,0 +1,262 @@ | |||
| /* | |||
There was a problem hiding this comment.
[low] test-inadequate
No test coverage for null totals response or resolveCodecovEntityInfo throwing. Tests only exercise the happy path.
| id: 'codecov.coverage_trend', | ||
| title: 'Codecov Coverage Trend (7d)', | ||
| description: 'Code coverage trend for the last 7 days.', | ||
| }, |
There was a problem hiding this comment.
[info] edge-case
CODECOV_TOTALS_FIELD_MAP type includes redundant diff union since diff is already a key of CodecovTotals.
|
/fs-fix run prettier 😆 |
|
🤖 Finished Fix · ✅ Success · Started 8:49 AM UTC · Completed 8:59 AM UTC |
Format index.ts and README.md with prettier to fix formatting inconsistencies (import line wrapping, markdown table alignment). Addresses review feedback on #3477
🔧 Fix agent — iteration 3 (human-triggered)Ran prettier on all PR-changed files per human instruction. 2 files were reformatted: packages/backend/src/index.ts and plugins/scorecard-backend-module-codecov/README.md. Fixed (1):
Tests: passed Updated by fullsend fix agent |
|
|
🤖 Finished Review · ✅ Success · Started 9:03 AM UTC · Completed 9:14 AM UTC |
| /** | ||
| * Represents the full response from the Codecov repos_retrieve API. | ||
| * @see https://docs.codecov.com/reference/repos_retrieve | ||
| */ |
There was a problem hiding this comment.
[high] nil/null handling
The totals field in CodecovRepoResponse is typed as non-nullable CodecovTotals, but the Codecov repos_retrieve API can return totals: null for repositories that exist but have never uploaded coverage data. When totals is null, both calculateMetric and calculateMetrics in CodecovMetricProvider will crash with Cannot read properties of null when accessing repoInfo.totals[field].
Suggested fix: Change the type to totals: CodecovTotals | null and add a null check in CodecovMetricProvider.calculateMetric and calculateMetrics before accessing totals fields. Return a sensible default (e.g., 0) or throw a descriptive error when totals is null.
| tracked_files: { | ||
| id: 'codecov.tracked_files', | ||
| title: 'Codecov Tracked Files', | ||
| description: 'Number of files tracked by Codecov.', |
There was a problem hiding this comment.
[medium] API behavior claims
The coverage_trend metric is described as 'Code coverage trend for the last 7 days' and mapped to totals.diff. However, the Codecov repos_retrieve API's totals.diff field represents the diff coverage of the latest commit/comparison, not a 7-day rolling trend. The README and the metric description both make this incorrect claim.
Suggested fix: Update the description to accurately reflect what totals.diff represents (e.g., 'Diff coverage for the latest comparison'), or use a different Codecov API endpoint that actually provides a time-based trend.
| const repoInfo = await this.client.getRepoInfo( | ||
| service, | ||
| owner, | ||
| repo, |
There was a problem hiding this comment.
[low] error handling gaps
If the Codecov API returns a response where specific totals fields are null, the returned value will be null cast as number, silently passing invalid data downstream. This is largely addressed by the null-handling fix for CodecovRepoResponse.totals.
| coverage: 'coverage', | ||
| coverage_trend: 'diff', | ||
| tracked_files: 'files', | ||
| tracked_lines: 'lines', |
There was a problem hiding this comment.
[low] edge cases
The threshold rules for partial_lines and missed_lines use absolute numeric boundaries (<10, 10-50, >50) that are not meaningful as universal defaults. A large project would reasonably have more than 50 missed lines even with excellent coverage.
| }, | ||
| covered_lines: { | ||
| id: 'codecov.covered_lines', | ||
| title: 'Codecov Covered Lines', |
There was a problem hiding this comment.
[low] logic errors
The type of CODECOV_TOTALS_FIELD_MAP includes | 'diff' but diff is already a member of keyof CodecovTotals. The redundant union can be simplified.
| @@ -0,0 +1,262 @@ | |||
| /* | |||
There was a problem hiding this comment.
[low] test adequacy
Tests do not cover the case where the Codecov API returns totals: null or where individual fields within totals are null — the most likely failure scenarios in production.
| this.logger = logger.child({ component: 'CodecovClient' }); | ||
| } | ||
|
|
||
| private resolveAuthToken(accountName?: string): string | undefined { |
There was a problem hiding this comment.
[low] Fail-Open Authentication
When a specific account name is requested via annotation but not found in config, resolveAuthToken() returns undefined and the API call proceeds without authentication. A warning is logged, but the silent fallback could mask configuration errors for private repos.
4 participants
Add a new scorecard-backend-module-codecov plugin that integrates with the Codecov API to provide 7 code coverage metrics: coverage percentage, coverage trend, tracked files, tracked lines, covered lines, partial lines, and missed lines. The module supports multi-account configuration with optional auth tokens and resolves service/owner/repo from entity annotations with fallback logic. All 7 metrics share a single API call via the batch provider pattern.
Co-Authored-By: Claude Opus 4.6 noreply@anthropic.com
Closes #3473
Post-script verification
fs/3473-scorecard-codecov-module)7ccaff17753df64c7ab288cdcba34cee5a657254..HEAD)