feat(cmd): add SARIF output format

[Syt3s]

Description

本 PR 为 ocr review 和 ocr scan 新增 SARIF v2.1.0（OASIS 标准）作为第三种输出格式，使审查结果可通过 GitHub Code Scanning 进行持久化告警追踪。

背景

OpenCodeReview 当前支持 text 和 json 两种输出格式。在 CI/CD 流水线中，JSON 输出被 post-review-comments.js 消费，以 PR 行内评论的形式发布审查发现。然而 PR 评论是对话式的——push 后即过时，没有持久化告警生命周期（open → fixed → dismissed），没有 Security tab 仪表盘，也无法作为分支保护门槛。

OCR 内置了 SQL 注入、XSS、线程安全、NPE 检测等安全规则，但这些安全发现无法与 CodeQL、Semgrep、Bandit 等 SAST 工具一起呈现在 GitHub Security tab 中，因为这些工具输出 SARIF 而 OCR 不支持。

改动内容

新增 --format sarif 选项，将 LlmComment 字段映射为 SARIF v2.1.0 result 对象：

LlmComment 字段	SARIF result 字段
Path	locations[].physicalLocation.artifactLocation.uri
StartLine / EndLine	locations[].physicalLocation.region.startLine / endLine
Content	message.text
Category	ruleId + tool.driver.rules[].id
Severity	level（critical/high → error，medium → warning，low/空/未知 → note）
SuggestionCode	fixes[].artifactChanges[].replacements[].insertedContent.text
ExistingCode	fixes[].artifactChanges[].replacements[].deletedRegion

这是一个纯输出层变更——不涉及审查 Agent、LLM 循环、Diff 解析器或工具系统。实现完全遵循现有 outputJSON 和 outputText 函数的相同模式。

用法

# 以 SARIF 格式输出审查结果
ocr review --from main --to feature-branch --format sarif > results.sarif

# 上传到 GitHub Code Scanning（在 GitHub Actions 工作流中）
- uses: github/codeql-action/upload-sarif@v3
  with:
    sarif_file: results.sarif

改动文件

文件	改动
cmd/opencodereview/sarif.go	新增 — SARIF v2.1.0 结构体定义 + outputSARIF() 函数 + severity/category 映射
cmd/opencodereview/sarif_test.go	新增 — 23 个测试用例，覆盖 AC-1 ~ AC-15
cmd/opencodereview/shared.go	修改 — emitRunResult 新增 sarif 输出分支
cmd/opencodereview/shared_flags.go	修改 — --format flag 描述和补全列表新增 sarif

未修改任何 internal/ 包。未引入新的第三方 Go 依赖（go.mod 未变）。

Type of Change

• Bug fix (non-breaking change that fixes an issue)
• New feature (non-breaking change that adds functionality)
• Breaking change (fix or feature that would cause existing functionality to change)
• Refactoring (no functional changes)
• Documentation update
• CI / Build / Tooling

How Has This Been Tested?

单元测试

