Repository navigation
Update ADO and Sonar MCP servers to use cloud URLs - #96
Conversation
There was a problem hiding this comment.
🟡 Changes recommended
SonarCloud project discovery needs organization scoping, and .NET scanning instructions need the correct organization argument.
2 open issues
What changed in this PR
Updates RAVEN’s ADO and Sonar MCP servers to default to cloud endpoints while preserving on-premises URL overrides.
Changes:
- Adds cloud defaults and ADO organization selection.
- Adds ADO project pagination and configuration tests.
- Updates setup prompts and configuration documentation.
| File | Description |
|---|---|
| scripts/setup-credentials.ps1 | Updates Windows setup prompts. |
| scripts/setup-credentials-mac.mjs | Updates macOS setup prompts. |
| README.md | Documents cloud configuration and scanning. |
| packages/sonar-mcp/src/server.ts | Defaults to SonarCloud. |
| packages/sonar-mcp/src/__tests__/server.test.ts | Checks explicit URL overrides. |
| packages/sonar-mcp/src/__tests__/cloud-url.test.ts | Tests the SonarCloud default. |
| packages/ado-mcp/src/server.ts | Adds cloud-aware organization handling. |
| packages/ado-mcp/src/ado-client.ts | Adds cloud detection and project pagination. |
| packages/ado-mcp/src/__tests__/server.test.ts | Tests Cloud and Server configuration. |
| packages/ado-mcp/src/__tests__/ado-client.test.ts | Tests organization URLs and pagination. |
| docs/TOOL_INVENTORY.md | Updates environment-variable requirements. |
| docs/SYSTEM_DESIGN_AND_ARCHITECTURE.md | Documents cloud destinations and defaults. |
| .github/copilot-instructions.md | Updates MCP configuration guidance. |
Review effort: Balanced
- Refactor server.ts and sonar-client.ts for better URL management. - Enhance test cases in cloud-url.test.ts and sonar-client.test.ts.
|
@jamesgagan - over to you for review and merge |
jamesgagan
left a comment
There was a problem hiding this comment.
PR Review: #96
Head SHA: a029d73452d61367f1b35f462e989dcfa4c41d7e
Scope: Full review, all 16 changed files. Copilot's two inline comments on the first commit were checked and are resolved in code.
Tier: Deep, set by scripts/setup-credentials* and the two credential-sending clients · Round: 1 · Decoy: rejected 5/5
TL;DR
18 findings: 2 Important, 5 Nit, 11 Pre-existing. The URL building, pagination and tests are correct. The blocker is a design choice: a missing ADO_BASE_URL or SONARQUBE_URL now silently sends the stored credential to a public SaaS host instead of failing, and the security docs still promise the opposite.
About two hours to fix, mostly doc edits plus one guard and one test.
Required changes
-
Make the Cloud destination explicit. Restore the
is not seterrors atpackages/ado-mcp/src/server.ts:68andpackages/sonar-mcp/src/server.ts:20and show the Cloud URLs uncommented in the README. If you keep the defaults instead, add a token-without-URL rejection to both setup scripts' validation blocks and make the server error name the resolved host. -
Add a test for
ADO_PATplusADO_DEFAULT_COLLECTIONwith noADO_BASE_URL, asserting whichever behaviour you choose in item 1. The current tests only assert the fallback as intended. -
Fix the compliance text that the defaults contradict:
.github/copilot-instructions.md:142anddocs/SYSTEM_DESIGN_AND_ARCHITECTURE.mdlines 628, 632 and 676. State that Azure DevOps Services and SonarQube Cloud are outside BC Gov infrastructure, are opt-in, and need a data-residency check. SonarQube Cloud has EU and US regions only.
Things that look scary but I verified are fine
-
Both Copilot comments are resolved.
organizationis required and forwarded on every page for Cloud, ignored on Server, with tests. The README and instructions document/o:for .NET and the scanner passes it through unchanged. The upstream .NET scanner source confirms it rejects/d:sonar.organization. -
The
$toprewrite. The old hand-built URL existed to keep$topunencoded. The new path keeps it literal on page one and page two, and tests pin the exact URLs. -
Guard ordering. Every one of the 16 ADO tools calls
getClient()beforedefaultCollection(), so the Cloud check always sees a live client. The new test covers the first-call case. -
organization: undefinedon Server. The Sonar request helper drops undefined params, so nothing is sent as the string"undefined". -
No prior fix is reverted. Blame shows both removed checks date from the initial import with no recorded rationale. PR #65's security remediation did not touch endpoints. CI, including the inventory drift gate, is green.
Issue 1 — A missing URL now sends the credential to a third-party host [Important]
Code references:
- [ADO server.ts L66-L75]()
raven/packages/ado-mcp/src/server.ts
Lines 66 to 75 in a029d73
- [Sonar server.ts L18-L22]()
raven/packages/sonar-mcp/src/server.ts
Lines 18 to 22 in a029d73
- if (!baseUrl) throw new Error("ADO_BASE_URL is not set. Add it to ~/.raven/.env.");
+ const baseUrl = process.env["ADO_BASE_URL"] || "https://dev.azure.com";
- if (!url) throw new Error("SONARQUBE_URL is not set (see scripts/setup-credentials.ps1)");
+ return process.env.SONARQUBE_URL || "https://sonarcloud.io";Steelman of the author's choice: Cloud-first is the PR's stated purpose. A token is still required, the setup prompts disclose the default, and an on-prem PAT is useless at dev.azure.com.
Why it still matters: This team runs both systems on-prem. A blank answer at the URL prompt, a typo in the variable name, a keychain migration that dropped one key, or a locked keychain now sends a live PAT, collection and project names, and write bodies to Microsoft, or a live Sonar token to SonarSource. sonar_run_scan puts the token and the cloud URL on the scanner command line. Error text never names the host, so the user cannot tell where the credential went. Every other server that needs a URL refuses to start without one, and both setup scripts already enforce URL-with-token for Jarvis. The section header still says "leave blank to skip", which no longer means skip.
Recommendation: See Required changes 1 and 2. If the defaults stay, add if (record.ADO_PAT && !record.ADO_BASE_URL?.trim()) and the Sonar twin to scripts/setup-credentials-mac.lib.mjs:48-56 and the matching block at scripts/setup-credentials.ps1:157-164.
Other sites with the same pattern: packages/github-mcp/src/server.ts:45 defaults to api.github.com, pre-existing, but it is paired with a mandatory repository allowlist and an HTTPS check. packages/sharepoint-mcp/src/sharepoint-client.ts:14 and packages/auth/src/cookie-cache.ts:20 fall back to non-resolving placeholder hosts, which is the repo's convention. Jenkins, Artifactory, RFC Buddy and Jarvis throw. Searched: _URL ... || / ?? across packages/*/src.
Verified: 5/5.
Issue 2 — The security and FOIPPA statements are now false [Important]
Code references:
- [copilot-instructions.md L141-L143]()
raven/.github/copilot-instructions.md
Lines 141 to 143 in a029d73
- [SYSTEM_DESIGN_AND_ARCHITECTURE.md L675-L677]()
raven/docs/SYSTEM_DESIGN_AND_ARCHITECTURE.md
Lines 675 to 677 in a029d73
RAVEN's MCP servers connect only to your configured BC Gov upstreams ... they add no third-party data sinks beyond those.
FOIPPA s.30.1 (Storage in Canada) | All enterprise data stays on BC Gov infrastructure.
Steelman of the author's choice: Line 628 was updated to name the cloud products "when configured", and a token is still an explicit act.
Why it still matters: Line 142 is an absolute with no such reading, and line 676 is the compliance table used in security walkthroughs. The PR added no residency caveat, no PIA or STRA note, and did not add these endpoints beside Jarvis in the data-residency callout. Line 632 says RAVEN "cannot phone home to an unconfigured host", which is exactly what the defaults do.
Recommendation: See Required change 3. If the defaults are dropped, this reduces to line 628 plus a residency note on 676.
Other sites with the same pattern: docs/SYSTEM_DESIGN_AND_ARCHITECTURE.md:249 and :255 ("fixed, configured endpoints"), pre-existing wording, weaker because the hosts are still fixed in code. docs/TOOL_INVENTORY.md:131-137 has the egress callout for Jarvis only.
Verified: 5/5.
Needs a human look
None. One Pre-existing item below had a 4/5 vote and is marked.
Lower-priority observations
Nits, all in this PR's code
-
Org in the base URL doubles the path.
ADO_BASE_URL=https://dev.azure.com/myorgis classed Cloud by hostname, the error then tells the user to set the org, and the result is/myorg/myorg/{project}. Main's docblock documented the server/collection URL form, so this is a trap the PR's own message leads into. Reject a Cloud base URL with a path, or derive the org from it. [ado-client.ts#L44-L46]() Verified 5/5.raven/packages/ado-mcp/src/ado-client.ts
Lines 44 to 46 in a029d73
-
list_projectserror says "or supply collection" but the tool takes no arguments. The check at line 503 is also dead. README line 149 and the instructions string say any call can override the collection. [server.ts#L493-L509]() Verified 5/5.raven/packages/ado-mcp/src/server.ts
Lines 493 to 509 in a029d73
-
Cloud detection is exact hostname equality.
https://sonarqube.usis an official SonarQube Cloud region per SonarSource's scanner source, and it skips the organization guard and drops the parameter. Match the two known hosts. [sonar-client.ts#L69-L79]() Verified 5/5.raven/packages/sonar-mcp/src/sonar-client.ts
Lines 69 to 79 in a029d73
-
list_release_pipelineson Cloud returns a raw 404. The docs say Server-only; a two-lineisCloudguard would say so instead. [server.ts#L584-L600]() Verified 5/5.raven/packages/ado-mcp/src/server.ts
Lines 584 to 600 in a029d73
-
Stale on-prem wording left in edited files. README line 22, the collection JSDoc at
ado-client.tsline 128, SYSTEM_DESIGN line 306, and.env.examplelines 119-121 still say Server-only or omit the defaults. Verified 5/5.
Pre-existing, scanner execution surface (sonar-mcp)
-
sonar_run_scanruns npm test, dotnet build and dotnet test inside the AI-suppliedprojectDirwith the full process environment, which holds every loaded credential. Untrusted repo test code can read them. [sonar-scanner.ts#L304-L306]() Verified 5/5.raven/packages/sonar-mcp/src/sonar-scanner.ts
Lines 304 to 306 in a029d73
-
The forbidden list is a six-entry denylist. It omits proxy, truststore, keystore, httpExtraHeaders, javaExePath, engineJarPath and javaOpts, all real upstream properties reachable through
extraArgs. Two lenses read upstream as ignoringapiBaseUrlwhensonar.host.urlis set, so the host itself cannot be redirected. [server.ts#L197-L210]() Verified 5/5.raven/packages/sonar-mcp/src/server.ts
Lines 197 to 210 in a029d73
-
The Sonar token is on the scanner command line in all three places. The Java scanner reads
SONAR_TOKEN, but the child inheritsSONARQUBE_TOKEN, so a fix must setSONAR_TOKENin the spawn env explicitly. Not confirmed for the .NET scanner. [sonar-scanner.ts#L390-L393]() Verified 5/5.raven/packages/sonar-mcp/src/sonar-scanner.ts
Lines 390 to 393 in a029d73
-
-Dsonar.organization=becomes/d:sonar.organization=for .NET, which the scanner rejects. PR #67 translated name and version in code; organization is the same class, andextraArgshas no description. [sonar-scanner.ts#L408-L421]() Verified 5/5.raven/packages/sonar-mcp/src/sonar-scanner.ts
Lines 408 to 421 in a029d73
Pre-existing, ADO client hardening
-
No PI scrubbing anywhere in ado-mcp. Assignees, comment and PR authors, descriptions and error bodies reach the AI raw, while SYSTEM_DESIGN 7.4 says every server instantiates a PiScrubber. [server.ts#L90-L93](
) Verified 5/5.raven/packages/ado-mcp/src/server.ts
Lines 90 to 93 in a029d73
-
http://base URLs are accepted by both clients, so the Basic header goes in clear text. Jenkins, Artifactory and GitHub refuse non-HTTPS. The newnew URL()calls are where the check fits. Your own PR #90 review listed this as a follow-up. [ado-client.ts#L40-L50]() Verified 5/5.raven/packages/ado-mcp/src/ado-client.ts
Lines 40 to 50 in a029d73
-
Work-item comments appear to be a preview-only resource. Four verifiers found independent clients pinning
7.1-preview.xfor comments and reports ofVssInvalidPreviewVersionExceptionon plain versions. If so,add_work_item_commenterrors andget_work_itemsilently drops comments through its empty catch, on Server too. One live call settles it. [ado-client.ts#L205-L221]() Verified 4/5, one UNSURE.raven/packages/ado-mcp/src/ado-client.ts
Lines 205 to 221 in a029d73
-
..passes throughencodeURIComponent, so an AI-supplied collection or project can leave the configured virtual directory on the same host. Jenkins rejects dot segments. [ado-client.ts#L117-L123]() Verified 5/5.raven/packages/ado-mcp/src/ado-client.ts
Lines 117 to 123 in a029d73
-
The api-version comment "7.1 (supported by ADO Server 2020+)" is wrong; Microsoft's table caps Server 2020 at 6.0.
listAllPagesalso has no page cap, so a server returning ever-changing tokens loops forever. [ado-client.ts#L29-L32]() Both verified 5/5.raven/packages/ado-mcp/src/ado-client.ts
Lines 29 to 32 in a029d73
Pre-existing, destination choice
The per-call collection on the four ADO write tools lets injected text pick any organization the PAT reaches, with no allowlist like GITHUB_REPOSITORY_ALLOWLIST. Bounded by PAT scope and the host client's write confirmation. The argument predates the PR; the PR makes the default host multi-tenant. Verified 5/5.
Two small setup-script items: the verify output shows four characters of every secret, and the PowerShell script writes the DPAPI file before tightening its ACL.
Findings I considered and dropped
- Branch name
agents/...is not a listed type. Squash-merged; not actionable. new URL()in the constructor throws a bare "Invalid URL". Same failure fetch produced before; behaviour, not a defect.- The "(6 tools)" count removed from the SonarQube heading. The Jenkins heading has none either, and the inventory gate checks tool names only.
- Legacy
{org}.visualstudio.comhosts classed as Server. The Server path still works there. - Scanner validation skipped when
NODE_ENV=test. Needs control of the server's environment, which already holds the secrets. - No fetch timeout or body-size cap in either client. Robustness only.
sonar.configinprojectDircan set exclusions that hide findings. Scan integrity, outside this PR.- Host switching for
vsrm.dev.azure.com. Documented as out of scope; the guard is the nit above.
Net recommendation
Fix the three Required changes first, then ship. The defaults are the only thing standing between this PR and a clean merge.
|
Hi @jamesgagan , thanks for the thorough review. This should now all be addressed. |


This pull request updates the ADO and Sonar MCP servers to use their respective cloud URLs as the default instead of placeholders. Key changes include:
ADO Configuration:
https://dev.azure.com.ADO_DEFAULT_COLLECTIONfor both Cloud organizations and Server collections.list_projectsmethod to handle cases where a Cloud organization may have more than 200 projects.Sonar Configuration:
https://sonarcloud.io.Documentation Updates:
.github/copilot-instructions.md,README.md, and other documentation files to reflect the changes in configuration and usage.Testing: