CLI-321 introduce 'run mcp' command#223
CLI-321 introduce 'run mcp' command#223sophio-japharidze-sonarsource wants to merge 2 commits intomasterfrom
Conversation
SummaryThis PR introduces a new hidden What changed:
Why: What reviewers should knowStart here: Review Key design decisions:
Integration points:
Test coverage:
Watch for:
|
| }; | ||
|
|
||
| if (projectInfo.hasSonarProps && projectInfo.sonarPropsData) { | ||
| print('Found sonar-project.properties'); |
There was a problem hiding this comment.
This change is needed so that project discovery does not pollute stdio for mcp
e6e028c to
94ce861
Compare
94ce861 to
38432f5
Compare
nquinquenel
left a comment
There was a problem hiding this comment.
Looks good overall, few comments
| for (const configSource of project.configSources) { | ||
| print(`Found ${configSource}`); | ||
| } |
There was a problem hiding this comment.
Is this wanted in scope of this PR? Was it for debug only?
There was a problem hiding this comment.
No, it's needed. Because before this print line was inside discoverProject method that is now also called from runMcp. And any print lines would interfere with the stdio.
Additionally, these prints do not belong in the discoverProject as it is an utility method that should not affect the CLI UX.
| child.on('error', reject); | ||
| child.on('exit', (code) => { | ||
| process.exitCode = code ?? 1; | ||
| resolve(); |
There was a problem hiding this comment.
I think it should not resolve() for non 0 exit code, maybe reject with a CommandFailedError message?
There was a problem hiding this comment.
Rejecting here would add another redundant error message to stdout (through generic CommandFailedError handler). When container exists with non-0 exit code, this command will exit too, with the same exit code as the continer.
| if (auth.connectionType === 'cloud') { | ||
| args.push('-e', 'SONARQUBE_ORG'); | ||
| env.SONARQUBE_ORG = org ?? ''; | ||
| } |
There was a problem hiding this comment.
If we want to optimize, SONARQUBE_URL is not needed in scope of SQC (it defaults to sonarcloud.io)
There was a problem hiding this comment.
I think it's ok to keep it because like this we will always explicitly set the URL and will not need extra checks saying something like isCloud && region==US, etc.
|
|
||
| if (context.withFsMount) { | ||
| const hostPath = normalizePath(context.projectRoot); | ||
| args.push('-v', `${hostPath}:/app/mcp-workspace:ro`); |
There was a problem hiding this comment.
I know it's not ideal but CAG requires to have :rw access (write included). Worth checking what we want to do, it's maybe not a good practice to put a write access via a wrapper command that is doing opaque things.
|
|
||
| if (context.withFsMount) { | ||
| const hostPath = normalizePath(context.projectRoot); | ||
| args.push('-v', `${hostPath}:/app/mcp-workspace:ro`); |
There was a problem hiding this comment.
I know it's not ideal but CAG requires to have :rw access (write included). Worth checking what we want to do, it's maybe not a good practice to put a write access via a wrapper command that is doing opaque things.
|
|
||
| if (auth.connectionType === 'cloud') { | ||
| args.push('-e', 'SONARQUBE_ORG'); | ||
| env.SONARQUBE_ORG = org ?? ''; |
There was a problem hiding this comment.
I think it should probably fail if we don't have an org defined. Currently, we won't be able to start the server properly.
There was a problem hiding this comment.
I have changed auth-resolver to NOT resolve auth if connection type is cloud and org key is not defined.
| * Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301, USA. | ||
| */ | ||
|
|
||
| // Integration tests for `sonar run mcp` |
There was a problem hiding this comment.
Should we test read only/toolsets options here?
a28dfd0 to
06b529e
Compare
|
There was a problem hiding this comment.
Solid implementation. The key architectural moves — extracting getMcpServerConfig into a shared lib, and refactoring discoverProject to return configSources instead of printing directly — are clean and well-motivated. The dedicated integration test asserting stdout cleanliness for MCP JSON-RPC transport is exactly the right thing to test. One uncovered path in auth-resolver.ts is worth a test before this is in production.
| if (connectionType === 'cloud' && orgKey === undefined) { | ||
| return null; | ||
| } |
There was a problem hiding this comment.
Missing test coverage: No test exercises the path where a saved connection has type: 'cloud' but orgKey is undefined, which makes resolveFromState return null. All existing auth-resolver tests supply orgKey: 'my-org'.
This guard prevents a malformed cloud state entry from producing auth — a regression here would silently allow the downstream code to pass an empty SONARQUBE_ORG env var to the container. A unit test against resolveFromState with a state fixture of { type: 'cloud', serverUrl: '...', /* no orgKey */ } would cover it.
- Mark as noise



No description provided.