internal/analyze/footguns.go

@@ -35,6 +35,7 @@
 package analyze
 
 import (
+	"os"
 	"regexp"
 	"sort"
 	"strings"
@@ -151,13 +152,41 @@ func findUnescapedVars(cmd string, matrixVars map[string]bool) []struct {
 	Line    int
 	Var     string
 	Context string
+} {
+	return scanLinesForUnescapedVars(strings.Split(cmd, "\n"), matrixVars)
+}
+
+// findUnescapedVarsInFile scans the FULL raw pipeline YAML text - not just
+// parsed `commands:` strings - for the same `${VAR}` footgun. This closes a
+// false-negative gap: Woodpecker's `${VAR}` substitution pass runs over the
+// entire raw pipeline file text before parsing, including step names,
+// top-level/inline comments and `environment:` values that live outside any
+// commands string, so a file can pass findUnescapedVars clean and still get
+// rejected by Woodpecker at compile time (oleks/pipetree#13 - a real repro:
+// a `#` comment ABOVE a step's commands: block containing literal `${VAR}`
+// example text compiled fine per the old command-only scan, but Woodpecker
+// itself hard-failed with "unable to parse variable name" on push).
+func findUnescapedVarsInFile(rawText string, matrixVars map[string]bool) []struct {
+	Line    int
+	Var     string
+	Context string
+} {
+	return scanLinesForUnescapedVars(strings.Split(rawText, "\n"), matrixVars)
+}
+
+// scanLinesForUnescapedVars is the line-level detector shared by the
+// per-command scan (findUnescapedVars) and the whole-file scan
+// (findUnescapedVarsInFile) - only the text they're handed differs.
+func scanLinesForUnescapedVars(lines []string, matrixVars map[string]bool) []struct {
+	Line    int
+	Var     string
+	Context string
 } {
 	var hits []struct {
 		Line    int
 		Var     string
 		Context string
 	}
-	lines := strings.Split(cmd, "\n")
 	for i, line := range lines {
 		trimmed := strings.TrimSpace(line)
 		for _, loc := range dollarBraceRe.FindAllStringSubmatchIndex(line, -1) {
@@ -190,6 +219,12 @@ func runFootguns(proj *model.Project, apos *[]StrayApostrophe, vars *[]Unescaped
 				matrixVars[axis] = true
 			}
 		}
+		// seen dedups the whole-file pass below against hits the
+		// per-command pass already attributed to a step, keyed on the
+		// (trimmed) source line and var name - both passes see the same
+		// physical line for anything inside a commands: string, and
+		// without this a hit there would be reported twice.
+		seen := make(map[string]bool)
 		for _, s := range pl.Steps {
 			for _, cmd := range s.Commands {
 				for _, h := range findStrayApostrophes(cmd) {
@@ -203,9 +238,23 @@ func runFootguns(proj *model.Project, apos *[]StrayApostrophe, vars *[]Unescaped
 						Project: proj.Name, Pipeline: pl.Name, Step: s.Name,
 						Line: h.Line, Var: h.Var, Context: h.Context,
 					})
+					seen[h.Var+"\x00"+h.Context] = true
 				}
 			}
 		}
+		if raw, err := os.ReadFile(pl.File); err == nil {
+			for _, h := range findUnescapedVarsInFile(string(raw), matrixVars) {
+				key := h.Var + "\x00" + h.Context
+				if seen[key] {
+					continue
+				}
+				seen[key] = true
+				*vars = append(*vars, UnescapedWoodpeckerVar{
+					Project: proj.Name, Pipeline: pl.Name,
+					Line: h.Line, Var: h.Var, Context: h.Context,
+				})
+			}
+		}
 	}
 }
 

internal/analyze/footguns_test.go

@@ -115,6 +115,30 @@ func TestFootguns_NegativeCase_MatrixVar(t *testing.T) {
 	}
 }
 