测试	验证内容
TestOutputSARIF_BasicStructure	顶层 SARIF 结构（$schema、version、runs、tool.driver、rules=8、results）
TestOutputSARIF_EmptyComments	空评论 → results: []（非 null），rules 仍存在
TestEmitRunResult_SarifNoFiles	无文件场景 → 合法 SARIF，空 results
TestOutputSARIF_FullFieldMapping	全字段映射：ruleId、level、message、locations[]、region、fixes、deletedRegion、insertedContent、partialFingerprints
TestSarifSeverityLevel	critical→error、high→error、medium→warning、low→note、空/未知→note（7 个子用例）
TestOutputSARIF_EmptyCategory	空 category → ruleId: "other"
TestOutputSARIF_ZeroLineNumbers	行号为 0 → region 省略、fixes 省略（deletedRegion 必需）
TestOutputSARIF_NoFixes	SuggestionCode 或 ExistingCode 为空 → fixes 省略（3 个子用例）
TestSarifRules	8 个 rule 的 id/name/shortDescription 正确
TestAddOutputFlags_IncludesSarif	--format flag 描述包含 sarif
TestEmitRunResult_Sarif	emitRunResult 以 sarif 格式输出合法 SARIF JSON 到 stdout
TestOutputSARIF_SchemaCompliance	字段类型验证 + 每个 replacement 必有 deletedRegion
TestOutputSARIF_JSONFormatting	2 空格缩进、换行结尾、合法 JSON
TestOutputSARIF_MultipleComments	3 条不同 path/category/severity 评论 → 正确独立映射
TestNewQuietHandle_Sarif	sarif 格式静默 stdout
TestSarifResultFromComment_EmptyPath	空 Path → Locations 和 Fixes 均为 nil
TestSarifResultFromComment_InvertedLineNumbers	反转行号 → region 和 fixes 省略
TestSarifResultFromComment_FixesWithEmptyPath	空 Path + 有 SuggestionCode → fixes 省略
TestSarifFingerprints_Stable	相同发现产生相同指纹，不依赖 message text
TestSarifFingerprints_EmptyExistingCodeFallback	ExistingCode 为空回退到 StartLine
TestSarifInvocation_ExecutionSuccessful	Complete/Skipped/Partial→true，Failed→false，nil→true
TestSarifInvocation_WarningsAsNotifications	manifest!=nil 过滤 subtask_error；manifest==nil 保留
TestSarifInvocation_NonCompleteAddsManifestNotification	非 Complete 状态添加 manifest message notification
TestSarifResults_DuplicateFingerprints	重复指纹追加 occurrence index
TestOutputPreview_SarifRejects	--preview --format sarif 返回错误

Checklist

• My code follows the project's coding style (go fmt, go vet)
• I have performed a self-review of my code
• I have added tests that prove my fix is effective or my feature works
• New and existing unit tests pass locally with my changes
• I have updated the documentation accordingly (if applicable)
• I have signed the CLA

CLI 参考文档和 GitHub Actions SARIF 上传示例计划作为后续 follow-up。

Related Issues

Fixes #819

[CLAassistant]

All committers have signed the CLA.

[github-actions]

🔍 OpenCodeReview found 3 issue(s) in this PR.

• ✅ Successfully posted inline: 2 comment(s)
• ❌ Failed to post inline: 1 comment(s)

---

[bug · high]

📄 cmd/opencodereview/shared.go (L272-L278)

⚠️ GitHub could not post this as an inline comment: Lines 272-278 could not be resolved (outside PR diff hunks)

Bug: newQuietHandle only silences stdout for JSON format, but SARIF is also a machine-readable format that writes to stdout. When using SARIF format with human audience, progress output will not be silenced and will corrupt the SARIF JSON output.

The condition should check for both "json" and "sarif" formats to match the machineReadable logic used elsewhere.

💡 Suggested Change

Before:

func newQuietHandle(outputFormat, audience string) *quietHandle {
	h := &quietHandle{}
	if outputFormat == "json" || audience == "agent" {
		h.fn = stdout.Quiet()
	}
	return h
}

After:

func newQuietHandle(outputFormat, audience string) *quietHandle {
	h := &quietHandle{}
	if outputFormat == "json" || outputFormat == "sarif" || audience == "agent" {
		h.fn = stdout.Quiet()
	}
	return h
}

[github-actions]

[maintainability · low]
Defensive programming: The region is only emitted when both c.StartLine > 0 && c.EndLine > 0, but there's no validation that StartLine <= EndLine. While the upstream resolver typically produces valid ranges from diff hunks, if a comment bypasses the resolver or the LLM produces an inverted range, the SARIF output would violate the spec requirement that endLine >= startLine, potentially causing SARIF consumers to reject or misinterpret the result.

Consider adding a check: if c.StartLine > 0 && c.EndLine >= c.StartLine

[github-actions]

