Skip to content

fix: interpret span correctly for github format #1621

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
SoMuchForSubtlety wants to merge 2 commits into
base: main
Choose a base branch
from
+38 −67
Open

fix: interpret span correctly for github format #1621

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
76a76d4
fix: interpret span correctly for github format
SoMuchForSubtlety Apr 9, 2026
bac5bc6
fix docstrings
SoMuchForSubtlety Apr 9, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
23 changes: 5 additions & 18 deletions cmd/api-linter/github_actions.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,34 +32,21 @@ func formatGitHubActionOutput(responses []lint.Response) []byte {
// https://docs.github.com/en/actions/using-workflows/workflow-commands-for-github-actions#setting-an-error-message

fmt.Fprintf(&buf, "::error file=%s", response.FilePath)

if problem.Location != nil {
// Some findings are *line level* and only have start positions but no
// starting column. Construct a switch fallthrough to emit as many of
// the location indicators are included.
switch len(problem.Location.Span) {
case 4:
fmt.Fprintf(&buf, ",endColumn=%d", problem.Location.Span[3])
fallthrough
case 3:
fmt.Fprintf(&buf, ",endLine=%d", problem.Location.Span[2])
fallthrough
case 2:
fmt.Fprintf(&buf, ",col=%d", problem.Location.Span[1])
fallthrough
case 1:
fmt.Fprintf(&buf, ",line=%d", problem.Location.Span[0])
}
loc := lint.FileLocationFromPBLocation(problem.Location, nil)
fmt.Fprintf(&buf, ",line=%d,endLine=%d,col=%d,endColumn=%d", loc.Start.Line, loc.End.Line, loc.Start.Column, loc.End.Column)
}

// GitHub uses :: as control characters (which are also used to delimit
// linter rules. In order to prevent confusion, replace the double colon
// with two Armenian full stops which are indistinguishable to my eye.
runeThatLooksLikeTwoColonsButIsActuallyTwoArmenianFullStops := "։։"
title := strings.ReplaceAll(string(problem.RuleID), "::", runeThatLooksLikeTwoColonsButIsActuallyTwoArmenianFullStops)
message := strings.ReplaceAll(problem.Message, "\n", "\\n")
message := strings.ReplaceAll(problem.Message, "\n", "%0A")
uri := problem.GetRuleURI()
if uri != "" {
message += "\\n\\n" + uri
message += "%0A%0A" + uri
}
fmt.Fprintf(&buf, ",title=%s::%s\n", title, message)
}
Expand Down
40 changes: 12 additions & 28 deletions cmd/api-linter/github_actions_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -53,27 +53,11 @@ func TestFormatGitHubActionOutput(t *testing.T) {
Span: []int32{5, 6, 7},
},
},
{
RuleID: "line::col",
Message: "Line and column",
Location: &descriptorpb.SourceCodeInfo_Location{
Span: []int32{5, 6},
},
},
{
RuleID: "line",
Message: "Line only",
Location: &descriptorpb.SourceCodeInfo_Location{
Span: []int32{5},
},
},
},
},
},
want: `::error file=example.proto,endColumn=8,endLine=7,col=6,line=5,title=line։։col։։endLine։։endColumn::line, column, endline, and endColumn
::error file=example.proto,endLine=7,col=6,line=5,title=line։։col։։endLine::Line, column, and endline
::error file=example.proto,col=6,line=5,title=line։։col::Line and column
::error file=example.proto,line=5,title=line::Line only
want: `::error file=example.proto,line=6,endLine=8,col=7,endColumn=8,title=line։։col։։endLine։։endColumn::line, column, endline, and endColumn
::error file=example.proto,line=6,endLine=6,col=7,endColumn=7,title=line։։col։։endLine::Line, column, and endline
`,
},
{
Expand All @@ -90,16 +74,16 @@ func TestFormatGitHubActionOutput(t *testing.T) {
},
{
RuleID: "core::naming_formats::field_names",
Message: "multi\nline\ncomment",
Message: "multi%0Aline%0Acomment",
Location: &descriptorpb.SourceCodeInfo_Location{
Span: []int32{5, 6, 7, 8},
},
},
},
},
},
want: `::error file=example.proto,endColumn=4,endLine=3,col=2,line=1,title=core։։naming_formats։։field_names::\n\nhttps://linter.aip.dev/naming_formats/field_names
::error file=example.proto,endColumn=8,endLine=7,col=6,line=5,title=core։։naming_formats։։field_names::multi\nline\ncomment\n\nhttps://linter.aip.dev/naming_formats/field_names
want: `::error file=example.proto,line=2,endLine=4,col=3,endColumn=4,title=core։։naming_formats։։field_names::%0A%0Ahttps://linter.aip.dev/naming_formats/field_names
::error file=example.proto,line=6,endLine=8,col=7,endColumn=8,title=core։։naming_formats։։field_names::multi%0Aline%0Acomment%0A%0Ahttps://linter.aip.dev/naming_formats/field_names
`,
},
{
Expand Down Expand Up @@ -133,13 +117,13 @@ func TestFormatGitHubActionOutput(t *testing.T) {
},
},
},
want: `::error file=example.proto,title=core։։naming_formats։։field_names::\n\nhttps://linter.aip.dev/naming_formats/field_names
::error file=example.proto,title=core։։naming_formats։։field_names::\n\nhttps://linter.aip.dev/naming_formats/field_names
::error file=example2.proto,title=core։։0131։։request_message։։name::\n\nhttps://linter.aip.dev/131/request_message/name
::error file=example2.proto,title=core։։0132։։response_message։։name::\n\nhttps://linter.aip.dev/132/response_message/name
::error file=example3.proto,title=core։։naming_formats։։field_names::\n\nhttps://linter.aip.dev/naming_formats/field_names
::error file=example4.proto,title=core։։naming_formats։։field_names::\n\nhttps://linter.aip.dev/naming_formats/field_names
::error file=example4.proto,title=core։։0132։։response_message։։name::\n\nhttps://linter.aip.dev/132/response_message/name
want: `::error file=example.proto,title=core։։naming_formats։։field_names::%0A%0Ahttps://linter.aip.dev/naming_formats/field_names
::error file=example.proto,title=core։։naming_formats։։field_names::%0A%0Ahttps://linter.aip.dev/naming_formats/field_names
::error file=example2.proto,title=core։։0131։։request_message։։name::%0A%0Ahttps://linter.aip.dev/131/request_message/name
::error file=example2.proto,title=core։։0132։։response_message։։name::%0A%0Ahttps://linter.aip.dev/132/response_message/name
::error file=example3.proto,title=core։։naming_formats։։field_names::%0A%0Ahttps://linter.aip.dev/naming_formats/field_names
::error file=example4.proto,title=core։։naming_formats։։field_names::%0A%0Ahttps://linter.aip.dev/naming_formats/field_names
::error file=example4.proto,title=core։։0132։։response_message։։name::%0A%0Ahttps://linter.aip.dev/132/response_message/name
`,
},
}
Expand Down
42 changes: 21 additions & 21 deletions lint/problem.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -76,36 +76,36 @@ func (p Problem) MarshalYAML() (interface{}, error) {

// Marshal defines how to represent a serialized Problem.
func (p Problem) marshal() interface{} {
var fl fileLocation
var fl FileLocation
if p.Location != nil {
// If Location is set, use it.
fl = fileLocationFromPBLocation(p.Location, p.Descriptor)
fl = FileLocationFromPBLocation(p.Location, p.Descriptor)
} else if p.Descriptor != nil {
// Otherwise, use the descriptor's location.
// This is the protobuf-go idiomatic way to get the source location.
// Note: ParentFile() called on a FileDescriptor returns itself.
loc := p.Descriptor.ParentFile().SourceLocations().ByDescriptor(p.Descriptor)
fl = fileLocation{
fl = FileLocation{
Path: p.Descriptor.ParentFile().Path(),
Start: position{
Start: Position{
Line: loc.StartLine + 1,
Column: loc.StartColumn + 1,
},
End: position{
End: Position{
Line: loc.EndLine + 1,
Column: loc.EndColumn,
},
}
} else {
// Default location if no descriptor.
fl = fileLocationFromPBLocation(nil, nil)
fl = FileLocationFromPBLocation(nil, nil)
}

// Return a marshal-able structure.
return struct {
Message string `json:"message" yaml:"message"`
Suggestion string `json:"suggestion,omitempty" yaml:"suggestion,omitempty"`
Location fileLocation `json:"location" yaml:"location"`
Location FileLocation `json:"location" yaml:"location"`
RuleID RuleName `json:"rule_id" yaml:"rule_id"`
RuleDocURI string `json:"rule_doc_uri" yaml:"rule_doc_uri"`
Category string `json:"category,omitempty" yaml:"category,omitempty"`
Expand All @@ -124,35 +124,35 @@ func (p Problem) GetRuleURI() string {
return getRuleURL(string(p.RuleID), ruleURLMappings)
}

// position describes a one-based position in a source code file.
// Position describes a one-based position in a source code file.
// They are one-indexed, as a human counts lines or columns.
type position struct {
type Position struct {
Line int `json:"line_number" yaml:"line_number"`
Column int `json:"column_number" yaml:"column_number"`
}

// fileLocation describes a location in a source code file.
// FileLocation describes a location in a source code file.
//
// Note: Positions are one-indexed, as a human counts lines or columns
// in a file.
type fileLocation struct {
Start position `json:"start_position" yaml:"start_position"`
End position `json:"end_position" yaml:"end_position"`
type FileLocation struct {
Start Position `json:"start_position" yaml:"start_position"`
End Position `json:"end_position" yaml:"end_position"`
Path string `json:"path" yaml:"path"`
}

// fileLocationFromPBLocation returns a new fileLocation object based on a
// FileLocationFromPBLocation returns a new FileLocation object based on a
// protocol buffer SourceCodeInfo_Location
func fileLocationFromPBLocation(l *dpb.SourceCodeInfo_Location, d protoreflect.Descriptor) fileLocation {
func FileLocationFromPBLocation(l *dpb.SourceCodeInfo_Location, d protoreflect.Descriptor) FileLocation {
// Spans are guaranteed by protobuf to have either three or four ints.
span := []int32{0, 0, 1}
if l != nil {
span = l.Span
}

var fl fileLocation
var fl FileLocation
if d != nil {
fl = fileLocation{Path: d.ParentFile().Path()}
fl = FileLocation{Path: d.ParentFile().Path()}
}

// If `span` has four ints; they correspond to
Expand All @@ -161,11 +161,11 @@ func fileLocationFromPBLocation(l *dpb.SourceCodeInfo_Location, d protoreflect.D
// We add one because spans are zero-indexed, but not to the end column
// because we want the ending position to be inclusive and not exclusive.
if len(span) == 4 {
fl.Start = position{
fl.Start = Position{
Line: int(span[0]) + 1,
Column: int(span[1]) + 1,
}
fl.End = position{
fl.End = Position{
Line: int(span[2]) + 1,
Column: int(span[3]),
}
Expand All @@ -177,11 +177,11 @@ func fileLocationFromPBLocation(l *dpb.SourceCodeInfo_Location, d protoreflect.D
//
// We add one because spans are zero-indexed, but not to the end column
// because we want the ending position to be inclusive and not exclusive.
fl.Start = position{
fl.Start = Position{
Line: int(span[0]) + 1,
Column: int(span[1]) + 1,
}
fl.End = position{
fl.End = Position{
Line: int(span[0]) + 1,
Column: int(span[2]),
}
Expand Down