DONT MERGE: feat: add kosli attest sarif command

[AlexKantor87]

⚠️ DO NOT MERGE — Depends on server #5538. CI will stay red until that PR merges and the new server image ships, because integration tests target a production server image that doesn't yet have the /sarif route.

Summary

Adds a first-class kosli attest sarif command, parallel to kosli attest snyk, that:

• Accepts SARIF v2.1.0 results from any compatible scanner (Checkov, Trivy, Semgrep, Snyk, CodeQL, etc.) — not just Snyk.
• Surfaces the tool name and version dynamically from the SARIF report's runs[0].tool.driver fields, so the Kosli UI tile reads "Checkov 3.2.1" / "Trivy 0.52" / etc. depending on what was run.
• Adds a -C, --compliant flag (default true, mirroring attest generic) so the caller controls whether the attestation is compliant. The CLI does not derive compliance from the SARIF findings — that decision is the caller's, expressed via the flag (typically driven by their own policy or rego rules).

Why

Customers currently map non-Snyk SARIF scans (notably Checkov) to the snyk attestation type because no native SARIF type exists. This works for parsing/display but bakes Snyk-specific compliance logic ("any high/medium/low → fail") into a generic SARIF flow, hardcodes "Snyk" as the tool label, and gives customers no way to express their own threshold. The new sarif type fixes all three.

Changes

Slice 1 — refactor: internal/snyk → internal/sarif

The existing snyk parser is genuinely a generic SARIF v2.1.0 parser — it just lives in a snyk-named package. Renamed to reflect actual scope; the attest snyk command keeps working unchanged via the renamed package. Wire format preserved (still posts snyk_results JSON key for backwards compatibility).

Slice 2 — new attest sarif command

• New cmd/kosli/attestSarif.go with --scan-results, --upload-results, and --compliant flags
• POSTs to new endpoint api/v2/attestations/{org}/{flow}/trail/{trail}/sarif (added in server#5538)
• Payload includes is_compliant: bool taken from the flag verbatim
• Full test suite parallel to snyk, plus --compliant=false and explicit --compliant=true cases

Local verification

End-to-end smoke-tested against a local demo running the server branch:

• Reported kosli attest sarif --scan-results sarif-iac.json → 201, attestation stored with dynamic tool name "Snyk IaC" 1.1177.0 and is_compliant: true
• Reported kosli attest sarif --scan-results sarif-helm.json --compliant=false → 201, is_compliant: false stored verbatim despite zero high-severity findings
• go test ./internal/sarif/... — passes
• Full suite (go test ./...) — would only pass against a server with the /sarif route, hence the merge block

Test plan

• Wait for server#5538 to merge and ship to the published server image
• CI re-runs green on this PR (no further code changes needed)
• Smoke test on a review env: report a SARIF attestation with --compliant=true and --compliant=false, confirm both tiles render correctly

🤖 Generated with Claude Code

[claude]

Claude finished @AlexKantor87's task in 3m 38s —— View job

---

PR Review: kosli attest sarif command

• Phase 1: Static analysis (go build, go vet, go mod tidy)
• Phase 2: Code review of all changed files
• Phase 3: Post inline comments and summary

---

Static Analysis

All three checks pass cleanly:

• go build ./... — compiles without errors
• go vet ./... — no issues
• go mod tidy — no drift in go.mod / go.sum

---

Summary

Well-structured PR that follows established codebase patterns closely. The refactor (slice 1) is clean — renaming internal/snyk → internal/sarif without behavioral changes, and the new command (slice 2) mirrors attestSnyk almost exactly, which makes it easy to review and maintain. The --compliant flag design (caller-controlled, not derived from findings) is a sound decision. Debug logging is a nice addition.

Issues Found

1. Unsafe type assertions in findLevel — potential panic (internal/sarif/sarif.go:150-155)

Pre-existing but higher-risk now. problem.(map[string]interface{}) and severity.(string) will panic if a non-Snyk SARIF producer uses unexpected types. With the broader scope of this command, safe comma-ok assertions are warranted:

problemMap, ok := problem.(map[string]interface{})
if !ok {
    return "", fmt.Errorf("could not find level for rule ID: %s", id)
}
severity, exists := problemMap["severity"]
if !exists {
    return "", fmt.Errorf("could not find level for rule ID: %s", id)
}
s, ok := severity.(string)
if !ok {
    return "", fmt.Errorf("could not find level for rule ID: %s", id)
}
return s, nil

Fix this →

2. Commented-out defaultConfiguration block may be needed for non-Snyk tools (internal/sarif/sarif.go:146-149)

The commented-out defaultConfig path reads reportingDescriptor.defaultConfiguration.level — the standard SARIF v2.1.0 way to express default severity. The current code only falls through to Snyk-specific Properties["problem"]["severity"]. Non-Snyk tools that omit level on individual results (relying instead on the rule's defaultConfiguration) will hit the error path "could not find level for rule ID". Consider uncommenting it as a primary fallback before the Snyk-specific property lookup.

3. Test coverage gaps (see inline comment)

• All command-level tests use only testdata/snyk_sarif.json. Adding a non-Snyk fixture (Checkov/Trivy) would prove the generic claim end-to-end.
• No test case for --upload-results=false.

4. Confirm sarif_results JSON key with server (see inline comment)

The payload uses json:"sarif_results" (vs. snyk_results in SnykAttestationPayload). Worth double-checking this matches what the server route in #5538 expects.

Minor / Pre-existing

• Schema validation (internal/sarif/sarif.go:53): The $schema URL allowlist may reject valid SARIF from tools that omit it or use a different URL. See inline comment. Not a blocker — consider a follow-up.
• The trailing whitespace issue from the prior review on attestSarifShortDesc has been fixed in the latest commit.
• The debug logging else branch suggested in the prior review has also been addressed.

What's Good

• Clean separation: the refactor slice and feature slice are independent, reviewable commits.
• The new command faithfully follows the existing attestSnyk pattern — minimal cognitive overhead for maintainers.
• --compliant flag design avoids hardcoding compliance logic in the CLI, giving customers full control.
• Debug logging with tool name and finding counts is genuinely useful for production troubleshooting.
• PR description is thorough with clear test plan and merge-block rationale.

---

[AlexKantor87]

Reviewer findings — actions taken (commit fbfc6e14)

Worked through all 5 inline review threads. Each thread has a per-finding reply explaining the action and is resolved.

Path	Finding	Action
internal/sarif/sarif.go:53	Long $schema validation line + non-Snyk SARIF may omit/vary $schema	Line length is style — left as-is for parity with the snyk-era code. The substantive concern (Checkov / Trivy may be rejected) is captured in server#5554 — that work adds real fixtures and will exercise this path
cmd/kosli/attestSarif.go:28	Trailing whitespace in attestSarifShortDesc	Fixed in fbfc6e14
cmd/kosli/attestSarif.go:28 (re-flagged)	Same as above	Same fix — fbfc6e14
cmd/kosli/attestSarif_test.go:37	Need command-level test with non-Snyk SARIF fixture	Captured in server#5554 — fixtures should come from real customer output (ADCB / Checkov), not synthesised
cmd/kosli/attestSarif.go:192	Else branch on zero-runs SARIF	Fixed in fbfc6e14 — else now logs tool name + compliant flag

Local verification on the new commit

• go build ./... — clean
• go test ./internal/sarif/... — pass

The full integration test suite still depends on the server PR (server#5538) merging before the published image picks up the /sarif route. This PR remains DONT MERGE until that lands.