chore(linter): traverse "runArgs" as the "docker run" argv it is

dockerargs/dockerargs.go

@@ -2,8 +2,8 @@
 // devcontainer tooling, gives meaning to:
 //
 //   - "runArgs", which becomes the argv of the "docker run" command the tooling builds. [Parse] is
-//     the single place that knows where a flag's value can be written, so a rule only has to know
-//     the values it cares about and never which entry of the array holds one.
+//     the single place that knows where a flag's value can be written, so its callers only have to
+//     know the values they care about and never which entry of the array holds one.
 //   - the values themselves, whose syntax is Docker's wherever they are written: a "securityOpt"
 //     entry ([ParseSecurityOpt]), a capability name ([Capability]), a boolean ([IsTrue]).
 package dockerargs

linter/linter.go

@@ -69,7 +69,7 @@ func (l *Linter) LintDocument(path string, fileType FileType, doc *Document, dir
 	rctx := &Context{Path: path, Type: fileType, Root: doc.tree, Dir: dir}
 	var issues []Issue
 	seen := map[Issue]struct{}{}
-	walk(doc.tree, "", nil, patterns, func(r *Rule, node *Node) {
+	walk(doc.tree, fileType, patterns, func(r *Rule, node *Node) {
 		id := r.ID
 		severity := l.severities[id]
 		for _, f := range safeCheck(r, rctx, node) {

linter/linter_test.go

@@ -429,3 +429,50 @@ func TestLintDocument_RulePanicIsRecovered(t *testing.T) {
 		t.Errorf("Severity = %v, want %v", issues[0].Severity, SeverityError)
 	}
 }
+
+// runArgsSpyRule is a stub Rule that reports every element of "runArgs" it is handed, naming how it
+// was reached. It declares every file type deliberately: a rule that declares only some leaves
+// LintDocument with no patterns for the rest, which it short-circuits before traversing anything, so
+// a test written on such a rule would pass whatever the traversal does with the file types it skips.
+var runArgsSpyRule = &Rule{
+	ID:          "run-args-spy",
+	Description: "reports how each element of runArgs was reached",
+	FileTypes:   []FileType{Devcontainer, Feature, Template},
+	Paths:       []string{"/runArgs/*"},
+	Check: func(_ *Context, node *Node) []Finding {
+		if node.Arg == nil {
+			return []Finding{{Message: "element " + node.Pointer, Offset: node.Value.StartOffset}}
+		}
+		return []Finding{{Message: "flag --" + node.Arg.Flag, Offset: node.Value.StartOffset}}
+	},
+}
+
+// TestLintDocument_RunArgsFileTypes checks that "runArgs" is read as a "docker run" argv only in a
+// devcontainer.json. It is not a property of a Feature or a Template, so there the array is an
+// ordinary one, walked by index.
+func TestLintDocument_RunArgsFileTypes(t *testing.T) {
+	t.Parallel()
+
+	tests := []struct {
+		fileType FileType
+		want     []string
+	}{
+		{Devcontainer, []string{"flag --cap-add"}},
+		{Feature, []string{"element /runArgs/0"}},
+		{Template, []string{"element /runArgs/0"}},
+	}
+	for _, tt := range tests {
+		t.Run(string(tt.fileType), func(t *testing.T) {
+			t.Parallel()
+			l := New()
+			l.RegisterRule(runArgsSpyRule, SeverityWarn)
+			var got []string
+			for _, issue := range lintSource(t, l, "config.json", tt.fileType, `{"runArgs": ["--cap-add=ALL"]}`) {
+				got = append(got, issue.Message)
+			}
+			if !slices.Equal(got, tt.want) {
+				t.Errorf("messages = %v, want %v", got, tt.want)
+			}
+		})
+	}
+}

linter/rule.go

@@ -6,6 +6,7 @@ import (
 	"io/fs"
 	"strings"
 
+	"github.com/bare-devcontainer/decolint/dockerargs"
 	"github.com/tailscale/hujson"
 )
 
@@ -238,6 +239,10 @@ type Node struct {
 	Pointer string
 	// Value is the HuJSON value at Pointer.
 	Value *hujson.Value
+	// Arg is the "docker run" flag occurrence the value was reached as, set only on a node the
+	// "runArgs" traversal produced (see [Rule.Paths]) and nil on every other node. Value is then the
+	// element the flag's value is written in, which is the flag's own element or the one after it.
+	Arg *dockerargs.Arg
 }
 
 // Finding is a single problem reported by a rule.
@@ -280,13 +285,20 @@ type Rule struct {
 	// Paths are the JSON Pointer patterns of the values this rule wants to inspect. A "*" segment
 	// matches any object member name or array index (e.g. "/mounts/*"); the empty string matches the
 	// document root.
+	//
+	// A devcontainer.json's "runArgs" is traversed as the "docker run" argv it becomes, so its
+	// elements are addressed by flag rather than by index: "/runArgs/--volume" matches once per
+	// occurrence of that flag, whichever spelling the argv uses, and [Node.Arg] carries the value the
+	// occurrence gives it. A rule reporting a flag's absence cannot be driven by that, since a flag
+	// that is not there is never matched; it inspects the document root instead.
 	Paths []string
 	// Example shows the rule firing and not firing on realistic configuration. Tests lint both: Bad
 	// must report the rule, Good must not.
 	Example Example
-	// Check inspects one value matching Paths and returns any findings. It is called at most once per
-	// rule for a given value, even if several patterns match it. Check must be safe for concurrent
-	// use, since it may be called for multiple files.
+	// Check inspects one value matching Paths and returns any findings. It is called once for a given
+	// value, even if several patterns match it — except for a "runArgs" element naming several flags,
+	// which it is called for once per flag, with [Node.Arg] telling the occurrences apart. Check must
+	// be safe for concurrent use, since it may be called for multiple files.
 	Check func(ctx *Context, node *Node) []Finding
 }
 

linter/walk.go

@@ -5,6 +5,7 @@ import (
 	"strconv"
 	"strings"
 
+	"github.com/bare-devcontainer/decolint/dockerargs"
 	"github.com/tailscale/hujson"
 )
 
@@ -56,29 +57,34 @@ func matches(pat, segs []string) bool {
 	return true
 }
 
-// walk traverses the syntax tree depth-first exactly once and calls visit for every (rule, value)
-// pair where one of the rule's patterns matches the value's path. A rule is visited at most once
-// per value. pointer and segs describe the location of v; they must be "" and nil for the document
-// root.
-func walk(v *hujson.Value, pointer string, segs []string, patterns []pattern, visit func(*Rule, *Node)) {
-	node := &Node{Pointer: pointer, Value: v}
-	var called []*Rule
-	for _, p := range patterns {
-		if !matches(p.segments, segs) {
-			continue
-		}
-		if slices.Contains(called, p.rule) {
-			continue
-		}
-		called = append(called, p.rule)
-		visit(p.rule, node)
-	}
+// walk traverses the syntax tree of a file of the given type depth-first exactly once and calls
+// visit for every (rule, value) pair where one of the rule's patterns matches the value's path. A
+// rule is visited once for a value however many of its patterns match, except for a "runArgs"
+// element naming several flags, which is visited once per flag (see runArgsFlags).
+func walk(root *hujson.Value, fileType FileType, patterns []pattern, visit func(*Rule, *Node)) {
+	w := walker{patterns: patterns, runArgs: fileType == Devcontainer, visit: visit}
+	w.value(root, "", nil)
+}
+
+// walker carries the state of one traversal.
+type walker struct {
+	patterns []pattern
+	// runArgs reports whether the document's "runArgs" is traversed as an argv (see runArgsFlags).
+	// Only a devcontainer.json has one: it is not a property of a Feature or a Template.
+	runArgs bool
+	visit   func(*Rule, *Node)
+}
+
+// value visits v and descends into it. pointer and segs describe the location of v; they must be ""
+// and nil for the document root.
+func (w *walker) value(v *hujson.Value, pointer string, segs []string) {
+	w.dispatch(&Node{Pointer: pointer, Value: v}, segs)
 
-	// append(segs, seg) below may share segs's backing array across sibling calls, so a later
-	// sibling can overwrite an element a previous sibling appended. This is safe only because
-	// traversal is sequential and no walk call retains segs past its own return (matches reads it
-	// synchronously via the visit callback). Parallelizing this traversal or having Node retain segs
-	// would require copying it first.
+	// append(segs, seg) here and in runArgsFlags may share segs's backing array across sibling calls,
+	// so a later sibling can overwrite an element a previous sibling appended. This is safe only
+	// because traversal is sequential and no walk call retains segs past its own return (matches
+	// reads it synchronously via the visit callback). Parallelizing this traversal or having Node
+	// retain segs would require copying it first.
 	switch t := v.Value.(type) {
 	case *hujson.Object:
 		for i := range t.Members {
@@ -88,12 +94,71 @@ func walk(v *hujson.Value, pointer string, segs []string, patterns []pattern, vi
 				continue
 			}
 			seg := name.String()
-			walk(&m.Value, pointer+"/"+escapeSegment(seg), append(segs, seg), patterns, visit)
+			w.value(&m.Value, pointer+"/"+escapeSegment(seg), append(segs, seg))
 		}
 	case *hujson.Array:
+		if w.runArgs && len(segs) == 1 && segs[0] == "runArgs" {
+			w.runArgsFlags(t, pointer, segs)
+			return
+		}
 		for i := range t.Elements {
 			seg := strconv.Itoa(i)
-			walk(&t.Elements[i], pointer+"/"+seg, append(segs, seg), patterns, visit)
+			w.value(&t.Elements[i], pointer+"/"+seg, append(segs, seg))
 		}
 	}
 }
+
+// RunArgs returns every "docker run" flag occurrence in arr, a devcontainer.json's "runArgs", as
+// [dockerargs.Parse] reads the argv the array becomes; [dockerargs.Arg.Index] indexes arr.Elements.
+// An element that is not a string, which the devcontainer tooling could not hand to docker at all,
+// stands in as an empty entry so that the elements around it keep the positions docker would read
+// them at.
+//
+// It is the reading the traversal hands "/runArgs/--flag" patterns (see [Rule.Paths]), for the rules
+// that cannot be driven by it because they report a flag's absence.
+func RunArgs(arr *hujson.Array) []dockerargs.Arg {
+	argv := make([]string, len(arr.Elements))
+	for i, elem := range arr.Elements {
+		if lit, ok := elem.Value.(hujson.Literal); ok && lit.Kind() == '"' {
+			argv[i] = lit.String()
+		}
+	}
+	return dockerargs.Parse(argv)
+}
+
+// runArgsFlags visits the elements of arr, a devcontainer.json's "runArgs", as the "docker run" argv
+// the array becomes: each flag occurrence is reached at the flag's long spelling, so "-v" and
+// "--volume" alike are reached at "/runArgs/--volume", on the element the flag's value is written
+// in.
+//
+// One element can hold several occurrences — a run of shorthands, "-it", names two flags — and is
+// visited once for each, so that a rule subscribing to more than one of them, or to "/runArgs/*",
+// sees every flag the argv gives rather than whichever comes first. Collapsing them would drop a
+// flag silently, which is the failure this addressing exists to prevent. The elements are
+// deliberately not visited by index as well: that reaches the same occurrence by a second path and
+// buys nothing, where two occurrences in one element are two distinct things to report on.
+func (w *walker) runArgsFlags(arr *hujson.Array, pointer string, segs []string) {
+	for _, arg := range RunArgs(arr) {
+		node := &Node{
+			Pointer: pointer + "/" + strconv.Itoa(arg.Index),
+			Value:   &arr.Elements[arg.Index],
+			Arg:     &arg,
+		}
+		w.dispatch(node, append(segs, "--"+arg.Flag))
+	}
+}
+
+// dispatch calls visit for every rule with a pattern matching segs, at most once per rule.
+func (w *walker) dispatch(node *Node, segs []string) {
+	var called []*Rule
+	for _, p := range w.patterns {
+		if !matches(p.segments, segs) {
+			continue
+		}
+		if slices.Contains(called, p.rule) {
+			continue
+		}
+		called = append(called, p.rule)
+		w.visit(p.rule, node)
+	}
+}

linter/walk_test.go

@@ -4,6 +4,8 @@ import (
 	"slices"
 	"testing"
 
+	"github.com/bare-devcontainer/decolint/dockerargs"
+	"github.com/google/go-cmp/cmp"
 	"github.com/tailscale/hujson"
 )
 
@@ -68,7 +70,7 @@ func TestWalk_Dispatch(t *testing.T) {
 			var calls []string
 			root := parseValue(t, src)
 			patterns := compilePatterns(pathSpy("spy", tt.paths))
-			walk(&root, "", nil, patterns, func(_ *Rule, node *Node) {
+			walk(&root, Devcontainer, patterns, func(_ *Rule, node *Node) {
 				calls = append(calls, node.Pointer)
 			})
 			if !slices.Equal(calls, tt.want) {
@@ -90,7 +92,7 @@ func TestWalk_SingleTraversal(t *testing.T) {
 		compilePatterns(pathSpy("b", []string{"/image"}))...,
 	)
 	var callsA, callsB []string
-	walk(&root, "", nil, patterns, func(r *Rule, node *Node) {
+	walk(&root, Devcontainer, patterns, func(r *Rule, node *Node) {
 		switch r.ID {
 		case "a":
 			callsA = append(callsA, node.Pointer)
@@ -103,6 +105,116 @@ func TestWalk_SingleTraversal(t *testing.T) {
 	}
 }
 
+func TestRunArgs(t *testing.T) {
+	t.Parallel()
+
+	tests := []struct {
+		name string
+		src  string
+		want []dockerargs.Arg
+	}{
+		{"empty array", `[]`, nil},
+		{"flag and value in one element", `["--cap-drop=ALL"]`, []dockerargs.Arg{
+			{Flag: "cap-drop", Value: "ALL", Index: 0},
+		}},
+		{"value in the following element", `["--cap-drop", "ALL"]`, []dockerargs.Arg{
+			{Flag: "cap-drop", Value: "ALL", Index: 1},
+		}},
+		// A non-string element keeps its position so that the ones after it keep theirs.
+		{"non-string element", `[123, "--privileged"]`, []dockerargs.Arg{
+			{Flag: "privileged", Value: "true", Index: 1},
+		}},
+		{"non-string element consumed as a value", `["--label", 123, "--privileged"]`, []dockerargs.Arg{
+			{Flag: "label", Value: "", Index: 1},
+			{Flag: "privileged", Value: "true", Index: 2},
+		}},
+	}
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			t.Parallel()
+			v := parseValue(t, tt.src)
+			arr, ok := v.Value.(*hujson.Array)
+			if !ok {
+				t.Fatalf("%s is not an array", tt.src)
+			}
+			if diff := cmp.Diff(tt.want, RunArgs(arr)); diff != "" {
+				t.Errorf("RunArgs(%s) mismatch (-want +got):\n%s", tt.src, diff)
+			}
+		})
+	}
+}
+
+// TestWalk_RunArgs checks the traversal of a devcontainer.json's "runArgs" as the "docker run" argv
+// it becomes: its elements are reached by flag rather than by index, and each of them at most once.
+func TestWalk_RunArgs(t *testing.T) {
+	t.Parallel()
+
+	// visit is one (rule, value) pair walk produced: where the value is, its source text, and the
+	// flag occurrence it was reached as, if any.
+	type visit struct {
+		pointer string
+		element string // the visited value, as written in the source
+		flag    string
+		value   string
+	}
+	tests := []struct {
+		name  string
+		paths []string
+		src   string
+		want  []visit
+	}{
+		{"long flag holding its value", []string{"/runArgs/--cap-add"}, `{"runArgs": ["--cap-add=ALL"]}`,
+			[]visit{{"/runArgs/0", `"--cap-add=ALL"`, "cap-add", "ALL"}}},
+		{"long flag consuming the next element", []string{"/runArgs/--cap-add"}, `{"runArgs": ["--cap-add", "ALL"]}`,
+			[]visit{{"/runArgs/1", `"ALL"`, "cap-add", "ALL"}}},
+		{"shorthand reaches the long spelling", []string{"/runArgs/--volume"}, `{"runArgs": ["-v", "/a:/b"]}`,
+			[]visit{{"/runArgs/1", `"/a:/b"`, "volume", "/a:/b"}}},
+		{"one element naming several flags", []string{"/runArgs/--interactive", "/runArgs/--tty"}, `{"runArgs": ["-it"]}`,
+			[]visit{{"/runArgs/0", `"-it"`, "interactive", "true"}, {"/runArgs/0", `"-it"`, "tty", "true"}}},
+		{"every occurrence of a flag", []string{"/runArgs/--cap-add"}, `{"runArgs": ["--cap-add=ALL", "--cap-add=NET_ADMIN"]}`,
+			[]visit{{"/runArgs/0", `"--cap-add=ALL"`, "cap-add", "ALL"}, {"/runArgs/1", `"--cap-add=NET_ADMIN"`, "cap-add", "NET_ADMIN"}}},
+		{"another flag's value names no flag", []string{"/runArgs/--cap-add"}, `{"runArgs": ["--label", "--cap-add=ALL"]}`, nil},
+		{"non-string element", []string{"/runArgs/--cap-add"}, `{"runArgs": [123, "--cap-add=ALL"]}`,
+			[]visit{{"/runArgs/1", `"--cap-add=ALL"`, "cap-add", "ALL"}}},
+		{"every copy of a duplicated member", []string{"/runArgs/--cap-add"},
+			`{"runArgs": ["--cap-add=ALL"], "runArgs": ["--cap-add=NET_ADMIN"]}`,
+			[]visit{{"/runArgs/0", `"--cap-add=ALL"`, "cap-add", "ALL"}, {"/runArgs/0", `"--cap-add=NET_ADMIN"`, "cap-add", "NET_ADMIN"}}},
+
+		// The elements are addressed by flag only, so a wildcard reaches an element once per flag it
+		// names — and only the elements a flag's value is written in.
+		{"wildcard over flags holding their values", []string{"/runArgs/*"}, `{"runArgs": ["--privileged", "--init"]}`,
+			[]visit{{"/runArgs/0", `"--privileged"`, "privileged", "true"}, {"/runArgs/1", `"--init"`, "init", "true"}}},
+		{"wildcard over a flag consuming the next element", []string{"/runArgs/*"}, `{"runArgs": ["--cap-add", "ALL"]}`,
+			[]visit{{"/runArgs/1", `"ALL"`, "cap-add", "ALL"}}},
+		{"wildcard over one element naming several flags", []string{"/runArgs/*"}, `{"runArgs": ["-it"]}`,
+			[]visit{{"/runArgs/0", `"-it"`, "interactive", "true"}, {"/runArgs/0", `"-it"`, "tty", "true"}}},
+
+		{"the array itself", []string{"/runArgs"}, `{"runArgs": ["--cap-add=ALL"]}`,
+			[]visit{{"/runArgs", `["--cap-add=ALL"]`, "", ""}}},
+		{"a runArgs that is not an array", []string{"/runArgs/--cap-add"}, `{"runArgs": "--cap-add=ALL"}`, nil},
+		{"a runArgs that is not the document's", []string{"/build/runArgs/*"}, `{"build": {"runArgs": ["--cap-add=ALL"]}}`,
+			[]visit{{"/build/runArgs/0", `"--cap-add=ALL"`, "", ""}}},
+	}
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			t.Parallel()
+			var got []visit
+			root := parseValue(t, tt.src)
+			patterns := compilePatterns(pathSpy("spy", tt.paths))
+			walk(&root, Devcontainer, patterns, func(_ *Rule, node *Node) {
+				v := visit{pointer: node.Pointer, element: tt.src[node.Value.StartOffset:node.Value.EndOffset]}
+				if node.Arg != nil {
+					v.flag, v.value = node.Arg.Flag, node.Arg.Value
+				}
+				got = append(got, v)
+			})
+			if !slices.Equal(got, tt.want) {
+				t.Errorf("visited %v, want %v", got, tt.want)
+			}
+		})
+	}
+}
+
 func TestSplitPointer(t *testing.T) {
 	t.Parallel()