Skip to content

Commit dace583

Browse files
pelikhanCopilotgithub-actions[bot]
authored
fix: enforce consolestderr on wasm and guard native/wasm CI parity (#67634)
* fix: enforce console formatting on wasm and guard CI parity Mirror the dedicated consolestderr check for WebAssembly and compare native and wasm analyzer sets independently of registry membership. Track the existing contextcancelnotdeferred exception and cover target drift regressions. Fixes #67609 Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> * docs: add draft ADR for native/wasm linter parity enforcement * fix: ignore commented CI linter commands and simplify regression Filter full-line shell comments before both target and analyzer scans. Replace workflow-format-dependent mutation tests with a focused standalone block-scalar regression while retaining the production parity guard. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> --------- Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
1 parent 1064352 commit dace583

5 files changed

Lines changed: 185 additions & 11 deletions

File tree

‎.github/workflows/cgo.yml‎

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1554,6 +1554,9 @@ jobs:
15541554
- name: Run custom linters (wasm)
15551555
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"
15561556

1557+
- name: Check console stderr formatting (wasm)
1558+
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"
1559+
15571560
# Ensure no action shell scripts invoke python or python3
15581561
- name: Lint action shell scripts
15591562
run: make lint-action-sh
Lines changed: 54 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,54 @@
1+
# ADR-67634: Enforce Native/WebAssembly Custom-Linter Parity via YAML-Parsing CI Sync Tests
2+
3+
**Date**: 2026-10-11
4+
**Status**: Draft
5+
**Deciders**: pelikhan (PR #67634 author), gh-aw maintainers
6+
7+
---
8+
9+
### Context
10+
11+
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.
12+
13+
### Decision
14+
15+
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.
16+
17+
### Alternatives Considered
18+
19+
#### Alternative 1: Keep the hard-coded step-count guard and just add the missing wasm step
20+
21+
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.
22+
23+
#### Alternative 2: Generate the CI linter steps from the analyzer registry
24+
25+
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.
26+
27+
#### Alternative 3: Collapse to a single linter invocation covering both targets
28+
29+
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.
30+
31+
### Consequences
32+
33+
#### Positive
34+
35+
- Native-only or wasm-only enforcement gaps now fail fast in `go test ./pkg/linters` instead of reaching `main`.
36+
- `consolestderr` violations are caught in WebAssembly-constrained code paths, closing #67609.
37+
- Removing the hard-coded step count eliminates a recurring source of unrelated CI failures when `cgo.yml` is edited.
38+
- Deliberate exceptions are documented in code and tied to tracking issues, and stale exceptions are rejected automatically once parity is restored.
39+
40+
#### Negative
41+
42+
- 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.
43+
- The native-only exception list is a manually maintained allow-list that must be kept honest by reviewers.
44+
- 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.
45+
46+
#### Neutral
47+
48+
- CI runtime grows slightly with the extra dedicated wasm step.
49+
- `pkg/linters/README.md` becomes the canonical description of which analyzers are enforced per target and why exceptions exist.
50+
- 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).
51+
52+
---
53+
54+
*ADR created by [adr-writer agent]. Review and finalize before changing status from Draft to Accepted.*

‎pkg/linters/README.md‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -376,6 +376,7 @@ _ = typeassertionnil.Analyzer
376376

377377
- The package is intentionally organized as a namespace (`pkg/linters/*`) so individual analyzers remain isolated and independently testable.
378378
- 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.
379+
- `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.
379380
- `typeassertionnil` is registered but excluded from CI enforcement pending a cross-platform enforcement-readiness audit.
380381
- `unchecked_deferredclose` is registered but excluded from CI enforcement until existing deferred `Close()` findings are remediated.
381382
- `excessivefuncparams` exposes a `-max-params` analyzer flag and defaults to `8` parameters (`DefaultMaxParams`).

‎pkg/linters/ci_sync_test.go‎

Lines changed: 32 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,32 @@
1+
//go:build !integration
2+
3+
package linters_test
4+
5+
import (
6+
"testing"
7+
8+
"github.com/stretchr/testify/assert"
9+
)
10+
11+
func TestCIEnforcedLinterSetsIgnoreShellComments(t *testing.T) {
12+
t.Parallel()
13+
workflow := []byte(`
14+
jobs:
15+
lint-go-custom:
16+
steps:
17+
- run: |
18+
# GOOS=js GOARCH=wasm make golint-custom LINTER_FLAGS="-commentedout -test=false"
19+
echo skipped
20+
- run: |
21+
# GOOS=js GOARCH=wasm
22+
make golint-custom LINTER_FLAGS="-consolestderr -test=false"
23+
- env:
24+
GOOS: js
25+
GOARCH: wasm
26+
run: make golint-custom LINTER_FLAGS='-errstringmatch -test=false'
27+
`)
28+
29+
native, wasm := parseCIEnforcedLinterSets(t, workflow)
30+
assert.Equal(t, []string{"consolestderr"}, sortedKeys(native))
31+
assert.Equal(t, []string{"errstringmatch"}, sortedKeys(wasm))
32+
}

‎pkg/linters/doc_sync_test.go‎

Lines changed: 95 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -12,14 +12,15 @@ import (
1212
"testing"
1313

1414
"github.com/github/gh-aw/pkg/linters"
15+
"github.com/goccy/go-yaml"
1516
"github.com/stretchr/testify/assert"
1617
"github.com/stretchr/testify/require"
1718
)
1819

1920
var (
2021
docBulletRe = regexp.MustCompile(`^//\s+-\s+([a-z0-9_-]+)\s+—`)
2122
readmeTableRe = regexp.MustCompile(`^\|\s+` + "`" + `([a-z0-9_-]+)` + "`" + `\s+\|`)
22-
linterFlagsRe = regexp.MustCompile(`LINTER_FLAGS="([^"]+)"`)
23+
linterFlagsRe = regexp.MustCompile(`LINTER_FLAGS=["']([^"']+)["']`)
2324
)
2425

2526
var notYetEnforced = map[string]string{
@@ -121,17 +122,11 @@ func TestCIEnforcedLintersMatchRegistry(t *testing.T) {
121122
workflowBytes, err := os.ReadFile("../../.github/workflows/cgo.yml")
122123
require.NoError(t, err, "cgo.yml must be present")
123124

124-
flagMatches := linterFlagsRe.FindAllStringSubmatch(string(workflowBytes), -1)
125-
require.Len(t, flagMatches, 3, "cgo.yml must define native, console-stream, and wasm LINTER_FLAGS values")
126-
125+
native, wasm := parseCIEnforcedLinterSets(t, workflowBytes)
127126
enforced := make(map[string]struct{})
128-
for _, match := range flagMatches {
129-
for flag := range strings.FieldsSeq(match[1]) {
130-
if flag == "-test=false" {
131-
continue
132-
}
133-
require.Truef(t, strings.HasPrefix(flag, "-"), "LINTER_FLAGS entry %q must be a flag", flag)
134-
enforced[strings.TrimPrefix(flag, "-")] = struct{}{}
127+
for _, target := range []map[string]struct{}{native, wasm} {
128+
for name := range target {
129+
enforced[name] = struct{}{}
135130
}
136131
}
137132

@@ -150,6 +145,95 @@ func TestCIEnforcedLintersMatchRegistry(t *testing.T) {
150145
assert.NotContainsf(t, enforced, name, "remove %q from notYetEnforced once CI enforces it", name)
151146
assert.NotEmptyf(t, reason, "notYetEnforced analyzer %q must have a reason", name)
152147
}
148+
for name := range enforced {
149+
assert.Containsf(t, registry, name, "CI-enforced analyzer %q must be registered", name)
150+
}
151+
}
152+
153+
func TestCIEnforcedLintersMatchTargets(t *testing.T) {
154+
t.Parallel()
155+
workflowBytes, err := os.ReadFile("../../.github/workflows/cgo.yml")
156+
require.NoError(t, err, "cgo.yml must be present")
157+
158+
native, wasm := parseCIEnforcedLinterSets(t, workflowBytes)
159+
require.NotEmpty(t, native, "cgo.yml must enforce custom linters on native Go code")
160+
require.NotEmpty(t, wasm, "cgo.yml must enforce custom linters on WebAssembly code")
161+
162+
// Track the pre-existing asymmetry separately from analyzers not enforced at all.
163+
nativeOnly := map[string]string{
164+
"contextcancelnotdeferred": "existing native-only enforcement is tracked in #65753; remove this exception when wasm enables it",
165+
}
166+
expectedNativeOnly := make(map[string]struct{}, len(nativeOnly))
167+
for name, reason := range nativeOnly {
168+
assert.NotEmptyf(t, reason, "native-only analyzer %q must have a reason", name)
169+
expectedNativeOnly[name] = struct{}{}
170+
}
171+
assert.Equal(t, sortedKeys(expectedNativeOnly), linterSetDifference(native, wasm),
172+
"native and wasm CI must enforce the same custom linters except documented native-only exceptions; "+
173+
"mirror dedicated steps as well as bulk flags, and remove exceptions once parity is restored")
174+
assert.Empty(t, linterSetDifference(wasm, native),
175+
"wasm CI must not enforce custom linters missing from native CI")
176+
}
177+
178+
func parseCIEnforcedLinterSets(t *testing.T, workflowBytes []byte) (map[string]struct{}, map[string]struct{}) {
179+
t.Helper()
180+
var workflow struct {
181+
Jobs map[string]struct {
182+
Steps []struct {
183+
Run string `yaml:"run"`
184+
Env map[string]string `yaml:"env"`
185+
} `yaml:"steps"`
186+
} `yaml:"jobs"`
187+
}
188+
require.NoError(t, yaml.Unmarshal(workflowBytes, &workflow), "CI workflow must be valid YAML")
189+
job, ok := workflow.Jobs["lint-go-custom"]
190+
require.True(t, ok, "CI workflow must define the lint-go-custom job")
191+
192+
native := make(map[string]struct{})
193+
wasm := make(map[string]struct{})
194+
for _, step := range job.Steps {
195+
activeLines := slices.DeleteFunc(strings.Split(step.Run, "\n"), func(line string) bool {
196+
return strings.HasPrefix(strings.TrimSpace(line), "#")
197+
})
198+
run := strings.Join(activeLines, "\n")
199+
goos, goarch := step.Env["GOOS"], step.Env["GOARCH"]
200+
for field := range strings.FieldsSeq(run) {
201+
key, value, ok := strings.Cut(field, "=")
202+
if !ok {
203+
continue
204+
}
205+
switch key {
206+
case "GOOS":
207+
goos = strings.Trim(value, `"'`)
208+
case "GOARCH":
209+
goarch = strings.Trim(value, `"'`)
210+
}
211+
}
212+
target := native
213+
if goos == "js" && goarch == "wasm" {
214+
target = wasm
215+
}
216+
for _, match := range linterFlagsRe.FindAllStringSubmatch(run, -1) {
217+
for flag := range strings.FieldsSeq(match[1]) {
218+
if flag == "-test=false" {
219+
continue
220+
}
221+
require.Truef(t, strings.HasPrefix(flag, "-"), "LINTER_FLAGS entry %q must be a flag", flag)
222+
target[strings.TrimPrefix(flag, "-")] = struct{}{}
223+
}
224+
}
225+
}
226+
return native, wasm
227+
}
228+
229+
func linterSetDifference(left, right map[string]struct{}) []string {
230+
difference := make(map[string]struct{})
231+
for name := range left {
232+
if _, ok := right[name]; !ok {
233+
difference[name] = struct{}{}
234+
}
235+
}
236+
return sortedKeys(difference)
153237
}
154238

155239
func parseDocBulletSet(t *testing.T) map[string]struct{} {

0 commit comments

Comments
 (0)