+// TestUnescapedVar_RawFilePass_PositiveCase reproduces oleks/pipetree#13: a
+// literal, unescaped ${VAR} in a YAML comment ABOVE a step's commands:
+// block. The per-command scan alone would miss it (the comment line is
+// never part of any parsed Commands string), but Woodpecker's own
+// substitution pass runs over the whole raw file, so pipetree must catch it
+// too via the whole-file pass (findUnescapedVarsInFile).
+func TestUnescapedVar_RawFilePass_PositiveCase(t *testing.T) {
+	proj := loadProject(t, "deals", "testdata/deals-comment-above-commands.yaml")
+	report := Run([]*model.Project{proj})
+
+	found := false
+	for _, u := range report.UnescapedVars {
+		if u.Var == "VAR" {
+			found = true
+			if u.Step != "" {
+				t.Errorf("expected no step attribution for a comment-line hit outside any commands: block, got Step=%q", u.Step)
+			}
+		}
+	}
+	if !found {
+		t.Errorf("expected the whole-file pass to flag the unescaped ${VAR} in the comment above the step, got: %+v", report.UnescapedVars)
+	}
+}
+
 // TestFindStrayApostrophes_Unit exercises the line-level detector directly
 // against small synthetic commands, independent of YAML parsing.
 func TestFindStrayApostrophes_Unit(t *testing.T) {

internal/analyze/testdata/deals-comment-above-commands.yaml

@@ -0,0 +1,18 @@
+# Repro for oleks/pipetree#13: an explanatory comment ABOVE a step's
+# commands: block containing literal, unescaped ${VAR} example text (the
+# genuine mistake made while documenting this exact footgun on
+# oleks/deals). The old command-only scan (findUnescapedVars) never saw
+# this line because a comment line isn't part of any parsed Commands
+# string - but Woodpecker's own ${VAR} substitution pass runs over the
+# ENTIRE raw pipeline file, so it hard-failed at compile time ("unable to
+# parse variable name") on oleks/deals pipelines #12/#13 and
+# oleks/element-web-patched #5/#6. Below: this comment escapes it as
+# ${VAR} unescaped on purpose, to reproduce the false negative.
+when:
+  - event: [push, pull_request]
+
+steps:
+  - name: build
+    image: git.oleks.space/oleks/nix-ci:latest
+    commands:
+      - echo "no footgun down here"

internal/render/analyze.go

@@ -32,8 +32,12 @@ func Analyze(w io.Writer, report analyze.Report) {
 		_, _ = fmt.Fprintf(w, "  %s / %s / step %s, line %d: %s\n", a.Project, a.Pipeline, a.Step, a.Line, a.Context)
 	}
 
-	_, _ = fmt.Fprintf(w, "\n%d unescaped ${VAR} in step commands (Woodpecker substitutes these at parse time, before the shell runs - escape as $${VAR} to hand it to the shell):\n", len(report.UnescapedVars))
+	_, _ = fmt.Fprintf(w, "\n%d unescaped ${VAR} in the pipeline file (Woodpecker substitutes these at parse time, before the shell runs - escape as $${VAR} to hand it to the shell):\n", len(report.UnescapedVars))
 	for _, u := range report.UnescapedVars {
-		_, _ = fmt.Fprintf(w, "  %s / %s / step %s, line %d: ${%s} in %s\n", u.Project, u.Pipeline, u.Step, u.Line, u.Var, u.Context)
+		if u.Step != "" {
+			_, _ = fmt.Fprintf(w, "  %s / %s / step %s, line %d: ${%s} in %s\n", u.Project, u.Pipeline, u.Step, u.Line, u.Var, u.Context)
+		} else {
+			_, _ = fmt.Fprintf(w, "  %s / %s, line %d (outside any step's commands): ${%s} in %s\n", u.Project, u.Pipeline, u.Line, u.Var, u.Context)
+		}
 	}
 }