feat: rework Checkmarx from CxSAST on-premise to Checkmarx One cloud

[kolatts]

Replaces the CxSAST on-premise integration with Checkmarx One cloud.

Config now uses clientId, clientSecret, and tenantName instead of username/password. Auth uses OAuth2 client_credentials flow via iam.checkmarx.net/{tenant}. API endpoints updated from /cxrestapi/ to /api/. Project and scan IDs are now UUID strings.

Closes #274

Generated with Claude Code

[claude]

Claude finished @kolatts's task in 3m 13s —— View job

---

PR Review: feat: rework Checkmarx from CxSAST on-premise to Checkmarx One cloud

• Clean up stale Claude comments (none found)
• Read changed files and review PR
• Post review feedback
• Submit formal GitHub review (requested changes)

---

Summary

The rework is solid. The OAuth2 client_credentials flow in checkmarxFetch.ts is clean, the token cache (with the 60-second early-refresh buffer) is correct, and the separation between the fetcher factory and the HTTP client layer is a good pattern. Config is wired correctly through types → env resolution → loadConfig → maskConfig → config init wizard. Tests cover the key token lifecycle paths well.

Two substantive issues to fix before merging, one concern worth confirming:

---

Blocking

config test and config check don't guard on tenantName

src/services/config/commands.ts:212 (config test):

