fix(#3532): standardize metric IDs from snake_case to lowerCamelCase#3534
fix(#3532): standardize metric IDs from snake_case to lowerCamelCase#3534fullsend-ai-coder[bot] wants to merge 1 commit 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
|
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #3534 +/- ##
=======================================
Coverage 54.28% 54.28%
=======================================
Files 2346 2346
Lines 89642 89644 +2
Branches 25056 25056
=======================================
+ Hits 48662 48664 +2
Misses 40744 40744
Partials 236 236
*This pull request uses carry forward flags. Click here to find out more. Continue to review full report in Codecov by Harness.
🚀 New features to boost your workflow:
|
|
🤖 Finished Review · ✅ Success · Started 5:09 PM UTC · Completed 5:22 PM UTC |
Review — PR #3534Verdict: Request changes SummaryThis PR renames all scorecard metric and provider IDs from However, the OpenSSF module's dynamic ID generation regex produces incorrect IDs for 3 out of 18 metrics, and the tests fail to catch this because they either mirror the same buggy regex or don't cover the problematic inputs. Findings🔴 HIGH — logic-error: OpenSSF ID generation regex fails for all-caps metric namesFile: The new regex in .replace(/-([a-zA-Z])/g, (_, c) => c.toUpperCase())
.replace(/^[A-Z]/, c => c.toLowerCase())only lowercases the first character of the string, which is incorrect when the OpenSSF check name starts with multiple uppercase letters or is fully uppercase. This produces IDs that don't match the README documentation:
Remediation: Either apply .toLowerCase()
.replace(/-([a-z])/g, (_, c) => c.toUpperCase())Or change the second replace to lowercase all leading uppercase characters: 🔴 HIGH — test-integrity: Bulk provider ID assertion is a tautologyFile: The bulk assertion that validates all 18 provider IDs computes expected values using the exact same regex as the production code: const expectedProviderIds = OPENSSF_METRICS.map(metric => {
const normalizedName = metric.name
.replace(/-([a-zA-Z])/g, (_, c) => c.toUpperCase())
.replace(/^[A-Z]/, c => c.toLowerCase());
return `openssf.${normalizedName}`;
});This test will always pass regardless of whether the output is correct — it validates the buggy behavior rather than the intended behavior. The test should assert against hardcoded expected values. Remediation: Replace the computed 🟡 MEDIUM — test-inadequate: Missing edge case tests for uppercase acronym inputsFile: The unit test only validates normalization for Remediation: Add test cases for 🟡 LOW — tier-mismatch: "fix" prefix on a breaking changeThe PR title uses a Dimensions reviewed
Previous runReviewFindingsHigh
Info
|
![fullsend-ai-review[bot]](https://avatars.githubusercontent.com/in/3478501?s=60&v=4){kind=small}
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
|
/fs-fix rename |
|
🤖 Fix · ❌ Terminated · Started 9:09 AM UTC · Ended 10:44 AM UTC |
Rename all Scorecard metric and provider IDs from snake_case to lowerCamelCase to align with the app-config.yaml naming convention and the planned Scorecard design. Key changes: - Provider ID definitions (e.g. github.open_prs -> github.openPrs) - SonarQube metric config keys and type members - OpenSSF dynamic ID generation (hyphen-to-camelCase conversion) - Translation keys in ref.ts and all 5 locale files - Config schema (config.d.ts) and YAML config keys - All test fixtures, e2e tests, and documentation - MetricProvidersRegistry error message format SonarQube API metric keys (e.g. security_rating, code_smells) are preserved as-is since they are external API field names. This is a breaking change for existing configurations that reference metric IDs by name. Closes #3532
fullsend-ai-coder
Bot
force-pushed
the
agent/3532-snake-case-to-camel-case
branch
from
a97e9fd to
60e9a1b
Compare
|
|
🤖 Finished Fix · ❌ Failure · Started 9:09 AM UTC · Completed 10:44 AM UTC |
|
🤖 Finished Review · ✅ Success · Started 10:47 AM UTC · Completed 11:03 AM UTC |
| const normalizedName = this.getMetricName() | ||
| .toLowerCase() | ||
| .replace(/-/g, '_'); | ||
| .replace(/-([a-zA-Z])/g, (_, c) => c.toUpperCase()) |
There was a problem hiding this comment.
[high] logic-error
The new camelCase conversion regex fails for OpenSSF check names starting with consecutive uppercase letters. replace(/^[A-Z]/, c => c.toLowerCase()) only lowercases the first character, producing incorrect IDs: CII-Best-Practices → cIIBestPractices (should be ciiBestPractices), CI-Tests → cITests (should be ciTests), SAST → sAST (should be sast). 3 out of 18 metrics produce IDs that don't match the README documentation.
Suggested fix: Apply .toLowerCase() first, then convert hyphens to camelCase: .toLowerCase().replace(/-([a-z])/g, (_, c) => c.toUpperCase()). Alternatively, change the second replace to lowercase all leading uppercase characters: .replace(/^[A-Z]+/, m => m.toLowerCase()). Update the test to match.
| const providerIds = providers.map(provider => provider.getProviderId()); | ||
| const expectedProviderIds = OPENSSF_METRICS.map(metric => { | ||
| const normalizedName = metric.name.toLowerCase().replace(/-/g, '_'); | ||
| const normalizedName = metric.name |
There was a problem hiding this comment.
[high] test-integrity
The bulk assertion that validates all 18 provider IDs computes expected values using the exact same (buggy) regex as the production code. This is a tautological test that will always pass regardless of whether the output is correct, masking the bug where CII-Best-Practices produces cIIBestPractices instead of the documented ciiBestPractices.
Suggested fix: Replace the computed expectedProviderIds with a hardcoded array of all 18 expected provider IDs (e.g., ['openssf.binaryArtifacts', 'openssf.branchProtection', 'openssf.ciiBestPractices', 'openssf.ciTests', ...]). This would catch the uppercase acronym bug.
| it('normalizes hyphenated check names for provider id', () => { | ||
| const provider = new OpenSSFMetricProvider(hyphenatedCheckConfig); | ||
| expect(provider.getProviderId()).toBe('openssf.code_review'); | ||
| expect(provider.getProviderId()).toBe('openssf.codeReview'); |
There was a problem hiding this comment.
[medium] test-inadequate
The unit test only validates normalization for Code-Review (simple case) and Maintained (no hyphen). It does not test any of the three problematic inputs with consecutive uppercase letters: CII-Best-Practices, CI-Tests, or SAST.
Suggested fix: Add test cases for CII-Best-Practices (expected: openssf.ciiBestPractices), CI-Tests (expected: openssf.ciTests), and SAST (expected: openssf.sast) to verify correct handling of uppercase acronyms.
1 participant
Rename all Scorecard metric and provider IDs from snake_case to lowerCamelCase to align with the app-config.yaml naming convention and the planned Scorecard design.
Key changes:
SonarQube API metric keys (e.g. security_rating, code_smells) are preserved as-is since they are external API field names.
This is a breaking change for existing configurations that reference metric IDs by name.
Closes #3532
Post-script verification
agent/3532-snake-case-to-camel-case)ed84b21fc3f37da0e0d2ffb05b78432d11ec7ce6..HEAD)