-
Notifications
You must be signed in to change notification settings - Fork 1.6k
feat: OSS SAST set up #1398
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
feat: OSS SAST set up #1398
Conversation
|
@modelcontextprotocol/client
@modelcontextprotocol/server
@modelcontextprotocol/express
@modelcontextprotocol/hono
@modelcontextprotocol/node
commit: |
felixweinberger
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for your contribution to the MCP TypeScript SDK!
Suggestion: REQUEST_CHANGES
Comment: This PR adds valuable infrastructure for test coverage and SAST analysis. However, there are several issues that need to be addressed before merging:
Critical Issues
1. Missing SONAR_TOKEN Secret Setup
The sast job is currently skipping on this PR because it requires a SONAR_TOKEN secret that doesn't exist yet. The PR description should document the setup steps required by maintainers to configure:
- Creating a SonarCloud project
- Adding the
SONAR_TOKENrepository secret
Without this, the new CI job will silently skip on all PRs.
2. Hardcoded Project Version
In sonar-project.properties:
sonar.projectVersion=2.0.0-alpha.0
This version will become stale immediately. Consider either:
- Removing this line (SonarCloud can work without it)
- Dynamically reading it from
package.jsonin the workflow
3. Missing fetch-depth: 0 for SonarCloud
The SonarCloud documentation recommends using fetch-depth: 0 in the checkout step for proper blame information:
- uses: actions/checkout@v6
with:
fetch-depth: 04. sast Job Not in publish Dependencies
The publish job depends only on [build, test] but not sast. If SAST analysis is intended to be a quality gate, consider adding it to the dependencies:
needs: [build, test, sast]Minor Issues
5. Reports Directory Potentially Created in Wrong Location
The sonar reporter outputs to reports/sonar-report.xml (relative to package), but the sonar-project.properties expects it at ./reports/sonar-report.xml (relative to root). Verify this works correctly when running from the root.
6. Coverage Includes Examples but SonarCloud Excludes Them
The root vitest.config.js includes examples/**/src/**/*.ts in coverage, but sonar-project.properties only sources from packages/. This is inconsistent—either include examples in SonarCloud or exclude them from coverage collection.
Questions
-
Has the SonarCloud project been created at https://sonarcloud.io for
modelcontextprotocol_typescript-sdk? -
Is there a plan for setting up the
SONAR_TOKENsecret? This typically requires org admin access. -
Why add
vitest-sonar-reporterto the shared vitest config rather than only in the root config? Individual packages runningtest:coveragelocally won't benefit from SonarCloud reporting.
The coverage infrastructure itself looks well-designed. The separation of per-package and root-level coverage is a good pattern. Please address the issues above and this will be ready for approval.
This PR introduces test coverage scripts in
package.jsonfiles that generates merged reports across all packages in the monorepo, along with SonarCloud integration for static application security testing (SAST) and code quality analysis. The newtest:coverage:allcommand runs tests from the root and produces a single coverage report combining results from all packages, while individual packages can still generate their own coverage via test:coverageMotivation and Context
Test Coverage Infrastructure:
Previously, there was no way to generate a unified coverage report across the monorepo. Each package could run tests independently, but aggregating coverage metrics for the entire codebase required manual effort. This change enables:
SAST/SonarCloud Integration:
Adds static application security testing via SonarCloud to the CI/CD pipeline. This provides:
How Has This Been Tested?
./coverage/pnpm --filter <package> test:coverageBreaking Changes
None. All changes are additive.
Types of changes
Checklist
Additional context
New scripts:
test:coverage:all(root) — runs all tests with merged coveragetest:coverage(per package) — runs coverage for individual packageConfiguration changes:
New dependencies:
@vitest/coverage-v8— V8 coverage provider@modelcontextprotocol/vitest-config— added to root for config inheritanceSonarCloud setup:
sonar-project.propertieswith project configuration