if (cfg.checkmarx.baseUrl && cfg.checkmarx.clientId && cfg.checkmarx.clientSecret) {

src/services/config/commands.ts:447 (config check):

if (!cfg.checkmarx.clientId || !cfg.checkmarx.clientSecret) {

tenantName is absent from both guards. If a user configures baseUrl + clientId + clientSecret but forgets tenantName, both commands will attempt the connectivity call, which internally calls buildCheckmarxFetcher(), which throws:

PncliError: Checkmarx tenantName not configured. Run: pncli config init

That error propagates as an uncaught exception in the config test path (the outer catch will still emit JSON, but it's unexpected), and in config check it bypasses the clean blank/invalid/error categorisation. Every other service is guarded consistently — this one needs tenantName added to both conditions.

Fix this →

---

listProjects silently truncates (src/services/checkmarx/client.ts:14)

const res = await this.http.checkmarx<CxOneProjectsResponse>('/api/projects', { params: { limit: 100 } });
return res.projects;

The response type includes totalCount and filteredTotalCount, so the API paginates. With limit: 100 hardcoded and no pagination loop, organizations with more than 100 projects get a silent partial list. At minimum, the command should warn when res.filteredTotalCount > res.projects.length. The same applies to listScans.

Fix this →

---

Concern (worth verifying)

CxOneResultsSummary field names (src/types/checkmarx.ts:31-37)

export interface CxOneResultsSummary {
  highSeverity: number;
  mediumSeverity: number;
  lowSeverity: number;
  infoSeverity: number;
  severitiesTotalCount?: number;
}

The Checkmarx One GET /api/results/summary?scan-id=... endpoint uses a different shape depending on API version — some versions return HIGH/MEDIUM/LOW/INFO as keys, others return arrays. If the field names don't match the actual response, pncli checkmarx scan stats will return an object where all severity counts are undefined. Worth doing a quick sanity check against the real API (or the Checkmarx One API docs) before shipping. If the shape is correct, a one-line comment citing the API version would prevent future confusion.

---

Minor

• CX_IAM_BASE = 'https://iam.checkmarx.net' is hardcoded (checkmarxFetch.ts:5). The skills/pncli/checkmarx.md claims the IAM host is always iam.checkmarx.net regardless of region, which aligns with Checkmarx's own guidance for cloud tenants. Fine as-is, but if you ever need to support private/on-prem CxOne instances this will need to become configurable.

• The PncliError throws from buildCheckmarxFetcher use exit code 1, while most PncliErrors elsewhere use the named ExitCode constants. Not a functional issue, but slightly inconsistent.

[claude]

Two blocking issues before merge:

1. Missing tenantName in config test / config check guards — both commands check baseUrl + clientId + clientSecret but not tenantName. When tenantName is absent, buildCheckmarxFetcher() throws inside the connectivity branch instead of reporting 'not configured'. Add && cfg.checkmarx.tenantName to the config test condition (line 212) and || !cfg.checkmarx.tenantName to the config check blank guard (line 447).

2. Silent truncation in listProjects / listScans — hardcoded limit: 100 with no pagination loop means orgs with >100 projects/scans get partial results silently. The response already carries filteredTotalCount; at minimum emit a stderr warning when results are truncated.

Concern worth verifying: CxOneResultsSummary field names (highSeverity, mediumSeverity, etc.) may not match the actual Checkmarx One API response shape — if they're off, scan stats returns all-undefined severity counts.

The OAuth2 flow, token caching, config wiring, and test coverage are all well done.

[claude]

Claude Code is working…

Tasks

• Clean up stale Claude comments
• Read changed files and review PR
• Identify remaining blocking issues (2 still open from previous review)
• Fix: add tenantName to config test and config check guards
• Fix: add truncation warnings in listProjects / listScans
• Commit and push fixes
• Submit formal GitHub review

View job run

[claude]

The two blocking issues from the previous review are now resolved (fixed in the latest commit):

1. tenantName guard — added to both config test (line 212) and config check (line 447). A missing tenantName now correctly reports "not configured" instead of propagating a PncliError throw from buildCheckmarxFetcher.

2. Truncation warning — listProjects and listScans now emit a stderr warning when filteredTotalCount exceeds the returned count.

The e249adc commit (API key support) is well done: treating the API key as a JWT refresh token with grant_type: refresh_token, extracting the client ID from the azp claim, and deriving the IAM host from the API base URL (replacing the hardcoded constant) are all correct.

Remaining concern (non-blocking): CxOneResultsSummary field names (highSeverity, mediumSeverity, etc.) should be verified against the actual Checkmarx One API response before the next time this command is tested against a live tenant — if the field names don't match, scan stats returns all-undefined severity counts silently.

[claude]

Claude finished @claude[bot]'s task in 3m 35s —— View job

---

PR Review: feat: rework Checkmarx from CxSAST on-premise to Checkmarx One cloud

• Clean up stale Claude comments and reviews
• Read changed files and review PR
• Post review feedback
• Submit formal GitHub review (approved)

---

Summary

All blocking issues from the prior review cycle are resolved. The code is clean and ready to merge.

What was fixed since the CHANGES_REQUESTED review

tenantName guard (src/services/config/commands.ts:212, 447) — both config test and config check now include tenantName in their guards. A missing tenantName correctly produces "not configured" instead of propagating a raw PncliError from buildCheckmarxFetcher.

Truncation warnings (src/services/checkmarx/client.ts:15, 29) — listProjects and listScans now emit a stderr warning when filteredTotalCount exceeds the returned slice. Correct and minimal.

CI workflow change (e249adc / .github/workflows/claude-triage.yml)

The rework is sound. Writing PR title/body to /tmp/pr-title.txt and /tmp/pr-body.md and letting the post-step create the PR is cleaner than having the agent race the workflow. The --state all flag on the PR existence check correctly prevents duplicate PRs when a closed PR already exists for the branch. Removing gh pr create from allowedTools is the right call.

Remaining non-blocking concern

CxOneResultsSummary field names (highSeverity, mediumSeverity, lowSeverity, infoSeverity) should be verified against the live Checkmarx One /api/results/summary response shape before the first production use of pncli checkmarx scan stats. If the API returns HIGH/MEDIUM/LOW/INFO keys or a different structure, all counts will silently be undefined. Not a merge blocker — just verify on first live test.

Code quality

OAuth2 client_credentials flow and API key (refresh_token) flow are both correct. Token cache with 60-second early-refresh is correct. IAM host derivation via regex (/(^|\.)ast\./ → $1iam.) handles standard and regional URLs correctly. Config wiring (env vars → global config), maskConfig, and test coverage (token lifecycle, error paths, caching, expiry) are all solid.

[claude]

Re-review after latest synchronize. All blocking issues from the prior round are resolved; the code is clean and ready to merge.

Both fixes confirmed present:

1. tenantName guard in config test (commands.ts:212) and config check (commands.ts:447) — missing tenantName now correctly surfaces as 'not configured' rather than a raw PncliError throw.
2. Truncation warnings in listProjects and listScans (client.ts:15, 29) — stderr warning when filteredTotalCount exceeds returned slice.

CI workflow change (e249adc) is sound: writing PR title/body to /tmp files and letting the post-step create the PR is cleaner than having the agent race the workflow; --state all on the existence check prevents duplicates.

Remaining non-blocking concern: CxOneResultsSummary field names (highSeverity, mediumSeverity, lowSeverity, infoSeverity) should be verified against the live /api/results/summary response before first production use of pncli checkmarx scan stats.

OAuth2 flow, token caching, IAM host derivation, config wiring, masking, and test coverage are all correct. Approved.