From a7a6b766c45685b845c92e1486c15f8a54359036 Mon Sep 17 00:00:00 2001 From: Claude Date: Fri, 24 Jul 2026 02:32:38 +0000 Subject: [PATCH 1/8] feat(cli): add sarif output format Add -format=sarif, writing a SARIF 2.1.0 log for upload to GitHub Code Scanning. The run's rule catalog lists the rules referenced by results with their descriptions and category tags, and issue positions map to %SRCROOT%-relative physical locations. Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_01EUTAwRxUmzDnAPFyR3ESby --- README.md | 13 +++ cmd/decolint/config.go | 2 +- cmd/decolint/format.go | 20 +++- cmd/decolint/format_test.go | 8 +- cmd/decolint/main_test.go | 74 +++++++++++++++ cmd/decolint/opts.go | 6 +- format/doc.go | 2 +- format/sarif.go | 181 ++++++++++++++++++++++++++++++++++++ format/sarif_test.go | 68 ++++++++++++++ 9 files changed, 366 insertions(+), 8 deletions(-) create mode 100644 format/sarif.go create mode 100644 format/sarif_test.go diff --git a/README.md b/README.md index 9fca875..6adc5f3 100644 --- a/README.md +++ b/README.md @@ -187,6 +187,19 @@ Select a different output format to change this: ``` ::warning file=.devcontainer/devcontainer.json,line=4,col=12,title=no-image-latest::image "ubuntu:latest" uses the "latest" tag; pin a specific version ``` +- `sarif` — a [SARIF 2.1.0](https://docs.oasis-open.org/sarif/sarif/v2.1.0/sarif-v2.1.0.html) + log, for upload to [GitHub Code + Scanning](https://docs.github.com/en/code-security/code-scanning/integrating-with-code-scanning/uploading-a-sarif-file-to-github) + so findings appear as alerts in the repository's Security tab: + ```yaml + - run: decolint -merge -format=sarif . > decolint.sarif + continue-on-error: true # decolint exits 1 on findings; the upload must still run + - uses: github/codeql-action/upload-sarif@v3 + with: + sarif_file: decolint.sarif + ``` + Run `decolint` from the repository root with relative directory + arguments, so the reported paths resolve to repository files. ### Exit codes diff --git a/cmd/decolint/config.go b/cmd/decolint/config.go index e332a85..85fc947 100644 --- a/cmd/decolint/config.go +++ b/cmd/decolint/config.go @@ -24,7 +24,7 @@ type Config struct { // also cause exit code 1. The -deny-warnings flag can enable it as well. DenyWarnings bool `json:"denyWarnings"` // Format selects how lint issues are written to stdout: "text" (the default when empty), "json", - // or "github". The -format flag takes precedence. + // "github", or "sarif". The -format flag takes precedence. Format string `json:"format"` // LocalEnv maps names to the values "${localEnv:NAME}" (and "${env:NAME}") resolve to during // variable substitution, which runs with Merge; see [substitute.Context.LocalEnv]. It is also diff --git a/cmd/decolint/format.go b/cmd/decolint/format.go index 00b3810..36a9e68 100644 --- a/cmd/decolint/format.go +++ b/cmd/decolint/format.go @@ -7,6 +7,7 @@ import ( "github.com/bare-devcontainer/decolint/format" "github.com/bare-devcontainer/decolint/linter" + "github.com/bare-devcontainer/decolint/rules" ) // Format identifies how lint issues are written to stdout. @@ -25,7 +26,24 @@ func parseFormat(name string) (Format, error) { return format.JSONFormat{}, nil case "github": return format.GitHubFormat{}, nil + case "sarif": + return format.SARIFFormat{Version: version, Rules: sarifRules()}, nil default: - return nil, fmt.Errorf("unknown format %q (want one of: text, json, github)", name) + return nil, fmt.Errorf("unknown format %q (want one of: text, json, github, sarif)", name) } } + +// sarifRules adapts the built-in rule catalog into the shape [format.SARIFFormat] consumes, so the +// format package does not depend on the rules package. +func sarifRules() []format.SARIFRule { + regs := rules.Builtin() + out := make([]format.SARIFRule, len(regs)) + for i, reg := range regs { + out[i] = format.SARIFRule{ + ID: reg.Rule.ID, + Description: reg.Rule.Description, + Category: reg.Rule.Category.String(), + } + } + return out +} diff --git a/cmd/decolint/format_test.go b/cmd/decolint/format_test.go index df7ad9c..05e6789 100644 --- a/cmd/decolint/format_test.go +++ b/cmd/decolint/format_test.go @@ -4,6 +4,7 @@ import ( "testing" "github.com/bare-devcontainer/decolint/format" + "github.com/google/go-cmp/cmp" ) func TestParseFormat(t *testing.T) { @@ -19,6 +20,7 @@ func TestParseFormat(t *testing.T) { {"text", "text", format.TextFormat{}, false}, {"json", "json", format.JSONFormat{}, false}, {"github", "github", format.GitHubFormat{}, false}, + {"sarif", "sarif", format.SARIFFormat{Version: version, Rules: sarifRules()}, false}, {"unknown", "bogus", nil, true}, } for _, tt := range tests { @@ -31,8 +33,10 @@ func TestParseFormat(t *testing.T) { if err != nil { return } - if got != tt.want { - t.Errorf("parseFormat(%q) = %v, want %v", tt.in, got, tt.want) + // cmp.Diff rather than ==: SARIFFormat is not comparable, so an interface comparison + // would panic. + if diff := cmp.Diff(tt.want, got); diff != "" { + t.Errorf("parseFormat(%q) mismatch (-want +got):\n%s", tt.in, diff) } }) } diff --git a/cmd/decolint/main_test.go b/cmd/decolint/main_test.go index e0611ae..50c3c80 100644 --- a/cmd/decolint/main_test.go +++ b/cmd/decolint/main_test.go @@ -509,6 +509,80 @@ func TestRun_OutputFormat(t *testing.T) { } }) + t.Run("sarif log", func(t *testing.T) { + t.Parallel() + + var stdout, stderr bytes.Buffer + exitCode := run(t.Context(), []string{"-format=sarif", "-platform=vscode,codespaces", violationsDir}, &stdout, &stderr) + if exitCode != 1 { + t.Fatalf("exit code = %d, want 1; stderr: %s", exitCode, stderr.String()) + } + + // Decoded structurally rather than compared as a golden string: message texts and rule + // descriptions are owned by the rule tests and the format package's own tests. + var log struct { + Version string `json:"version"` + Runs []struct { + Tool struct { + Driver struct { + Name string `json:"name"` + Rules []struct { + ID string `json:"id"` + } `json:"rules"` + } `json:"driver"` + } `json:"tool"` + Results []struct { + RuleID string `json:"ruleId"` + RuleIndex int `json:"ruleIndex"` + Level string `json:"level"` + Locations []struct { + PhysicalLocation struct { + ArtifactLocation struct { + URI string `json:"uri"` + } `json:"artifactLocation"` + } `json:"physicalLocation"` + } `json:"locations"` + } `json:"results"` + } `json:"runs"` + } + if err := json.Unmarshal(stdout.Bytes(), &log); err != nil { + t.Fatalf("output is not a SARIF log: %v\noutput: %s", err, stdout.String()) + } + if log.Version != "2.1.0" { + t.Errorf("version = %q, want %q", log.Version, "2.1.0") + } + if len(log.Runs) != 1 { + t.Fatalf("runs = %d, want 1", len(log.Runs)) + } + run := log.Runs[0] + if run.Tool.Driver.Name != "decolint" { + t.Errorf("driver name = %q, want %q", run.Tool.Driver.Name, "decolint") + } + + var got []string + for _, result := range run.Results { + got = append(got, result.RuleID) + if result.Level != "error" { + t.Errorf("result %s level = %q, want %q", result.RuleID, result.Level, "error") + } + if result.RuleIndex < 0 || result.RuleIndex >= len(run.Tool.Driver.Rules) { + t.Errorf("result %s ruleIndex = %d, out of range", result.RuleID, result.RuleIndex) + } else if id := run.Tool.Driver.Rules[result.RuleIndex].ID; id != result.RuleID { + t.Errorf("result %s ruleIndex points at rule %q", result.RuleID, id) + } + if len(result.Locations) != 1 { + t.Fatalf("result %s locations = %d, want 1", result.RuleID, len(result.Locations)) + } + if uri := result.Locations[0].PhysicalLocation.ArtifactLocation.URI; uri != violationsFile { + t.Errorf("result %s uri = %q, want %q", result.RuleID, uri, violationsFile) + } + } + want := []string{"no-bind-mount", "no-host-port-format"} + if diff := cmp.Diff(want, got, cmpopts.SortSlices(func(a, b string) bool { return a < b })); diff != "" { + t.Errorf("fired rules mismatch (-want +got):\n%s", diff) + } + }) + t.Run("format selected by config file", func(t *testing.T) { t.Parallel() diff --git a/cmd/decolint/opts.go b/cmd/decolint/opts.go index 2486333..ff2a19e 100644 --- a/cmd/decolint/opts.go +++ b/cmd/decolint/opts.go @@ -37,8 +37,8 @@ type Options struct { // "merge": true). mergeSet bool // Format is the raw -format flag value ("" if not given), naming how lint issues are written to - // stdout: "text", "json", or "github". A non-empty value replaces the config file's "format" - // member; it is resolved into a Format by parseFormat in runLint. + // stdout: "text", "json", "github", or "sarif". A non-empty value replaces the config file's + // "format" member; it is resolved into a Format by parseFormat in runLint. Format string // Version, when set, causes the program to print its version and exit. Version bool @@ -60,7 +60,7 @@ func parseOptions(args []string, output io.Writer) (Options, error) { fs.BoolVar(&opts.DenyWarnings, "deny-warnings", false, "treat warnings as failures (exit code 1); overrides the config file's \"denyWarnings\" member") fs.StringVar(&opts.ConfigPath, "config", "", "path to a config file (default: auto-discover .decolint.jsonc or .decolint.json in the current directory)") fs.StringVar(&platformFlag, "platform", "", "comma-separated target platforms to include in addition to \"all\" (vscode, codespaces); overrides the config file's \"platforms\" member") - fs.StringVar(&formatFlag, "format", "", "output format: text (default), json, or github; overrides the config file's \"format\" member") + fs.StringVar(&formatFlag, "format", "", "output format: text (default), json, github, or sarif; overrides the config file's \"format\" member") fs.BoolVar(&opts.Merge, "merge", false, "lint the merged (effective) configuration, including referenced Features and base image metadata; overrides the config file's \"merge\" member") fs.BoolVar(&opts.Version, "version", false, "print version information and exit") fs.BoolVar(&opts.ListRules, "rules", false, "print the built-in rules as a Markdown table (category, target platforms, current severity), then exit") diff --git a/format/doc.go b/format/doc.go index 6fb2c6b..b858a1e 100644 --- a/format/doc.go +++ b/format/doc.go @@ -1,3 +1,3 @@ // Package format implements the output formats decolint can write lint issues in: one-line-per-issue -// text, a JSON array, and GitHub Actions workflow command annotations. +// text, a JSON array, GitHub Actions workflow command annotations, and a SARIF 2.1.0 log. package format diff --git a/format/sarif.go b/format/sarif.go new file mode 100644 index 0000000..1deecda --- /dev/null +++ b/format/sarif.go @@ -0,0 +1,181 @@ +package format + +import ( + "bytes" + "encoding/json/v2" + "fmt" + "io" + "maps" + "path/filepath" + "slices" + + "github.com/bare-devcontainer/decolint/linter" +) + +// SARIFRule describes one rule for the SARIF rule catalog. It mirrors the [linter.Rule] fields the +// SARIF output needs, so this package does not depend on the rules package. +type SARIFRule struct { + ID string + Description string + Category string +} + +// SARIFFormat prints a SARIF 2.1.0 log, suitable for upload to GitHub Code Scanning. +type SARIFFormat struct { + // Version is the tool version recorded in the run. + Version string + // Rules is the rule catalog used to describe the rules referenced by issues. + Rules []SARIFRule +} + +const ( + sarifSchemaURI = "https://json.schemastore.org/sarif-2.1.0.json" + sarifVersion = "2.1.0" + informationURI = "https://github.com/bare-devcontainer/decolint" +) + +// The wire structs cover the subset of SARIF 2.1.0 decolint emits. Fields marshal in declaration +// order, so the output is deterministic. + +type sarifLog struct { + Schema string `json:"$schema"` + Version string `json:"version"` + Runs []sarifRun `json:"runs"` +} + +type sarifRun struct { + Tool sarifTool `json:"tool"` + Results []sarifResult `json:"results"` +} + +type sarifTool struct { + Driver sarifDriver `json:"driver"` +} + +type sarifDriver struct { + Name string `json:"name"` + Version string `json:"version"` + InformationURI string `json:"informationUri"` + Rules []sarifRuleDescriptor `json:"rules"` +} + +type sarifRuleDescriptor struct { + ID string `json:"id"` + ShortDescription sarifMessage `json:"shortDescription,omitzero"` + Properties sarifProperties `json:"properties,omitzero"` +} + +type sarifProperties struct { + Tags []string `json:"tags"` +} + +type sarifMessage struct { + Text string `json:"text"` +} + +type sarifResult struct { + RuleID string `json:"ruleId"` + RuleIndex int `json:"ruleIndex"` + Level string `json:"level"` + Message sarifMessage `json:"message"` + Locations []sarifLocation `json:"locations"` +} + +type sarifLocation struct { + PhysicalLocation sarifPhysicalLocation `json:"physicalLocation"` +} + +type sarifPhysicalLocation struct { + ArtifactLocation sarifArtifactLocation `json:"artifactLocation"` + Region sarifRegion `json:"region"` +} + +type sarifArtifactLocation struct { + URI string `json:"uri"` + URIBaseID string `json:"uriBaseId"` +} + +type sarifRegion struct { + StartLine int `json:"startLine"` + StartColumn int `json:"startColumn"` +} + +// WriteIssues writes issues to w as a SARIF 2.1.0 log with a single run. It marshals into an +// in-memory buffer first so that a failure never leaves partial output on w. +// +// The run's rule catalog lists only the rules referenced by issues, sorted by rule ID; a rule +// missing from f.Rules is listed with its ID alone. Issue paths become forward-slash URIs relative +// to %SRCROOT%, so uploads map to repository files when decolint runs at the repository root. +func (f SARIFFormat) WriteIssues(w io.Writer, issues []linter.Issue) error { + catalog := make(map[string]SARIFRule, len(f.Rules)) + for _, r := range f.Rules { + catalog[r.ID] = r + } + + referenced := make(map[string]bool) + for _, issue := range issues { + referenced[issue.RuleID] = true + } + ids := slices.Sorted(maps.Keys(referenced)) + + descriptors := make([]sarifRuleDescriptor, 0, len(ids)) + index := make(map[string]int, len(ids)) + for i, id := range ids { + index[id] = i + desc := sarifRuleDescriptor{ID: id} + if r, ok := catalog[id]; ok { + desc.ShortDescription = sarifMessage{Text: r.Description} + desc.Properties = sarifProperties{Tags: []string{r.Category}} + } + descriptors = append(descriptors, desc) + } + + results := make([]sarifResult, 0, len(issues)) + for _, issue := range issues { + level := "error" + if issue.Severity == linter.SeverityWarn { + level = "warning" + } + // startColumn is emitted as the issue's byte column even though SARIF defaults to UTF-16 + // code units; the two agree on ASCII lines, and alerts anchor by line regardless. + results = append(results, sarifResult{ + RuleID: issue.RuleID, + RuleIndex: index[issue.RuleID], + Level: level, + Message: sarifMessage{Text: issue.Message}, + Locations: []sarifLocation{{ + PhysicalLocation: sarifPhysicalLocation{ + ArtifactLocation: sarifArtifactLocation{ + URI: filepath.ToSlash(issue.Path), + URIBaseID: "%SRCROOT%", + }, + Region: sarifRegion{StartLine: issue.Line, StartColumn: issue.Col}, + }, + }}, + }) + } + + log := sarifLog{ + Schema: sarifSchemaURI, + Version: sarifVersion, + Runs: []sarifRun{{ + Tool: sarifTool{Driver: sarifDriver{ + Name: "decolint", + Version: f.Version, + InformationURI: informationURI, + Rules: descriptors, + }}, + Results: results, + }}, + } + + var buf bytes.Buffer + if err := json.MarshalWrite(&buf, log); err != nil { + return fmt.Errorf("marshal sarif: %w", err) + } + buf.WriteByte('\n') + if _, err := w.Write(buf.Bytes()); err != nil { + return fmt.Errorf("write issues: %w", err) + } + return nil +} diff --git a/format/sarif_test.go b/format/sarif_test.go new file mode 100644 index 0000000..d431427 --- /dev/null +++ b/format/sarif_test.go @@ -0,0 +1,68 @@ +package format + +import ( + "errors" + "strings" + "testing" +) + +// testSARIFFormat returns a SARIFFormat whose catalog knows only one of the two rules in +// testIssues, so both the known- and unknown-rule descriptor paths are exercised. +func testSARIFFormat() SARIFFormat { + return SARIFFormat{ + Version: "1.2.3", + Rules: []SARIFRule{ + { + ID: "no-image-latest", + Description: "images should be pinned to a specific version", + Category: "reproducibility", + }, + }, + } +} + +func TestSARIFWriteIssues(t *testing.T) { + t.Parallel() + + var sb strings.Builder + if err := testSARIFFormat().WriteIssues(&sb, testIssues()); err != nil { + t.Fatalf("WriteIssues: %v", err) + } + + want := `{"$schema":"https://json.schemastore.org/sarif-2.1.0.json","version":"2.1.0","runs":[` + + `{"tool":{"driver":{"name":"decolint","version":"1.2.3","informationUri":"https://github.com/bare-devcontainer/decolint","rules":[` + + `{"id":"no-image-latest","shortDescription":{"text":"images should be pinned to a specific version"},"properties":{"tags":["reproducibility"]}},` + + `{"id":"some-error-rule"}]}},"results":[` + + `{"ruleId":"no-image-latest","ruleIndex":0,"level":"warning","message":{"text":"image \"ubuntu:latest\" uses the \"latest\" tag; pin a specific version"},` + + `"locations":[{"physicalLocation":{"artifactLocation":{"uri":".devcontainer/devcontainer.json","uriBaseId":"%SRCROOT%"},"region":{"startLine":4,"startColumn":12}}}]},` + + `{"ruleId":"some-error-rule","ruleIndex":1,"level":"error","message":{"text":"something is broken"},` + + `"locations":[{"physicalLocation":{"artifactLocation":{"uri":".devcontainer/devcontainer.json","uriBaseId":"%SRCROOT%"},"region":{"startLine":8,"startColumn":3}}}]}]}]}` + + "\n" + if sb.String() != want { + t.Errorf("WriteIssues sarif = %q, want %q", sb.String(), want) + } +} + +func TestSARIFWriteIssues_Empty(t *testing.T) { + t.Parallel() + + var sb strings.Builder + if err := testSARIFFormat().WriteIssues(&sb, nil); err != nil { + t.Fatalf("WriteIssues: %v", err) + } + + want := `{"$schema":"https://json.schemastore.org/sarif-2.1.0.json","version":"2.1.0","runs":[` + + `{"tool":{"driver":{"name":"decolint","version":"1.2.3","informationUri":"https://github.com/bare-devcontainer/decolint","rules":[]}},"results":[]}]}` + + "\n" + if sb.String() != want { + t.Errorf("WriteIssues sarif (empty) = %q, want %q", sb.String(), want) + } +} + +func TestSARIFWriteIssues_WriteError(t *testing.T) { + t.Parallel() + + if err := testSARIFFormat().WriteIssues(errWriter{}, testIssues()); !errors.Is(err, errWrite) { + t.Errorf("WriteIssues error = %v, want %v", err, errWrite) + } +} From 8e77ca0f404c13ffeab76042ff58d8b1f977bba1 Mon Sep 17 00:00:00 2001 From: nozaq Date: Fri, 24 Jul 2026 16:59:22 +0000 Subject: [PATCH 2/8] test(cli): compare SARIF output with cmp.Diff Build an expected SARIF log and compare it in one cmp.Diff instead of asserting each field individually. Named struct types decode only the structural subset so message texts and rule descriptions stay owned by the rule and format package tests. Co-Authored-By: Claude Opus 4.8 (1M context) --- cmd/decolint/main_test.go | 121 ++++++++++++++++++++------------------ 1 file changed, 63 insertions(+), 58 deletions(-) diff --git a/cmd/decolint/main_test.go b/cmd/decolint/main_test.go index 50c3c80..c9c01e8 100644 --- a/cmd/decolint/main_test.go +++ b/cmd/decolint/main_test.go @@ -460,6 +460,45 @@ func TestRun_Flags(t *testing.T) { }) } +// The sarif* types decode the structural subset of a SARIF log the "sarif log" test asserts. The +// message texts and rule descriptions the format carries are left out on purpose: they are owned by +// the rule tests and the format package's own tests. +type ( + sarifLog struct { + Version string `json:"version"` + Runs []sarifRun `json:"runs"` + } + sarifRun struct { + Tool sarifTool `json:"tool"` + Results []sarifResult `json:"results"` + } + sarifTool struct { + Driver sarifDriver `json:"driver"` + } + sarifDriver struct { + Name string `json:"name"` + Rules []sarifRule `json:"rules"` + } + sarifRule struct { + ID string `json:"id"` + } + sarifResult struct { + RuleID string `json:"ruleId"` + RuleIndex int `json:"ruleIndex"` + Level string `json:"level"` + Locations []sarifLocation `json:"locations"` + } + sarifLocation struct { + PhysicalLocation sarifPhysicalLocation `json:"physicalLocation"` + } + sarifPhysicalLocation struct { + ArtifactLocation sarifArtifactLocation `json:"artifactLocation"` + } + sarifArtifactLocation struct { + URI string `json:"uri"` + } +) + func TestRun_OutputFormat(t *testing.T) { t.Parallel() @@ -518,68 +557,34 @@ func TestRun_OutputFormat(t *testing.T) { t.Fatalf("exit code = %d, want 1; stderr: %s", exitCode, stderr.String()) } - // Decoded structurally rather than compared as a golden string: message texts and rule - // descriptions are owned by the rule tests and the format package's own tests. - var log struct { - Version string `json:"version"` - Runs []struct { - Tool struct { - Driver struct { - Name string `json:"name"` - Rules []struct { - ID string `json:"id"` - } `json:"rules"` - } `json:"driver"` - } `json:"tool"` - Results []struct { - RuleID string `json:"ruleId"` - RuleIndex int `json:"ruleIndex"` - Level string `json:"level"` - Locations []struct { - PhysicalLocation struct { - ArtifactLocation struct { - URI string `json:"uri"` - } `json:"artifactLocation"` - } `json:"physicalLocation"` - } `json:"locations"` - } `json:"results"` - } `json:"runs"` - } + var log sarifLog if err := json.Unmarshal(stdout.Bytes(), &log); err != nil { t.Fatalf("output is not a SARIF log: %v\noutput: %s", err, stdout.String()) } - if log.Version != "2.1.0" { - t.Errorf("version = %q, want %q", log.Version, "2.1.0") - } - if len(log.Runs) != 1 { - t.Fatalf("runs = %d, want 1", len(log.Runs)) - } - run := log.Runs[0] - if run.Tool.Driver.Name != "decolint" { - t.Errorf("driver name = %q, want %q", run.Tool.Driver.Name, "decolint") - } - var got []string - for _, result := range run.Results { - got = append(got, result.RuleID) - if result.Level != "error" { - t.Errorf("result %s level = %q, want %q", result.RuleID, result.Level, "error") - } - if result.RuleIndex < 0 || result.RuleIndex >= len(run.Tool.Driver.Rules) { - t.Errorf("result %s ruleIndex = %d, out of range", result.RuleID, result.RuleIndex) - } else if id := run.Tool.Driver.Rules[result.RuleIndex].ID; id != result.RuleID { - t.Errorf("result %s ruleIndex points at rule %q", result.RuleID, id) - } - if len(result.Locations) != 1 { - t.Fatalf("result %s locations = %d, want 1", result.RuleID, len(result.Locations)) - } - if uri := result.Locations[0].PhysicalLocation.ArtifactLocation.URI; uri != violationsFile { - t.Errorf("result %s uri = %q, want %q", result.RuleID, uri, violationsFile) - } - } - want := []string{"no-bind-mount", "no-host-port-format"} - if diff := cmp.Diff(want, got, cmpopts.SortSlices(func(a, b string) bool { return a < b })); diff != "" { - t.Errorf("fired rules mismatch (-want +got):\n%s", diff) + locAt := func(uri string) []sarifLocation { + return []sarifLocation{{PhysicalLocation: sarifPhysicalLocation{ + ArtifactLocation: sarifArtifactLocation{URI: uri}, + }}} + } + // The catalog lists only referenced rules, sorted by ID, so each result's ruleIndex points at + // its like-named catalog entry. + want := sarifLog{ + Version: "2.1.0", + Runs: []sarifRun{{ + Tool: sarifTool{Driver: sarifDriver{ + Name: "decolint", + Rules: []sarifRule{{ID: "no-bind-mount"}, {ID: "no-host-port-format"}}, + }}, + Results: []sarifResult{ + {RuleID: "no-bind-mount", RuleIndex: 0, Level: "error", Locations: locAt(violationsFile)}, + {RuleID: "no-host-port-format", RuleIndex: 1, Level: "error", Locations: locAt(violationsFile)}, + }, + }}, + } + sortResults := cmpopts.SortSlices(func(a, b sarifResult) bool { return a.RuleID < b.RuleID }) + if diff := cmp.Diff(want, log, sortResults); diff != "" { + t.Errorf("sarif log mismatch (-want +got):\n%s", diff) } }) From e59115e5133ce1a17f509bf626f9979baa98ebc5 Mon Sep 17 00:00:00 2001 From: Claude Date: Sat, 25 Jul 2026 01:43:57 +0000 Subject: [PATCH 3/8] fix(sarif): report an absolute path as a file URI Lint results are now named relative to the working directory, or absolutely for a file outside it. A SARIF artifact location may only carry a uriBaseId when its uri is a relative reference, so an absolute path is written as a file URI with no base id; resolving it against the source root would name a different file. Paths are percent-encoded, since a SARIF artifact location is a URI rather than a filesystem path. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01EUTAwRxUmzDnAPFyR3ESby --- README.md | 4 ++-- cmd/decolint/main_test.go | 11 ++++++--- format/sarif.go | 31 ++++++++++++++++++------- format/sarif_test.go | 48 +++++++++++++++++++++++++++++++++++++++ 4 files changed, 81 insertions(+), 13 deletions(-) diff --git a/README.md b/README.md index 401b72c..195016e 100644 --- a/README.md +++ b/README.md @@ -198,8 +198,8 @@ Select a different output format to change this: with: sarif_file: decolint.sarif ``` - Run `decolint` from the repository root with relative directory - arguments, so the reported paths resolve to repository files. + Run `decolint` from the repository root, so that the paths in the log + resolve to files in the repository. Every format reports paths relative to the directory `decolint` runs in, whichever way the linted directory was named on the command line. A file diff --git a/cmd/decolint/main_test.go b/cmd/decolint/main_test.go index 1a3f7bc..58f8b65 100644 --- a/cmd/decolint/main_test.go +++ b/cmd/decolint/main_test.go @@ -594,7 +594,8 @@ func TestRun_OutputFormat(t *testing.T) { Locations []struct { PhysicalLocation struct { ArtifactLocation struct { - URI string `json:"uri"` + URI string `json:"uri"` + URIBaseID string `json:"uriBaseId"` } `json:"artifactLocation"` } `json:"physicalLocation"` } `json:"locations"` @@ -629,8 +630,12 @@ func TestRun_OutputFormat(t *testing.T) { if len(result.Locations) != 1 { t.Fatalf("result %s locations = %d, want 1", result.RuleID, len(result.Locations)) } - if uri := result.Locations[0].PhysicalLocation.ArtifactLocation.URI; uri != violationsFile { - t.Errorf("result %s uri = %q, want %q", result.RuleID, uri, violationsFile) + location := result.Locations[0].PhysicalLocation.ArtifactLocation + if location.URI != violationsFile { + t.Errorf("result %s uri = %q, want %q", result.RuleID, location.URI, violationsFile) + } + if location.URIBaseID != "%SRCROOT%" { + t.Errorf("result %s uriBaseId = %q, want %q", result.RuleID, location.URIBaseID, "%SRCROOT%") } } want := []string{"no-bind-mount", "no-host-port-format"} diff --git a/format/sarif.go b/format/sarif.go index 1deecda..505efcc 100644 --- a/format/sarif.go +++ b/format/sarif.go @@ -6,8 +6,10 @@ import ( "fmt" "io" "maps" + "net/url" "path/filepath" "slices" + "strings" "github.com/bare-devcontainer/decolint/linter" ) @@ -32,6 +34,9 @@ const ( sarifSchemaURI = "https://json.schemastore.org/sarif-2.1.0.json" sarifVersion = "2.1.0" informationURI = "https://github.com/bare-devcontainer/decolint" + // srcRootBaseID is the SARIF base id a relative path is reported against. An upload resolves it + // to the root of the analyzed checkout. + srcRootBaseID = "%SRCROOT%" ) // The wire structs cover the subset of SARIF 2.1.0 decolint emits. Fields marshal in declaration @@ -92,7 +97,7 @@ type sarifPhysicalLocation struct { type sarifArtifactLocation struct { URI string `json:"uri"` - URIBaseID string `json:"uriBaseId"` + URIBaseID string `json:"uriBaseId,omitzero"` } type sarifRegion struct { @@ -104,8 +109,8 @@ type sarifRegion struct { // in-memory buffer first so that a failure never leaves partial output on w. // // The run's rule catalog lists only the rules referenced by issues, sorted by rule ID; a rule -// missing from f.Rules is listed with its ID alone. Issue paths become forward-slash URIs relative -// to %SRCROOT%, so uploads map to repository files when decolint runs at the repository root. +// missing from f.Rules is listed with its ID alone. Issue paths are reported as URIs; see +// [artifactLocationFor]. func (f SARIFFormat) WriteIssues(w io.Writer, issues []linter.Issue) error { catalog := make(map[string]SARIFRule, len(f.Rules)) for _, r := range f.Rules { @@ -145,11 +150,8 @@ func (f SARIFFormat) WriteIssues(w io.Writer, issues []linter.Issue) error { Message: sarifMessage{Text: issue.Message}, Locations: []sarifLocation{{ PhysicalLocation: sarifPhysicalLocation{ - ArtifactLocation: sarifArtifactLocation{ - URI: filepath.ToSlash(issue.Path), - URIBaseID: "%SRCROOT%", - }, - Region: sarifRegion{StartLine: issue.Line, StartColumn: issue.Col}, + ArtifactLocation: artifactLocationFor(issue.Path), + Region: sarifRegion{StartLine: issue.Line, StartColumn: issue.Col}, }, }}, }) @@ -179,3 +181,16 @@ func (f SARIFFormat) WriteIssues(w io.Writer, issues []linter.Issue) error { } return nil } + +// artifactLocationFor returns the SARIF location of the file at path, whose members must be a URI +// rather than a filesystem path. A relative path is reported against [srcRootBaseID]; an absolute +// one, which decolint reports for a file outside the directory it runs in, becomes an absolute file +// URI, since resolving it against the source root would name a different file. +func artifactLocationFor(path string) sarifArtifactLocation { + slashed := filepath.ToSlash(path) + if !filepath.IsAbs(path) { + return sarifArtifactLocation{URI: (&url.URL{Path: slashed}).String(), URIBaseID: srcRootBaseID} + } + // A Windows path is absolute without a leading "/", which a file URI's path needs. + return sarifArtifactLocation{URI: (&url.URL{Scheme: "file", Path: "/" + strings.TrimPrefix(slashed, "/")}).String()} +} diff --git a/format/sarif_test.go b/format/sarif_test.go index d431427..f13ec37 100644 --- a/format/sarif_test.go +++ b/format/sarif_test.go @@ -2,6 +2,7 @@ package format import ( "errors" + "path/filepath" "strings" "testing" ) @@ -43,6 +44,53 @@ func TestSARIFWriteIssues(t *testing.T) { } } +// TestSARIFWriteIssues_ArtifactLocation checks how a finding's path becomes a SARIF artifact +// location: a path built with the host's separators is reported against the source root with "/" +// ones, while an absolute path — which decolint reports for a file outside the directory it runs in +// — is reported as a file URI that no base id applies to. +func TestSARIFWriteIssues_ArtifactLocation(t *testing.T) { + t.Parallel() + + t.Run("relative path", func(t *testing.T) { + t.Parallel() + + issues := testIssues()[:1] + issues[0].Path = filepath.Join(".devcontainer", "go", "devcontainer.json") + + var sb strings.Builder + if err := testSARIFFormat().WriteIssues(&sb, issues); err != nil { + t.Fatalf("WriteIssues: %v", err) + } + + want := `"artifactLocation":{"uri":".devcontainer/go/devcontainer.json","uriBaseId":"%SRCROOT%"}` + if !strings.Contains(sb.String(), want) { + t.Errorf("WriteIssues sarif = %q, want it to contain %q", sb.String(), want) + } + }) + + t.Run("absolute path", func(t *testing.T) { + t.Parallel() + + issues := testIssues()[:1] + issues[0].Path = filepath.Join(t.TempDir(), "devcontainer.json") + + var sb strings.Builder + if err := testSARIFFormat().WriteIssues(&sb, issues); err != nil { + t.Fatalf("WriteIssues: %v", err) + } + + // The URI's own path varies with the host, so only its form is asserted: a file URI with an + // empty authority, and no base id to resolve it against. + out := sb.String() + if !strings.Contains(out, `"artifactLocation":{"uri":"file:///`) { + t.Errorf("WriteIssues sarif = %q, want an absolute file URI", out) + } + if strings.Contains(out, srcRootBaseID) { + t.Errorf("WriteIssues sarif = %q, want no %s for an absolute path", out, srcRootBaseID) + } + }) +} + func TestSARIFWriteIssues_Empty(t *testing.T) { t.Parallel() From 31538666bb3d0017988db2c5d379715ff0401464 Mon Sep 17 00:00:00 2001 From: Claude Date: Sat, 25 Jul 2026 02:04:38 +0000 Subject: [PATCH 4/8] test(cli): cover the SARIF location of a directory outside the working directory The choice between a relative path and an absolute one is made when the lint target is resolved, not in the format package, so exercise it end to end: a target outside the working directory must reach the SARIF log as an absolute file URI carrying no base id. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01EUTAwRxUmzDnAPFyR3ESby --- cmd/decolint/main_test.go | 47 +++++++++++++++++++++++++++++++++++++++ 1 file changed, 47 insertions(+) diff --git a/cmd/decolint/main_test.go b/cmd/decolint/main_test.go index 5a6ceca..ea4031c 100644 --- a/cmd/decolint/main_test.go +++ b/cmd/decolint/main_test.go @@ -9,6 +9,7 @@ import ( "io" "io/fs" "os" + "path" "path/filepath" "strings" "testing" @@ -646,6 +647,52 @@ func TestRun_OutputFormat(t *testing.T) { } }) + t.Run("sarif log outside the working directory", func(t *testing.T) { + t.Parallel() + + // A directory outside the working directory is reported absolutely, which SARIF can only + // express as an absolute file URI: no source root it could be relative to applies. The + // fixture trips missing-container-def, an error by default and free of any platform or fetch. + dir := writeDevcontainer(t, `{}`) + + var stdout, stderr bytes.Buffer + exitCode := run(t.Context(), []string{"-format=sarif", dir}, &stdout, &stderr) + if exitCode != 1 { + t.Fatalf("exit code = %d, want 1; stderr: %s", exitCode, stderr.String()) + } + + var log sarifLog + if err := json.Unmarshal(stdout.Bytes(), &log); err != nil { + t.Fatalf("output is not a SARIF log: %v\noutput: %s", err, stdout.String()) + } + + // A file URI's path carries exactly one leading slash, which a POSIX path already has and a + // Windows one does not. + config := filepath.Join(dir, ".devcontainer", "devcontainer.json") + uri := "file://" + path.Join("/", filepath.ToSlash(config)) + want := sarifLog{ + Version: "2.1.0", + Runs: []sarifRun{{ + Tool: sarifTool{Driver: sarifDriver{ + Name: "decolint", + Rules: []sarifRule{{ID: "missing-container-def"}}, + }}, + Results: []sarifResult{{ + RuleID: "missing-container-def", + RuleIndex: 0, + Level: "error", + // No uriBaseId: an absolute URI is not resolved against a source root. + Locations: []sarifLocation{{PhysicalLocation: sarifPhysicalLocation{ + ArtifactLocation: sarifArtifactLocation{URI: uri}, + }}}, + }}, + }}, + } + if diff := cmp.Diff(want, log); diff != "" { + t.Errorf("sarif log mismatch (-want +got):\n%s", diff) + } + }) + t.Run("format selected by config file", func(t *testing.T) { t.Parallel() From 8c841952560b4cca6ecf03d88f277a5c421b6931 Mon Sep 17 00:00:00 2001 From: Claude Date: Sat, 25 Jul 2026 02:11:42 +0000 Subject: [PATCH 5/8] fix(sarif): declare the %SRCROOT% base id in the run A relative artifact location referenced a base id that the log never declared, which is invalid SARIF. Declare it in originalUriBaseIds without a URI: decolint does not know where its working directory sits in the analyzed project, so the value is left to the consumer, which resolves it to the root of the checkout. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01EUTAwRxUmzDnAPFyR3ESby --- format/sarif.go | 20 ++++++++++++++++---- format/sarif_test.go | 15 ++++++++++----- 2 files changed, 26 insertions(+), 9 deletions(-) diff --git a/format/sarif.go b/format/sarif.go index 505efcc..fec635b 100644 --- a/format/sarif.go +++ b/format/sarif.go @@ -34,8 +34,9 @@ const ( sarifSchemaURI = "https://json.schemastore.org/sarif-2.1.0.json" sarifVersion = "2.1.0" informationURI = "https://github.com/bare-devcontainer/decolint" - // srcRootBaseID is the SARIF base id a relative path is reported against. An upload resolves it - // to the root of the analyzed checkout. + // srcRootBaseID is the SARIF base id a relative path is reported against. It is declared in the + // run but left unresolved (see sarifURIBase); a Code Scanning upload resolves it to the root of + // the analyzed checkout. srcRootBaseID = "%SRCROOT%" ) @@ -49,8 +50,16 @@ type sarifLog struct { } type sarifRun struct { - Tool sarifTool `json:"tool"` - Results []sarifResult `json:"results"` + Tool sarifTool `json:"tool"` + OriginalURIBaseIDs map[string]sarifURIBase `json:"originalUriBaseIds"` + Results []sarifResult `json:"results"` +} + +// sarifURIBase declares a base id without resolving it. A base id may carry the absolute URI it +// stands for, but decolint does not know where its working directory sits in the analyzed project, +// so it leaves the value to the consumer, which does. +type sarifURIBase struct { + Description sarifMessage `json:"description"` } type sarifTool struct { @@ -167,6 +176,9 @@ func (f SARIFFormat) WriteIssues(w io.Writer, issues []linter.Issue) error { InformationURI: informationURI, Rules: descriptors, }}, + OriginalURIBaseIDs: map[string]sarifURIBase{srcRootBaseID: { + Description: sarifMessage{Text: "The directory decolint ran in, which the reported paths are relative to."}, + }}, Results: results, }}, } diff --git a/format/sarif_test.go b/format/sarif_test.go index f13ec37..87d8dea 100644 --- a/format/sarif_test.go +++ b/format/sarif_test.go @@ -33,7 +33,9 @@ func TestSARIFWriteIssues(t *testing.T) { want := `{"$schema":"https://json.schemastore.org/sarif-2.1.0.json","version":"2.1.0","runs":[` + `{"tool":{"driver":{"name":"decolint","version":"1.2.3","informationUri":"https://github.com/bare-devcontainer/decolint","rules":[` + `{"id":"no-image-latest","shortDescription":{"text":"images should be pinned to a specific version"},"properties":{"tags":["reproducibility"]}},` + - `{"id":"some-error-rule"}]}},"results":[` + + `{"id":"some-error-rule"}]}},` + + `"originalUriBaseIds":{"%SRCROOT%":{"description":{"text":"The directory decolint ran in, which the reported paths are relative to."}}},` + + `"results":[` + `{"ruleId":"no-image-latest","ruleIndex":0,"level":"warning","message":{"text":"image \"ubuntu:latest\" uses the \"latest\" tag; pin a specific version"},` + `"locations":[{"physicalLocation":{"artifactLocation":{"uri":".devcontainer/devcontainer.json","uriBaseId":"%SRCROOT%"},"region":{"startLine":4,"startColumn":12}}}]},` + `{"ruleId":"some-error-rule","ruleIndex":1,"level":"error","message":{"text":"something is broken"},` + @@ -80,13 +82,14 @@ func TestSARIFWriteIssues_ArtifactLocation(t *testing.T) { } // The URI's own path varies with the host, so only its form is asserted: a file URI with an - // empty authority, and no base id to resolve it against. + // empty authority, and no base id to resolve it against. The run still declares the base id, + // so the absence is asserted on the member rather than on the name. out := sb.String() if !strings.Contains(out, `"artifactLocation":{"uri":"file:///`) { t.Errorf("WriteIssues sarif = %q, want an absolute file URI", out) } - if strings.Contains(out, srcRootBaseID) { - t.Errorf("WriteIssues sarif = %q, want no %s for an absolute path", out, srcRootBaseID) + if strings.Contains(out, `"uriBaseId"`) { + t.Errorf("WriteIssues sarif = %q, want no base id for an absolute path", out) } }) } @@ -100,7 +103,9 @@ func TestSARIFWriteIssues_Empty(t *testing.T) { } want := `{"$schema":"https://json.schemastore.org/sarif-2.1.0.json","version":"2.1.0","runs":[` + - `{"tool":{"driver":{"name":"decolint","version":"1.2.3","informationUri":"https://github.com/bare-devcontainer/decolint","rules":[]}},"results":[]}]}` + + `{"tool":{"driver":{"name":"decolint","version":"1.2.3","informationUri":"https://github.com/bare-devcontainer/decolint","rules":[]}},` + + `"originalUriBaseIds":{"%SRCROOT%":{"description":{"text":"The directory decolint ran in, which the reported paths are relative to."}}},` + + `"results":[]}]}` + "\n" if sb.String() != want { t.Errorf("WriteIssues sarif (empty) = %q, want %q", sb.String(), want) From bb1f24653fb012ac63327e2ff771db659beb7245 Mon Sep 17 00:00:00 2001 From: Claude Date: Sat, 25 Jul 2026 02:23:59 +0000 Subject: [PATCH 6/8] fix(sarif): drop the uriBaseId indirection A base id is only useful to a consumer that can resolve it, and nothing in the log gave %SRCROOT% a value: Code Scanning does not resolve base ids at all, and a viewer that tries is left with an unresolvable reference. Report a relative path on its own, as golangci-lint and oxlint do, and let the consumer resolve it against the root of the analyzed project. Also write a UNC path's host as the file URI's authority rather than folding it into the path, and name the sarif format in the config file template written by -init. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01EUTAwRxUmzDnAPFyR3ESby --- cmd/decolint/init.go | 4 ++-- cmd/decolint/main_test.go | 12 +++++----- format/sarif.go | 47 +++++++++++++++------------------------ format/sarif_test.go | 46 ++++++++++++++++++++++++-------------- 4 files changed, 54 insertions(+), 55 deletions(-) diff --git a/cmd/decolint/init.go b/cmd/decolint/init.go index c6c1f57..f4a4c45 100644 --- a/cmd/decolint/init.go +++ b/cmd/decolint/init.go @@ -43,8 +43,8 @@ func initConfigFile(output io.Writer) error { // "denyWarnings", when true, treats warnings as failures (exit code 1); // the -deny-warnings flag takes precedence, e.g.: // "denyWarnings": true -// "format" selects the output format ("text", "json", or "github"); the -// -format flag takes precedence, e.g.: +// "format" selects the output format ("text", "json", "github", or +// "sarif"); the -format flag takes precedence, e.g.: // "format": "github" // "localEnv" supplies the values ${localEnv:NAME} resolves to when merging, // and the environment Compose-file interpolation reads; environment diff --git a/cmd/decolint/main_test.go b/cmd/decolint/main_test.go index ea4031c..7b74891 100644 --- a/cmd/decolint/main_test.go +++ b/cmd/decolint/main_test.go @@ -552,8 +552,7 @@ type ( ArtifactLocation sarifArtifactLocation `json:"artifactLocation"` } sarifArtifactLocation struct { - URI string `json:"uri"` - URIBaseID string `json:"uriBaseId"` + URI string `json:"uri"` } ) @@ -620,10 +619,10 @@ func TestRun_OutputFormat(t *testing.T) { t.Fatalf("output is not a SARIF log: %v\noutput: %s", err, stdout.String()) } - // The fixture is inside the working directory, so it is reported relative to the source root. + // The fixture is inside the working directory, so it is reported relative to it. locAt := func(uri string) []sarifLocation { return []sarifLocation{{PhysicalLocation: sarifPhysicalLocation{ - ArtifactLocation: sarifArtifactLocation{URI: uri, URIBaseID: "%SRCROOT%"}, + ArtifactLocation: sarifArtifactLocation{URI: uri}, }}} } // The catalog lists only referenced rules, sorted by ID, so each result's ruleIndex points at @@ -651,8 +650,8 @@ func TestRun_OutputFormat(t *testing.T) { t.Parallel() // A directory outside the working directory is reported absolutely, which SARIF can only - // express as an absolute file URI: no source root it could be relative to applies. The - // fixture trips missing-container-def, an error by default and free of any platform or fetch. + // express as an absolute file URI. The fixture trips missing-container-def, an error by + // default and free of any platform or fetch. dir := writeDevcontainer(t, `{}`) var stdout, stderr bytes.Buffer @@ -681,7 +680,6 @@ func TestRun_OutputFormat(t *testing.T) { RuleID: "missing-container-def", RuleIndex: 0, Level: "error", - // No uriBaseId: an absolute URI is not resolved against a source root. Locations: []sarifLocation{{PhysicalLocation: sarifPhysicalLocation{ ArtifactLocation: sarifArtifactLocation{URI: uri}, }}}, diff --git a/format/sarif.go b/format/sarif.go index fec635b..8bc5045 100644 --- a/format/sarif.go +++ b/format/sarif.go @@ -34,10 +34,6 @@ const ( sarifSchemaURI = "https://json.schemastore.org/sarif-2.1.0.json" sarifVersion = "2.1.0" informationURI = "https://github.com/bare-devcontainer/decolint" - // srcRootBaseID is the SARIF base id a relative path is reported against. It is declared in the - // run but left unresolved (see sarifURIBase); a Code Scanning upload resolves it to the root of - // the analyzed checkout. - srcRootBaseID = "%SRCROOT%" ) // The wire structs cover the subset of SARIF 2.1.0 decolint emits. Fields marshal in declaration @@ -50,16 +46,8 @@ type sarifLog struct { } type sarifRun struct { - Tool sarifTool `json:"tool"` - OriginalURIBaseIDs map[string]sarifURIBase `json:"originalUriBaseIds"` - Results []sarifResult `json:"results"` -} - -// sarifURIBase declares a base id without resolving it. A base id may carry the absolute URI it -// stands for, but decolint does not know where its working directory sits in the analyzed project, -// so it leaves the value to the consumer, which does. -type sarifURIBase struct { - Description sarifMessage `json:"description"` + Tool sarifTool `json:"tool"` + Results []sarifResult `json:"results"` } type sarifTool struct { @@ -105,8 +93,7 @@ type sarifPhysicalLocation struct { } type sarifArtifactLocation struct { - URI string `json:"uri"` - URIBaseID string `json:"uriBaseId,omitzero"` + URI string `json:"uri"` } type sarifRegion struct { @@ -119,7 +106,7 @@ type sarifRegion struct { // // The run's rule catalog lists only the rules referenced by issues, sorted by rule ID; a rule // missing from f.Rules is listed with its ID alone. Issue paths are reported as URIs; see -// [artifactLocationFor]. +// [artifactURIFor]. func (f SARIFFormat) WriteIssues(w io.Writer, issues []linter.Issue) error { catalog := make(map[string]SARIFRule, len(f.Rules)) for _, r := range f.Rules { @@ -159,7 +146,7 @@ func (f SARIFFormat) WriteIssues(w io.Writer, issues []linter.Issue) error { Message: sarifMessage{Text: issue.Message}, Locations: []sarifLocation{{ PhysicalLocation: sarifPhysicalLocation{ - ArtifactLocation: artifactLocationFor(issue.Path), + ArtifactLocation: sarifArtifactLocation{URI: artifactURIFor(issue.Path)}, Region: sarifRegion{StartLine: issue.Line, StartColumn: issue.Col}, }, }}, @@ -176,9 +163,6 @@ func (f SARIFFormat) WriteIssues(w io.Writer, issues []linter.Issue) error { InformationURI: informationURI, Rules: descriptors, }}, - OriginalURIBaseIDs: map[string]sarifURIBase{srcRootBaseID: { - Description: sarifMessage{Text: "The directory decolint ran in, which the reported paths are relative to."}, - }}, Results: results, }}, } @@ -194,15 +178,20 @@ func (f SARIFFormat) WriteIssues(w io.Writer, issues []linter.Issue) error { return nil } -// artifactLocationFor returns the SARIF location of the file at path, whose members must be a URI -// rather than a filesystem path. A relative path is reported against [srcRootBaseID]; an absolute -// one, which decolint reports for a file outside the directory it runs in, becomes an absolute file -// URI, since resolving it against the source root would name a different file. -func artifactLocationFor(path string) sarifArtifactLocation { +// artifactURIFor returns the SARIF location of the file at path, which must be a URI rather than a +// filesystem path. A relative path stays relative, for the consumer to resolve against the root of +// the analyzed project; an absolute one, which decolint reports for a file outside the directory it +// runs in, becomes an absolute file URI, since resolving it that way would name a different file. +func artifactURIFor(path string) string { slashed := filepath.ToSlash(path) if !filepath.IsAbs(path) { - return sarifArtifactLocation{URI: (&url.URL{Path: slashed}).String(), URIBaseID: srcRootBaseID} + return (&url.URL{Path: slashed}).String() + } + // A UNC path names a host, which a file URI carries as its authority rather than in its path. + if rest, ok := strings.CutPrefix(slashed, "//"); ok { + host, share, _ := strings.Cut(rest, "/") + return (&url.URL{Scheme: "file", Host: host, Path: "/" + share}).String() } - // A Windows path is absolute without a leading "/", which a file URI's path needs. - return sarifArtifactLocation{URI: (&url.URL{Scheme: "file", Path: "/" + strings.TrimPrefix(slashed, "/")}).String()} + // A Windows drive path is absolute without a leading "/", which a file URI's path needs. + return (&url.URL{Scheme: "file", Path: "/" + strings.TrimPrefix(slashed, "/")}).String() } diff --git a/format/sarif_test.go b/format/sarif_test.go index 87d8dea..3276a18 100644 --- a/format/sarif_test.go +++ b/format/sarif_test.go @@ -33,13 +33,11 @@ func TestSARIFWriteIssues(t *testing.T) { want := `{"$schema":"https://json.schemastore.org/sarif-2.1.0.json","version":"2.1.0","runs":[` + `{"tool":{"driver":{"name":"decolint","version":"1.2.3","informationUri":"https://github.com/bare-devcontainer/decolint","rules":[` + `{"id":"no-image-latest","shortDescription":{"text":"images should be pinned to a specific version"},"properties":{"tags":["reproducibility"]}},` + - `{"id":"some-error-rule"}]}},` + - `"originalUriBaseIds":{"%SRCROOT%":{"description":{"text":"The directory decolint ran in, which the reported paths are relative to."}}},` + - `"results":[` + + `{"id":"some-error-rule"}]}},"results":[` + `{"ruleId":"no-image-latest","ruleIndex":0,"level":"warning","message":{"text":"image \"ubuntu:latest\" uses the \"latest\" tag; pin a specific version"},` + - `"locations":[{"physicalLocation":{"artifactLocation":{"uri":".devcontainer/devcontainer.json","uriBaseId":"%SRCROOT%"},"region":{"startLine":4,"startColumn":12}}}]},` + + `"locations":[{"physicalLocation":{"artifactLocation":{"uri":".devcontainer/devcontainer.json"},"region":{"startLine":4,"startColumn":12}}}]},` + `{"ruleId":"some-error-rule","ruleIndex":1,"level":"error","message":{"text":"something is broken"},` + - `"locations":[{"physicalLocation":{"artifactLocation":{"uri":".devcontainer/devcontainer.json","uriBaseId":"%SRCROOT%"},"region":{"startLine":8,"startColumn":3}}}]}]}]}` + + `"locations":[{"physicalLocation":{"artifactLocation":{"uri":".devcontainer/devcontainer.json"},"region":{"startLine":8,"startColumn":3}}}]}]}]}` + "\n" if sb.String() != want { t.Errorf("WriteIssues sarif = %q, want %q", sb.String(), want) @@ -47,9 +45,9 @@ func TestSARIFWriteIssues(t *testing.T) { } // TestSARIFWriteIssues_ArtifactLocation checks how a finding's path becomes a SARIF artifact -// location: a path built with the host's separators is reported against the source root with "/" -// ones, while an absolute path — which decolint reports for a file outside the directory it runs in -// — is reported as a file URI that no base id applies to. +// location: a path built with the host's separators stays relative but is written with "/" ones, +// while an absolute path — which decolint reports for a file outside the directory it runs in — is +// written as a file URI. func TestSARIFWriteIssues_ArtifactLocation(t *testing.T) { t.Parallel() @@ -64,7 +62,27 @@ func TestSARIFWriteIssues_ArtifactLocation(t *testing.T) { t.Fatalf("WriteIssues: %v", err) } - want := `"artifactLocation":{"uri":".devcontainer/go/devcontainer.json","uriBaseId":"%SRCROOT%"}` + want := `"artifactLocation":{"uri":".devcontainer/go/devcontainer.json"}` + if !strings.Contains(sb.String(), want) { + t.Errorf("WriteIssues sarif = %q, want it to contain %q", sb.String(), want) + } + }) + + t.Run("UNC path", func(t *testing.T) { + t.Parallel() + + // A UNC path names its host in the leading "//" segment, which belongs in the URI's authority + // rather than its path. The path is given in the form filepath.ToSlash yields for it on + // Windows, which is absolute on every host, so this holds off Windows too. + issues := testIssues()[:1] + issues[0].Path = "//server/share/devcontainer.json" + + var sb strings.Builder + if err := testSARIFFormat().WriteIssues(&sb, issues); err != nil { + t.Fatalf("WriteIssues: %v", err) + } + + want := `"artifactLocation":{"uri":"file://server/share/devcontainer.json"}` if !strings.Contains(sb.String(), want) { t.Errorf("WriteIssues sarif = %q, want it to contain %q", sb.String(), want) } @@ -82,15 +100,11 @@ func TestSARIFWriteIssues_ArtifactLocation(t *testing.T) { } // The URI's own path varies with the host, so only its form is asserted: a file URI with an - // empty authority, and no base id to resolve it against. The run still declares the base id, - // so the absence is asserted on the member rather than on the name. + // empty authority. out := sb.String() if !strings.Contains(out, `"artifactLocation":{"uri":"file:///`) { t.Errorf("WriteIssues sarif = %q, want an absolute file URI", out) } - if strings.Contains(out, `"uriBaseId"`) { - t.Errorf("WriteIssues sarif = %q, want no base id for an absolute path", out) - } }) } @@ -103,9 +117,7 @@ func TestSARIFWriteIssues_Empty(t *testing.T) { } want := `{"$schema":"https://json.schemastore.org/sarif-2.1.0.json","version":"2.1.0","runs":[` + - `{"tool":{"driver":{"name":"decolint","version":"1.2.3","informationUri":"https://github.com/bare-devcontainer/decolint","rules":[]}},` + - `"originalUriBaseIds":{"%SRCROOT%":{"description":{"text":"The directory decolint ran in, which the reported paths are relative to."}}},` + - `"results":[]}]}` + + `{"tool":{"driver":{"name":"decolint","version":"1.2.3","informationUri":"https://github.com/bare-devcontainer/decolint","rules":[]}},"results":[]}]}` + "\n" if sb.String() != want { t.Errorf("WriteIssues sarif (empty) = %q, want %q", sb.String(), want) From b35e746dc01a4167488d809f0c2fc2070138cc7b Mon Sep 17 00:00:00 2001 From: Claude Date: Sat, 25 Jul 2026 02:53:13 +0000 Subject: [PATCH 7/8] ci: verify the SARIF output uploads to code scanning Everything about the sarif format has been checked offline so far, which cannot show that GitHub accepts the log or that the relative paths it reports resolve to files in the repository. Upload a real log for the violations fixture, assert the analysis was ingested and its alerts landed on the linted file, then delete the analysis and fail if anything is left behind. The upload gets a category of its own so it forms an independent analysis set, and the cleanup deletes only that category. Manual only: a pull request from a fork never receives security-events: write. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01EUTAwRxUmzDnAPFyR3ESby --- .github/workflows/sarif-upload-check.yml | 121 +++++++++++++++++++++++ 1 file changed, 121 insertions(+) create mode 100644 .github/workflows/sarif-upload-check.yml diff --git a/.github/workflows/sarif-upload-check.yml b/.github/workflows/sarif-upload-check.yml new file mode 100644 index 0000000..4d7afd0 --- /dev/null +++ b/.github/workflows/sarif-upload-check.yml @@ -0,0 +1,121 @@ +name: SARIF Upload Check + +# Uploads a real decolint SARIF log to code scanning to prove GitHub accepts it and resolves the +# reported paths to files in the repository, then deletes the analysis so the Security tab is left +# as it was found. Manual only: a pull request from a fork never receives security-events: write. +on: + workflow_dispatch: + +permissions: {} + +env: + # A category of its own puts this check in an analysis set of its own, so it can neither replace + # nor be replaced by a real analysis, and the cleanup can target it exactly. + CATEGORY: decolint-sarif-upload-check + TOOL_NAME: decolint + FIXTURE: cmd/decolint/testdata/e2e/violations + +jobs: + upload-check: + runs-on: ubuntu-latest + permissions: + contents: read + security-events: write + steps: + - uses: actions/checkout@9c091bb21b7c1c1d1991bb908d89e4e9dddfe3e0 # v7.0.0 + with: + persist-credentials: false + - uses: actions/setup-go@924ae3a1cded613372ab5595356fb5720e22ba16 # v6.5.0 + with: + go-version-file: go.mod + - run: make build + + - name: Generate a SARIF log + # decolint exits 1 when it reports findings, which is what this check wants; any other + # non-zero status is a real failure. It runs from the repository root so that the reported + # paths are relative to it, which is how code scanning resolves them. Merging is off so the + # run needs no registry access and cannot fail for reasons unrelated to the upload. + run: ./bin/decolint -format=sarif -merge=false "$FIXTURE" > decolint.sarif || [ "$?" -eq 1 ] + + - name: Upload the SARIF log + id: upload + # wait-for-processing defaults to true, so this step fails if code scanning rejects the log. + uses: github/codeql-action/upload-sarif@99df26d4f13ea111d4ec1a7dddef6063f76b97e9 # v4.37.0 + with: + sarif_file: decolint.sarif + category: ${{ env.CATEGORY }} + + - name: Check the analysis + env: + GH_TOKEN: ${{ github.token }} + SARIF_ID: ${{ steps.upload.outputs.sarif-id }} + run: | + analyses=$(gh api "repos/$GITHUB_REPOSITORY/code-scanning/analyses?sarif_id=$SARIF_ID") + echo "$analyses" | jq . + # The category is stored as the automation id, which carries a trailing slash. + echo "$analyses" | jq -e --arg tool "$TOOL_NAME" --arg category "$CATEGORY" ' + length == 1 + and .[0].tool.name == $tool + and (.[0].category // "" | rtrimstr("/")) == $category + and .[0].results_count > 0 + ' > /dev/null + + - name: Check the findings resolve to repository files + env: + GH_TOKEN: ${{ github.token }} + run: | + want=$(jq -nc --arg path "$FIXTURE/.devcontainer/devcontainer.json" '[$path]') + # Alerts can lag the analysis, so give them a bounded chance to appear. + for _ in $(seq 1 10); do + got=$(gh api "repos/$GITHUB_REPOSITORY/code-scanning/alerts?tool_name=$TOOL_NAME&ref=$GITHUB_REF" 2>/dev/null || echo '[]') + got=$(printf '%s' "$got" | jq -c '[.[] | .most_recent_instance.location.path] | unique') + if [ "$got" = "$want" ]; then + echo "alerts resolve to $got" + exit 0 + fi + sleep 5 + done + echo "::error::alert paths are $got, want $want" + exit 1 + + - name: Delete the uploaded analysis + # Runs even when a check above failed, so a failure never leaves an analysis behind. + if: always() + env: + GH_TOKEN: ${{ github.token }} + run: | + api() { gh api "repos/$GITHUB_REPOSITORY/$1" 2>/dev/null || echo '[]'; } + # Swept by tool and category rather than by the id of this upload, so that anything an + # earlier failed run left behind is collected too. + analyses() { + api "code-scanning/analyses?tool_name=$TOOL_NAME&ref=$GITHUB_REF" \ + | jq -c --arg category "$CATEGORY" '[.[] | select((.category // "" | rtrimstr("/")) == $category)]' + } + # The alerts endpoint cannot filter by category, so this sweeps every decolint alert on the + # ref. That is exact while this workflow is decolint's only uploader; should a real + # decolint analysis ever be added, narrow it, or it will report that alert as left behind. + alerts() { api "code-scanning/alerts?tool_name=$TOOL_NAME&ref=$GITHUB_REF"; } + + # Only the most recent analysis of a set is deletable, so deleting a set takes one request + # per analysis. confirm_delete is required for the last one, which also discards its + # alerts. The loop is bounded so an analysis that refuses to go cannot hang the job. + for _ in $(seq 1 20); do + id=$(analyses | jq -r '[.[] | select(.deletable)] | .[0].id // empty') + if [ -z "$id" ]; then + break + fi + echo "deleting analysis $id" + gh api --method DELETE "repos/$GITHUB_REPOSITORY/code-scanning/analyses/$id?confirm_delete=true" --silent + done + + for _ in $(seq 1 10); do + left_analyses=$(analyses | jq 'length') + left_alerts=$(alerts | jq 'length') + if [ "$left_analyses" -eq 0 ] && [ "$left_alerts" -eq 0 ]; then + echo "nothing was left behind" + exit 0 + fi + sleep 3 + done + echo "::error::$left_analyses analyses and $left_alerts alerts were left behind" + exit 1 From 2408ed37086d82dd90d5571d3ef0ca18ced3b296 Mon Sep 17 00:00:00 2001 From: Claude Date: Sat, 25 Jul 2026 03:25:24 +0000 Subject: [PATCH 8/8] ci: run the SARIF upload check on pull requests that edit it Nothing triggers this workflow automatically, so a change that breaks it is only discovered the next time someone runs it by hand. Run it on a pull request that touches the workflow itself. An upload picks the ref its analysis lands on, and for a pull request that is a refs/pull ref rather than GITHUB_REF, so read it back from the analysis instead of assuming it, and sweep analyses by category across every ref. A pull request from a fork is skipped: its token is read-only however this workflow declares its permissions. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01EUTAwRxUmzDnAPFyR3ESby --- .github/workflows/sarif-upload-check.yml | 25 +++++++++++++++++------- 1 file changed, 18 insertions(+), 7 deletions(-) diff --git a/.github/workflows/sarif-upload-check.yml b/.github/workflows/sarif-upload-check.yml index 4d7afd0..4abf930 100644 --- a/.github/workflows/sarif-upload-check.yml +++ b/.github/workflows/sarif-upload-check.yml @@ -2,9 +2,13 @@ name: SARIF Upload Check # Uploads a real decolint SARIF log to code scanning to prove GitHub accepts it and resolves the # reported paths to files in the repository, then deletes the analysis so the Security tab is left -# as it was found. Manual only: a pull request from a fork never receives security-events: write. +# as it was found. Runs on demand, and on a pull request that edits this workflow, since a workflow +# nothing triggers automatically is otherwise only proven broken the next time someone runs it. on: workflow_dispatch: + pull_request: + paths: + - .github/workflows/sarif-upload-check.yml permissions: {} @@ -18,6 +22,9 @@ env: jobs: upload-check: runs-on: ubuntu-latest + # A pull request from a fork is given a read-only token however this workflow declares its + # permissions, so the upload could only fail. Skip it rather than report a failure. + if: github.event_name != 'pull_request' || github.event.pull_request.head.repo.full_name == github.repository permissions: contents: read security-events: write @@ -59,6 +66,9 @@ jobs: and (.[0].category // "" | rtrimstr("/")) == $category and .[0].results_count > 0 ' > /dev/null + # The upload picks the ref the analysis lands on, which for a pull request is a refs/pull + # ref rather than GITHUB_REF, so take it from the analysis instead of assuming it. + echo "ANALYSIS_REF=$(echo "$analyses" | jq -r '.[0].ref')" >> "$GITHUB_ENV" - name: Check the findings resolve to repository files env: @@ -67,10 +77,10 @@ jobs: want=$(jq -nc --arg path "$FIXTURE/.devcontainer/devcontainer.json" '[$path]') # Alerts can lag the analysis, so give them a bounded chance to appear. for _ in $(seq 1 10); do - got=$(gh api "repos/$GITHUB_REPOSITORY/code-scanning/alerts?tool_name=$TOOL_NAME&ref=$GITHUB_REF" 2>/dev/null || echo '[]') + got=$(gh api "repos/$GITHUB_REPOSITORY/code-scanning/alerts?tool_name=$TOOL_NAME&ref=$ANALYSIS_REF&per_page=100" 2>/dev/null || echo '[]') got=$(printf '%s' "$got" | jq -c '[.[] | .most_recent_instance.location.path] | unique') if [ "$got" = "$want" ]; then - echo "alerts resolve to $got" + echo "alerts on $ANALYSIS_REF resolve to $got" exit 0 fi sleep 5 @@ -84,17 +94,18 @@ jobs: env: GH_TOKEN: ${{ github.token }} run: | + ref="${ANALYSIS_REF:-$GITHUB_REF}" api() { gh api "repos/$GITHUB_REPOSITORY/$1" 2>/dev/null || echo '[]'; } - # Swept by tool and category rather than by the id of this upload, so that anything an - # earlier failed run left behind is collected too. + # Not filtered by ref: the category belongs to this workflow alone, so an analysis carrying + # it is one of ours whichever ref it landed on, including one an earlier run left behind. analyses() { - api "code-scanning/analyses?tool_name=$TOOL_NAME&ref=$GITHUB_REF" \ + api "code-scanning/analyses?tool_name=$TOOL_NAME&per_page=100" \ | jq -c --arg category "$CATEGORY" '[.[] | select((.category // "" | rtrimstr("/")) == $category)]' } # The alerts endpoint cannot filter by category, so this sweeps every decolint alert on the # ref. That is exact while this workflow is decolint's only uploader; should a real # decolint analysis ever be added, narrow it, or it will report that alert as left behind. - alerts() { api "code-scanning/alerts?tool_name=$TOOL_NAME&ref=$GITHUB_REF"; } + alerts() { api "code-scanning/alerts?tool_name=$TOOL_NAME&ref=$ref&per_page=100"; } # Only the most recent analysis of a set is deletable, so deleting a set takes one request # per analysis. confirm_delete is required for the last one, which also discards its