diff --git a/.github/workflows/cgo.yml b/.github/workflows/cgo.yml index 939d6e4789c..747f6afebe7 100644 --- a/.github/workflows/cgo.yml +++ b/.github/workflows/cgo.yml @@ -1554,6 +1554,9 @@ jobs: - name: Run custom linters (wasm) run: GOOS=js GOARCH=wasm make golint-custom LINTER_FLAGS="-errstringmatch -panicinlibrarycode -manualmutexunlock -osexitinlibrary -rawloginlib -logfatallibrary -regexpcompileinfunction -fprintlnsprintf -strconvparseignorederror -jsonmarshalignoredeerror -uncheckedtypeassertion -fmterrorfnoverbs -tolowerequalfold -httpnoctx -httprespbodyclose -httpstatuscode -timeafterleak -errortypeassertion -execcommandwithoutcontext -sprintfint -stringsindexcontains -stringscountcontains -bytesbufferstring -ioutildeprecated -mapclearloop -mapdeletecheck -sprintfbool -appendoneelement -timenowsub -stringsjoinone -writebytestring -lenstringsplit -stringreplaceminusone -osgetenvlibrary -ossetenvlibrary -stringsindexhasprefix -ctxbackground -wgdonenotdeferred -goroutinemissingrecover -trimleftright -walkfuncerrshadow -uncheckedflushreturn -bytescomparestring -nilctxpassed -stringbytesroundtrip -fileclosenotdeferred -timesleepnocontext -sprintferrorsnew -globwalkignorederror -appendbytestring -slicemakezerolength -sortslice -deferinloop -regexpdynamicpattern -generatedyamlheredoc -test=false" LINTER_PACKAGES="./pkg/console ./pkg/parser ./pkg/styles ./pkg/tty ./pkg/workflow" + - name: Check console stderr formatting (wasm) + run: GOOS=js GOARCH=wasm make golint-custom LINTER_FLAGS="-consolestderr -test=false" LINTER_PACKAGES="./pkg/console ./pkg/parser ./pkg/styles ./pkg/tty ./pkg/workflow" + # Ensure no action shell scripts invoke python or python3 - name: Lint action shell scripts run: make lint-action-sh diff --git a/docs/adr/67634-enforce-native-wasm-linter-parity-in-ci.md b/docs/adr/67634-enforce-native-wasm-linter-parity-in-ci.md new file mode 100644 index 00000000000..9c5d860a262 --- /dev/null +++ b/docs/adr/67634-enforce-native-wasm-linter-parity-in-ci.md @@ -0,0 +1,54 @@ +# ADR-67634: Enforce Native/WebAssembly Custom-Linter Parity via YAML-Parsing CI Sync Tests + +**Date**: 2026-10-11 +**Status**: Draft +**Deciders**: pelikhan (PR #67634 author), gh-aw maintainers + +--- + +### Context + +The custom analyzers in `pkg/linters` are enforced in CI through `LINTER_FLAGS` lists in `.github/workflows/cgo.yml`, which runs the linters twice: once natively and once with `GOOS=js GOARCH=wasm`. Some analyzers (notably `consolestderr`) were enforced only on the native target, so WebAssembly-only regressions could land undetected (see #67609). The previous guard in `pkg/linters` relied on a hard-coded expected step count, which was brittle: it broke on unrelated workflow edits while still failing to detect analyzer-set drift between the two targets. A known, deliberate exception exists — `contextcancelnotdeferred` is native-only pending the work tracked in #65753 — so the guard must tolerate an explicitly enumerated exception set rather than demanding exact equality. + +### Decision + +We will enforce native/WebAssembly analyzer parity by parsing the custom-linter job in `.github/workflows/cgo.yml` and comparing the *combined* analyzer sets (bulk step plus dedicated single-analyzer steps) per target, instead of asserting on a hard-coded step count. The parity test tracks native-only exceptions in an explicit allow-list keyed to tracking issues, and fails both when a target is missing an enforced analyzer and when a stale exception remains after parity is restored. As part of this decision, the `consolestderr -test=false` step is mirrored onto the wasm target using the existing wasm package scope. + +### Alternatives Considered + +#### Alternative 1: Keep the hard-coded step-count guard and just add the missing wasm step + +Minimal diff: add the `consolestderr (wasm)` step and bump the expected step count. This was rejected because it fixes only the current instance of the bug class — the next analyzer added to one target but not the other would still slip through — and the step count keeps producing false failures on unrelated workflow edits. + +#### Alternative 2: Generate the CI linter steps from the analyzer registry + +Rather than testing the workflow, derive the `LINTER_FLAGS` lists (and therefore both target steps) from `pkg/linters`' registry at compile/generation time, making drift structurally impossible. This is the stronger guarantee, but it requires a generator plus workflow regeneration plumbing and does not accommodate intentional per-target differences (such as the #65753 exception) without additional metadata. It remains a reasonable future step; parity testing was chosen as the lower-risk increment. + +#### Alternative 3: Collapse to a single linter invocation covering both targets + +Run the analyzers once over a union package set and drop per-target steps entirely. Rejected because `GOOS=js GOARCH=wasm` build constraints change which files are analyzed, so a single invocation would lose genuine wasm-specific coverage. + +### Consequences + +#### Positive + +- Native-only or wasm-only enforcement gaps now fail fast in `go test ./pkg/linters` instead of reaching `main`. +- `consolestderr` violations are caught in WebAssembly-constrained code paths, closing #67609. +- Removing the hard-coded step count eliminates a recurring source of unrelated CI failures when `cgo.yml` is edited. +- Deliberate exceptions are documented in code and tied to tracking issues, and stale exceptions are rejected automatically once parity is restored. + +#### Negative + +- The test parses CI YAML with string/structure matching, so it is coupled to the current step formatting (bulk flag layout, quoting, `GOOS=js GOARCH=wasm` prefix); reformatting the workflow can require test updates. +- The native-only exception list is a manually maintained allow-list that must be kept honest by reviewers. +- Analyzer flags are still duplicated between the native and wasm steps, so additions require touching two places — the test only detects the mistake rather than preventing it. + +#### Neutral + +- CI runtime grows slightly with the extra dedicated wasm step. +- `pkg/linters/README.md` becomes the canonical description of which analyzers are enforced per target and why exceptions exist. +- Regression coverage added in `pkg/linters/ci_sync_test.go` and `pkg/linters/doc_sync_test.go` (missing steps, bulk-flag drift, restored parity, step environment targets, single-quoted flags, commented-out enforcement). + +--- + +*ADR created by [adr-writer agent]. Review and finalize before changing status from Draft to Accepted.* diff --git a/pkg/linters/README.md b/pkg/linters/README.md index f3a8b8e5cbc..6f3245a0d0b 100644 --- a/pkg/linters/README.md +++ b/pkg/linters/README.md @@ -351,6 +351,7 @@ _ = typeassertionnil.Analyzer - The package is intentionally organized as a namespace (`pkg/linters/*`) so individual analyzers remain isolated and independently testable. - CI enforces selected production analyzers via the native and WebAssembly `LINTER_FLAGS` lists in `.github/workflows/cgo.yml`; `fprintferrorunchecked` is excluded from blocking checks and reported separately as a non-blocking warning on native Go code. +- `consolestderr` has dedicated blocking steps on both targets. `TestCIEnforcedLintersMatchTargets` compares the combined bulk and dedicated analyzer sets per target, independently of the registry guard; the pre-existing native-only `contextcancelnotdeferred` exception is tracked in #65753 and must be removed when WebAssembly enables it. - `typeassertionnil` is registered but excluded from CI enforcement pending a cross-platform enforcement-readiness audit. - `unchecked_deferredclose` is registered but excluded from CI enforcement until existing deferred `Close()` findings are remediated. - `excessivefuncparams` exposes a `-max-params` analyzer flag and defaults to `8` parameters (`DefaultMaxParams`). diff --git a/pkg/linters/ci_sync_test.go b/pkg/linters/ci_sync_test.go new file mode 100644 index 00000000000..3ba50e80f05 --- /dev/null +++ b/pkg/linters/ci_sync_test.go @@ -0,0 +1,32 @@ +//go:build !integration + +package linters_test + +import ( + "testing" + + "github.com/stretchr/testify/assert" +) + +func TestCIEnforcedLinterSetsIgnoreShellComments(t *testing.T) { + t.Parallel() + workflow := []byte(` +jobs: + lint-go-custom: + steps: + - run: | + # GOOS=js GOARCH=wasm make golint-custom LINTER_FLAGS="-commentedout -test=false" + echo skipped + - run: | + # GOOS=js GOARCH=wasm + make golint-custom LINTER_FLAGS="-consolestderr -test=false" + - env: + GOOS: js + GOARCH: wasm + run: make golint-custom LINTER_FLAGS='-errstringmatch -test=false' +`) + + native, wasm := parseCIEnforcedLinterSets(t, workflow) + assert.Equal(t, []string{"consolestderr"}, sortedKeys(native)) + assert.Equal(t, []string{"errstringmatch"}, sortedKeys(wasm)) +} diff --git a/pkg/linters/doc_sync_test.go b/pkg/linters/doc_sync_test.go index e3231b9e4e2..c22a92915e3 100644 --- a/pkg/linters/doc_sync_test.go +++ b/pkg/linters/doc_sync_test.go @@ -12,6 +12,7 @@ import ( "testing" "github.com/github/gh-aw/pkg/linters" + "github.com/goccy/go-yaml" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" ) @@ -19,7 +20,7 @@ import ( var ( docBulletRe = regexp.MustCompile(`^//\s+-\s+([a-z0-9_-]+)\s+—`) readmeTableRe = regexp.MustCompile(`^\|\s+` + "`" + `([a-z0-9_-]+)` + "`" + `\s+\|`) - linterFlagsRe = regexp.MustCompile(`LINTER_FLAGS="([^"]+)"`) + linterFlagsRe = regexp.MustCompile(`LINTER_FLAGS=["']([^"']+)["']`) ) var notYetEnforced = map[string]string{ @@ -121,17 +122,11 @@ func TestCIEnforcedLintersMatchRegistry(t *testing.T) { workflowBytes, err := os.ReadFile("../../.github/workflows/cgo.yml") require.NoError(t, err, "cgo.yml must be present") - flagMatches := linterFlagsRe.FindAllStringSubmatch(string(workflowBytes), -1) - require.Len(t, flagMatches, 3, "cgo.yml must define native, console-stream, and wasm LINTER_FLAGS values") - + native, wasm := parseCIEnforcedLinterSets(t, workflowBytes) enforced := make(map[string]struct{}) - for _, match := range flagMatches { - for flag := range strings.FieldsSeq(match[1]) { - if flag == "-test=false" { - continue - } - require.Truef(t, strings.HasPrefix(flag, "-"), "LINTER_FLAGS entry %q must be a flag", flag) - enforced[strings.TrimPrefix(flag, "-")] = struct{}{} + for _, target := range []map[string]struct{}{native, wasm} { + for name := range target { + enforced[name] = struct{}{} } } @@ -150,6 +145,95 @@ func TestCIEnforcedLintersMatchRegistry(t *testing.T) { assert.NotContainsf(t, enforced, name, "remove %q from notYetEnforced once CI enforces it", name) assert.NotEmptyf(t, reason, "notYetEnforced analyzer %q must have a reason", name) } + for name := range enforced { + assert.Containsf(t, registry, name, "CI-enforced analyzer %q must be registered", name) + } +} + +func TestCIEnforcedLintersMatchTargets(t *testing.T) { + t.Parallel() + workflowBytes, err := os.ReadFile("../../.github/workflows/cgo.yml") + require.NoError(t, err, "cgo.yml must be present") + + native, wasm := parseCIEnforcedLinterSets(t, workflowBytes) + require.NotEmpty(t, native, "cgo.yml must enforce custom linters on native Go code") + require.NotEmpty(t, wasm, "cgo.yml must enforce custom linters on WebAssembly code") + + // Track the pre-existing asymmetry separately from analyzers not enforced at all. + nativeOnly := map[string]string{ + "contextcancelnotdeferred": "existing native-only enforcement is tracked in #65753; remove this exception when wasm enables it", + } + expectedNativeOnly := make(map[string]struct{}, len(nativeOnly)) + for name, reason := range nativeOnly { + assert.NotEmptyf(t, reason, "native-only analyzer %q must have a reason", name) + expectedNativeOnly[name] = struct{}{} + } + assert.Equal(t, sortedKeys(expectedNativeOnly), linterSetDifference(native, wasm), + "native and wasm CI must enforce the same custom linters except documented native-only exceptions; "+ + "mirror dedicated steps as well as bulk flags, and remove exceptions once parity is restored") + assert.Empty(t, linterSetDifference(wasm, native), + "wasm CI must not enforce custom linters missing from native CI") +} + +func parseCIEnforcedLinterSets(t *testing.T, workflowBytes []byte) (map[string]struct{}, map[string]struct{}) { + t.Helper() + var workflow struct { + Jobs map[string]struct { + Steps []struct { + Run string `yaml:"run"` + Env map[string]string `yaml:"env"` + } `yaml:"steps"` + } `yaml:"jobs"` + } + require.NoError(t, yaml.Unmarshal(workflowBytes, &workflow), "CI workflow must be valid YAML") + job, ok := workflow.Jobs["lint-go-custom"] + require.True(t, ok, "CI workflow must define the lint-go-custom job") + + native := make(map[string]struct{}) + wasm := make(map[string]struct{}) + for _, step := range job.Steps { + activeLines := slices.DeleteFunc(strings.Split(step.Run, "\n"), func(line string) bool { + return strings.HasPrefix(strings.TrimSpace(line), "#") + }) + run := strings.Join(activeLines, "\n") + goos, goarch := step.Env["GOOS"], step.Env["GOARCH"] + for field := range strings.FieldsSeq(run) { + key, value, ok := strings.Cut(field, "=") + if !ok { + continue + } + switch key { + case "GOOS": + goos = strings.Trim(value, `"'`) + case "GOARCH": + goarch = strings.Trim(value, `"'`) + } + } + target := native + if goos == "js" && goarch == "wasm" { + target = wasm + } + for _, match := range linterFlagsRe.FindAllStringSubmatch(run, -1) { + for flag := range strings.FieldsSeq(match[1]) { + if flag == "-test=false" { + continue + } + require.Truef(t, strings.HasPrefix(flag, "-"), "LINTER_FLAGS entry %q must be a flag", flag) + target[strings.TrimPrefix(flag, "-")] = struct{}{} + } + } + } + return native, wasm +} + +func linterSetDifference(left, right map[string]struct{}) []string { + difference := make(map[string]struct{}) + for name := range left { + if _, ok := right[name]; !ok { + difference[name] = struct{}{} + } + } + return sortedKeys(difference) } func parseDocBulletSet(t *testing.T) map[string]struct{} {