[bug · medium]
Bug: The fixes block uses c.Path for the artifact location URI without checking whether it is empty. The location block above correctly guards on c.Path != "", but this fixes block does not. If a comment has non-empty SuggestionCode and ExistingCode but an empty Path, the emitted fix will contain "uri": "" in artifactLocation, which is structurally invalid for SARIF consumers and inconsistent with the location logic.

Consider guarding the fixes block with the same c.Path != "" check, or at minimum also requiring c.Path != "" in the condition.

Suggestion:

Suggested change

	if c.SuggestionCode != "" && c.ExistingCode != "" {
	if c.SuggestionCode != "" && c.ExistingCode != "" && c.Path != "" {

[lizhengfeng101]

Thanks for taking this on — SARIF support is a genuinely useful addition, and I like that you kept it strictly at the output layer and mirrored the existing outputJSON / outputText shape. The category set lines up exactly with the constants in internal/tool/code_comment.go, and since normalizeCodeCommentCategory already collapses anything unknown to other, every ruleId is guaranteed to resolve to a declared rule. Nice detail.

That said, I validated the output shape against the very schema the PR declares in $schema (json.schemastore.org/sarif-2.1.0.json), and I think the file we currently emit won't be accepted by GitHub. Three things I'd call blocking:

1. --format sarif doesn't silence progress output, so the document gets corrupted

newQuietHandle in shared.go:272 still only quiets stdout for json or audience=agent, and all the [ocr] … progress lines go through stdout.Writer() (see internal/agent/agent.go:272,277,313). So with the default --audience human:

$ ocr review --format sarif > results.sarif
$ head -3 results.sarif
[ocr] 12 file(s) changed, reviewing 12 in /repo
[ocr] estimated cost: ...
{

That isn't even parseable JSON. The machineReadable abstraction you added is doing the right thing inside emitRunResult, it just needs to reach the place that actually decides silencing. I'd pull it out so both sites share it:

func isMachineReadable(outputFormat string) bool {
	return outputFormat == "json" || outputFormat == "sarif"
}

func newQuietHandle(outputFormat, audience string) *quietHandle {
	h := &quietHandle{}
	if isMachineReadable(outputFormat) || audience == "agent" {
		h.fn = stdout.Quiet()
	}
	return h
}

The reason the suite stays green is that every test calls outputSARIF / emitRunResult directly and never goes through newQuietHandle (review_cmd.go:215). Worth a small TestNewQuietHandle_Sarif to lock it down.

2. result.location should be locations (an array)

In sarif.go:57 the field is declared as Location *sarifLocation with a json:"location,omitempty" tag. Checking the schema: definitions.result.properties has no location at all — only locations — and definitions.result sets additionalProperties: false. So this fails schema validation outright, and even where it's tolerated the alert ends up with no location and never lands on a line, which is the whole point of the feature.

type sarifResult struct {
	RuleID    string          `json:"ruleId"`
	Level     string          `json:"level"`
	Message   sarifMessage    `json:"message"`
	Locations []sarifLocation `json:"locations,omitempty"`
	Fixes     []sarifFix      `json:"fixes,omitempty"`
}

The result["location"] assertions in TestOutputSARIF_FullFieldMapping, _ZeroLineNumbers and _SchemaCompliance currently pin the wrong shape, so they'll need updating too.

3. replacement.deletedRegion is required, so the AC-8 "omit it" behaviour produces an invalid file

The schema has definitions.replacement.required == ["deletedRegion"]. In sarif.go:206-213, when line numbers are 0 or inverted we emit a replacement with only insertedContent. The important part: this doesn't degrade one finding — it invalidates the whole document, so every other alert in the run disappears with it.

Since there's no meaningful deletion range to construct when the lines are unknown anyway, I'd just not emit fixes in that case, which keeps the same rule you already apply to region:

hasRegion := c.StartLine > 0 && c.EndLine >= c.StartLine
if c.SuggestionCode != "" && c.ExistingCode != "" && c.Path != "" && hasRegion {
	// deletedRegion is always present here
}

TestOutputSARIF_ZeroLineNumbers and TestSarifResultFromComment_InvertedLineNumbers would flip to asserting fixes is absent.

---

Two things that speak to the stated goal rather than to validity:

partialFingerprints is what makes the alert lifecycle actually work. Without it GitHub fingerprints on ruleId + location + message, and our message.text is LLM prose that gets reworded on every run — so the same finding will keep getting closed and reopened, which undercuts the open → fixed → dismissed story the PR is built on. Something stable like sha256(Path + "|" + Category + "|" + normalize(ExistingCode)) under partialFingerprints: {"ocrFinding/v1": …} would fix it. I'd do this in this PR rather than as a follow-up.

Partial or failed runs will silently close real alerts. review_cmd.go:245 emits results whenever a manifest exists, including partial ones (budget exhausted, some files failed). A run that only covered 2 of 50 files produces a SARIF document indistinguishable from a clean full run, so GitHub marks everything it didn't see as fixed. Adding runs[].invocations[].executionSuccessful, derived from the manifest's terminal state, would prevent that. Same place you could surface ag.Warnings() as toolExecutionNotifications — right now warnings and ProjectSummary() are dropped entirely in the sarif path, and "file skipped, over token limit" is exactly the kind of thing a consumer needs to know.

---

Smaller stuff:

• The no-files branch in shared.go is redundant for sarif: because !machineReadable already skips the restore and the trace summary, falling through to the bottom outputSARIF call gives byte-identical output. You can drop the nested if and leave the early return as outputFormat == "json", then use !isMachineReadable(...) for the other three checks.
• sarifSeverityLevel falls back to warning, while normalizeCodeCommentSeverity in internal/tool/code_comment.go falls back to low (→ note here). Empty severity should only reach you via session resume, but I'd either match note or leave a comment explaining the deliberate bump.
• Docs go stale immediately in two spots even though they're listed as follow-up: pages/src/i18n/en.ts:168,224 (plus zh/ja) still say "Output format: text or json", and action.yml:279 hard-codes --format json — which means the headline use case (uploading from Actions) can't be done through the official action yet. Worth calling out explicitly in the description.
• outputPreview (output.go:483) only knows json, so --preview --format sarif silently prints the human view. Low priority, but mapping sarif→json there (or erroring) beats silence.

On the tests — the AC traceability is great, a few mechanical notes:

• json.Unmarshal results are discarded in _EmptyCategory, _ZeroLineNumbers, _NoFixes and _MultipleComments, so malformed output (issue 1) surfaces as a nil-map panic instead of a clear failure.
• TestEmitRunResult_Sarif* hand-roll os.Pipe instead of reusing captureStdout (output_helpers_test.go:383); the hand-rolled version deadlocks if output exceeds the ~64KB pipe buffer.
• t.Run(tc.severity, …) gives the empty-string case an unnamed subtest; a name field reads better.
• The schema URL and "OpenCodeReview" are re-typed as literals when sarifSchema / sarifToolName already exist.
• The gap worth closing: everything currently asserts "field exists and has the right type", which is precisely why issues 2 and 3 pass. A single assertion that locations exists and that each replacement carries a deletedRegion would have caught both — and validating the document against a vendored sarif-2.1.0.json (e.g. with santhosh-tekuri/jsonschema) would catch the whole class. Even simpler as a one-off sanity check: run github/codeql-action/upload-sarif against a real file once and paste the result — all three blockers would show up immediately.

Happy to re-review as soon as the schema issues are sorted. The structure is right; it's the wire format that needs a pass.

[Syt3s]

fixed

[lizhengfeng101]

Hey @Syt3s, nice work on this! The SARIF integration is clean and well-tested. I have a few things I'd like you to take a look at before we merge:

---

1. sarifNotification.Level is typed as sarifMessage — should be string

In SARIF v2.1.0, notification.level is a plain string enum ("none", "note", "warning", "error"), not a message object. Right now it serializes as:

{"level": {"text": "warning"}, "message": {"text": "slow"}}

but SARIF consumers (including GitHub Code Scanning) expect:

{"level": "warning", "message": {"text": "slow"}}

Should be a straightforward fix — change the struct field to string and update the assignment in sarifInvocationFromRun.

---

2. validateDelegateOptions still rejects "sarif"

shared_flags.go:161-162:

if opts.format != "text" && opts.format != "json" {
    return fmt.Errorf("invalid --format value %q: must be 'text' or 'json'", opts.format)
}

This means ocr delegate --format sarif will error out. Also registerDelegateFlags (lines 218-219) still shows "text or json" in the help text and doesn't include "sarif" in the completion list.

---

3. Fingerprint collision when ExistingCode is empty

The fingerprint hashes Path + "|" + category + "|" + TrimSpace(ExistingCode). When ExistingCode is empty (pretty common for findings without a fix suggestion), two different findings in the same file with the same category would produce identical fingerprints. GitHub Code Scanning uses these for dedup, so only one alert would be tracked.

Would it make sense to fall back to including StartLine (or even Content) in the hash when ExistingCode is empty? Or is this an intentional trade-off you've already considered?

---

4. (Minor) scan_cmd.go:218 — trace ID still prints for sarif

if opts.outputFormat != "json" {
    fmt.Fprintf(os.Stderr, "[ocr] TraceID: %s\n", traceID)
}

This goes to stderr so it won't corrupt the SARIF doc, but for consistency with the new isMachineReadable() helper you introduced, might be worth using it here too. Totally optional.

---

Let me know what you think — happy to discuss any of these. Items 1 and 2 should probably be fixed before merge; 3 and 4 are up to you whether to address now or follow up later.

[Syt3s]

Hi @lizhengfeng101, thanks for the thorough review!
I've pushed fixes for all four items — could you take another look when you have a moment? Let me know if anything else needs adjusting.

[lizhengfeng101]

Thanks for this — the SARIF modelling is clean and the comments explain the non-obvious constraints well (why deletedRegion can't be omitempty, why fixes need a valid region, why the fingerprint excludes message text). Verified locally on the PR head: go build ./..., go vet ./cmd/..., gofmt -l and go test ./cmd/... all pass.

Two things I'd like to see addressed before merge, both in sarifInvocationFromRun.

1. StateSkipped / StatePartial → executionSuccessful: false is the wrong mapping

cmd/opencodereview/sarif.go:327-331:

successful = manifest.TerminalState == session.StateComplete

Two of the four terminal states this rejects are not failures:

• StateSkipped — computeTerminal returns it when len(cov.Selected) == 0 (internal/session/manifest.go:947), and the human-readable rendering is "Review skipped: no items were selected." (cmd/opencodereview/output.go:398). That is a fully successful empty run — e.g. a PR that only touches lockfiles or paths excluded by the filter pipeline. Emitting executionSuccessful: false makes GitHub treat the analysis as not successful, surfacing a failure state on the Security tab and potentially skipping the alert-lifecycle update. This is one of the most common CI paths, so it will produce recurring false failures.
• StatePartial — the comment above the emit call in review_cmd.go states that "A successfully constructed manifest is publishable even when execution or session delivery failed." Partial is the expected, publishable outcome of budget truncation; declaring tool execution failed contradicts the pipeline's own contract.

Suggested fix — reserve false for StateFailed, and carry the state itself as a notification instead of discarding it:

successful := true
if manifest != nil {
    successful = manifest.TerminalState != session.StateFailed
    if manifest.TerminalState != session.StateComplete {
        inv.ToolExecutionNotifications = append(inv.ToolExecutionNotifications,
            sarifNotification{
                Level:   "warning",
                Message: sarifMessage{Text: manifestMessage(manifest, len(comments))},
            })
    }
}

manifestMessage already exists at output.go:388 and can be reused (it needs the comment count threaded in).

TestSarifInvocation_ExecutionSuccessful covers complete / partial / failed / nil — please add a StateSkipped case so the intended semantics are pinned down.

2. manifest == nil plus subtask errors makes SARIF report a clean, successful scan

sarifInvocationFromRun filters every isSubtaskErrorType warning unconditionally, on the grounds that those are "already expressed in the manifest's coverage.failed set". That premise only holds when a manifest exists:

• The JSON path uses warningsForOutput(warnings, manifest) (output.go:44), which does not filter when manifest == nil — the warnings reach the consumer intact.
• The text path explicitly handles manifest == nil && hasSubtaskErrors(warnings) by printing "Some files could not be reviewed due to errors" (output.go:71), which confirms the state is reachable.
• On the SARIF path in that same state, executionSuccessful defaults to true (no manifest) and every subtask error is dropped. The document becomes results: [] with executionSuccessful: true — a partially failed review renders on the Security tab as "scan passed, zero findings".

For a format meant to back a branch-protection gate, that is a silent false negative. Aligning with the JSON path's semantics should be enough:

// Subtask diagnostics are only redundant when a manifest froze them into
// coverage.failed; without one they are the only record of the failure.
if manifest != nil && isSubtaskErrorType(w.Type) {
    continue
}

Note that TestSarifInvocation_WarningsAsNotifications passes nil for the manifest and asserts the subtask error is filtered, so it currently locks in the behaviour above — worth updating alongside.

---

Smaller points, happy to take any of them as follow-ups:

• ocr delegate --format sarif now passes validateDelegateOptions (shared_flags.go:161) but delegate_cmd.go:179/:242 still branch on format == "json", and the delegate subcommands produce no findings at all — so it silently emits Markdown. I'd keep delegate's validation at text|json.
• --preview --format sarif writes a non-SARIF JSON document to stdout. The rationale in the comment makes sense, but ocr review -p --format sarif > r.sarif producing a differently-shaped document is harder to debug than an error. An empty-results SARIF (with preview info as notifications) or an explicit rejection would be friendlier.
• sarifFingerprints deliberately excludes the line number for cross-run stability, which is right — but two identical snippets in the same file under the same category (two identical string-concatenated SQL statements, two empty catch blocks) collide, and GitHub will fold them into one alert. Appending an occurrence index for duplicate fingerprints within a run would keep stability without losing findings.
• The stated motivation is appearing alongside CodeQL/Semgrep, but the 8 rules carry no properties — without tags: ["security"] and security-severity, security findings don't get a security-severity classification. Since severity is per-finding and security-severity is per-rule, that probably needs its own design (e.g. splitting rule ids by severity).
• Docs still say "text or json": pages/src/content/docs/{en,zh,ja,ru}/cli-reference.md:94 plus the --preview rows at :92/:319.
• The PR description is out of sync with the implementation in a few places: the severity table says empty/unknown → warning, while the code and tests use note (sarif.go:222 — and note is the right call, it matches normalizeCodeCommentSeverity defaulting to low); the field-mapping table says location.physicalLocation… where the implementation correctly uses the locations[] array; and the test count is ~20, not 16.

[Syt3s]

@lizhengfeng101Thanks for the careful review — really appreciate you catching these two issues in sarifInvocationFromRun. Both are now fixed:

executionSuccessful mapping — only StateFailed now maps to false. StateSkipped and StatePartial are true (publishable outcomes), and non-complete states carry the manifest message as a toolExecutionNotification so consumers can distinguish them from a clean run. Added a StateSkipped test case to pin this down.

manifest == nil + subtask errors — subtask diagnostics are now only filtered when a manifest exists to freeze them into coverage.failed. Without a manifest they're the sole record of the failure and are kept as notifications. Updated TestSarifInvocation_WarningsAsNotifications to cover both paths.

I also addressed the minor points: delegate validation stays at text|json, --preview --format sarif now errors explicitly, and duplicate fingerprints get an occurrence index appended.
Thanks again — happy to adjust if anything still looks off.

[lizhengfeng101]

LGTM