Skip to content

Cover wasm-only files in custom lint CI #42650

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

Merged
pelikhan merged 3 commits into from
Jul 1, 2026
+20 −6
Merged

Cover wasm-only files in custom lint CI #42650

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
e9ad6e5
Initial plan
Copilot Jul 1, 2026
b6cb68c
Plan minimal lint-gap fix
Copilot Jul 1, 2026
7e0f82a
Fix wasm custom lint coverage
Copilot Jul 1, 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
3 changes: 3 additions & 0 deletions .github/workflows/cgo.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1207,6 +1207,9 @@ jobs:
- name: Run custom linters
run: make golint-custom LINTER_FLAGS="-errstringmatch -panicinlibrarycode -manualmutexunlock -osexitinlibrary -rawloginlib -regexpcompileinfunction -fprintlnsprintf -strconvparseignorederror -jsonmarshalignoredeerror -uncheckedtypeassertion -fmterrorfnoverbs -tolowerequalfold -httpnoctx -timeafterleak -errortypeassertion -execcommandwithoutcontext -test=false"

- name: Run custom linters (wasm)
run: GOOS=js GOARCH=wasm make golint-custom LINTER_FLAGS="-errstringmatch -panicinlibrarycode -manualmutexunlock -osexitinlibrary -rawloginlib -regexpcompileinfunction -fprintlnsprintf -strconvparseignorederror -jsonmarshalignoredeerror -uncheckedtypeassertion -fmterrorfnoverbs -tolowerequalfold -httpnoctx -timeafterleak -errortypeassertion -execcommandwithoutcontext -test=false" LINTER_PACKAGES="./pkg/console ./pkg/parser ./pkg/styles ./pkg/tty ./pkg/workflow"

@github-actions github-actions Bot Jul 1, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[/codebase-design] The full LINTER_FLAGS string is copy-pasted from the step above — if a new analyzer flag is added to the main step, the wasm step silently misses it.

💡 Suggestion: extract a Makefile variable

Define a shared default in the Makefile so the flag set is the single source of truth:

# Default flags shared by all golint-custom passes
LINTER_FLAGS ?= -errstringmatch -panicinlibrarycode ... -test=false

Then the CI steps simply omit LINTER_FLAGS and both inherit the same defaults automatically:

- name: Run custom linters
  run: make golint-custom
- name: Run custom linters (wasm)
  run: GOOS=js GOARCH=wasm make golint-custom LINTER_PACKAGES="./pkg/console ..."

@copilot please address this.

@github-actions github-actions Bot Jul 1, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[/diagnosing-bugs] The wasm package list (./pkg/console ./pkg/parser ./pkg/styles ./pkg/tty ./pkg/workflow) is hardcoded. Any future package that gains a *_wasm.go production file won't be covered automatically, recreating the same blind spot this PR fixes.

💡 Suggestion: auto-discover packages with wasm files

Consider deriving the list from the repository instead:

WASM_PACKAGES ?= $(shell grep -rl '(go/redacted):build js' --include='*.go' $(shell go list -f '{{.Dir}}' ./...) | xargs -I{} dirname {} | sort -u | sed 's|.*|\./&|')

Or document the manual curation in a comment adjacent to the step so reviewers know to update it when adding new wasm files.

@copilot please address this.

@github-actions github-actions Bot Jul 1, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Duplicated LINTER_FLAGS will silently diverge from the default step over time.

💡 Suggested fix

The exact same 15-analyzer flags string is copy-pasted into both CI steps. Whenever an analyzer is added to or removed from the default step, the wasm step silently uses a stale set — meaning wasm-only code could evade a newly added rule, or the step could fail spuriously when an old flag is removed.

Extract the shared flags to a workflow-level env: variable or a Makefile variable so both invocations stay in sync automatically:

env:
  CUSTOM_LINTER_FLAGS: >-
    -errstringmatch -panicinlibrarycode -manualmutexunlock ...

jobs:
  ...
    - name: Run custom linters
      run: make golint-custom LINTER_FLAGS="$CUSTOM_LINTER_FLAGS"

    - name: Run custom linters (wasm)
      run: GOOS=js GOARCH=wasm make golint-custom LINTER_FLAGS="$CUSTOM_LINTER_FLAGS" LINTER_PACKAGES="..."

Or add a DEFAULT_LINTER_FLAGS variable to the Makefile and reference it from both steps. Either approach prevents silent coverage gaps when the analyzer set evolves.

@github-actions github-actions Bot Jul 1, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hardcoded LINTER_PACKAGES list will silently miss future wasm-only packages.

💡 Details

The five packages are manually enumerated. Any new package that gains a *_wasm.go file outside this list will never be covered by the wasm lint pass, and there is no automated guard to catch the omission.

At minimum, add a comment that developers must keep this list in sync with:

find pkg -name '*_wasm.go' | xargs -I{} dirname {} | sort -u

A stronger fix would derive the list dynamically in the Makefile by filtering packages that contain wasm build-tag files, so new packages are covered automatically.


# Ensure no action shell scripts invoke python or python3
- name: Lint action shell scripts
run: make lint-action-sh
Expand Down
7 changes: 5 additions & 2 deletions Makefile
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -715,14 +715,17 @@ golint:
# Builds and runs linters defined in cmd/linters against the full repository.
# Override the large-function line limit with: make golint-custom MAX_LINES=80
# Limit the analyzer set with: make golint-custom LINTER_FLAGS="-errstringmatch -test=false"
# Limit analyzed packages with: make golint-custom LINTER_PACKAGES="./pkg/console ./pkg/workflow"
# Prefix with GOOS/GOARCH to analyze alternate build targets without cross-compiling the linter runner itself.
MAX_LINES ?= 60
LINTER_FLAGS ?=
LINTER_PACKAGES ?= ./cmd/... ./pkg/...
.PHONY: golint-custom
golint-custom:
@echo "Building custom linters..."
@go build -o /tmp/gh-aw-linters ./cmd/linters
@env -u GOOS -u GOARCH go build -o /tmp/gh-aw-linters ./cmd/linters
Comment thread
pelikhan marked this conversation as resolved.
@echo "Running custom linters (largefunc max-lines=$(MAX_LINES))..."
@/tmp/gh-aw-linters $(LINTER_FLAGS) -largefunc.max-lines=$(MAX_LINES) ./cmd/... ./pkg/...
@/tmp/gh-aw-linters $(LINTER_FLAGS) -largefunc.max-lines=$(MAX_LINES) $(LINTER_PACKAGES)

# Run incremental linter (only changed files since BASE_REF)
# This provides 50-75% faster linting on PRs by only checking changed files
Expand Down
Binary file modified gh-aw-wasm
View file
Open in desktop
Binary file not shown.
3 changes: 2 additions & 1 deletion pkg/console/console_wasm.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ package console
import (
"fmt"
"os"
"strconv"
"strings"
)

Expand Down Expand Up @@ -38,7 +39,7 @@ func FormatError(err CompilerError) string {

if len(err.Context) > 0 && err.Position.Line > 0 {
maxLineNum := err.Position.Line + len(err.Context)/2
lineNumWidth := len(fmt.Sprintf("%d", maxLineNum))
lineNumWidth := len(strconv.Itoa(maxLineNum))
for i, line := range err.Context {
lineNum := err.Position.Line - len(err.Context)/2 + i
if lineNum < 1 {
Expand Down
13 changes: 10 additions & 3 deletions pkg/workflow/github_cli_wasm.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,15 +16,15 @@ import (
// tool validation (WithSkipValidation(true)).

func setupGHCommand(ctx context.Context, args ...string) *exec.Cmd {
return exec.Command("echo", "gh CLI not available in Wasm")
return ghUnavailableCommand(ctx)
}

func ExecGH(args ...string) *exec.Cmd {
return exec.Command("echo", "gh CLI not available in Wasm")
return ghUnavailableCommand(context.Background())
}

func ExecGHContext(ctx context.Context, args ...string) *exec.Cmd {
return exec.Command("echo", "gh CLI not available in Wasm")
return ghUnavailableCommand(ctx)
}

func ExecGHWithOutput(args ...string) (stdout, stderr bytes.Buffer, err error) {
Expand All @@ -43,6 +43,13 @@ func RunGHCombined(spinnerMessage string, args ...string) ([]byte, error) {
return nil, errors.New("gh CLI not available in Wasm")
}

func ghUnavailableCommand(ctx context.Context) *exec.Cmd {
if ctx == nil {

@github-actions github-actions Bot Jul 1, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[/diagnosing-bugs] The nil-context guard is good defensive programming, but none of the current callers can pass nil (they either have a real ctx or use context.Background()). Adding a brief comment would explain the intent for future readers and keep it from looking like dead code.

💡 Suggested comment 
func ghUnavailableCommand(ctx context.Context) *exec.Cmd {
    // Guard against nil context: exec.CommandContext panics on nil.
    if ctx == nil {
        ctx = context.Background()
    }
    return exec.CommandContext(ctx, "echo", "gh CLI not available in Wasm")
}

@copilot please address this.

ctx = context.Background()
}
return exec.CommandContext(ctx, "echo", "gh CLI not available in Wasm")

@github-actions github-actions Bot Jul 1, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Using exec.CommandContext changes the observable error type when context is already cancelled.

💡 Details

Previously all three stubs used exec.Command, which returns an *exec.Cmd that fails at Start()/Run() with a platform-not-supported error regardless of context state. After this change, if the caller's context is already cancelled or deadline-exceeded when cmd.Start() is called, the error is context.Canceled / context.DeadlineExceeded instead.

Any caller that inspects the error message or type (e.g., looking for "not available in Wasm") will receive a different error class. The file comment states these stubs are never called at runtime due to WithSkipValidation(true), but that is a runtime contract that tests or future code might not uphold. If using exec.CommandContext is intentional to satisfy the linter, consider adding a comment explaining the known behavioral difference, or wrapping the result to normalize the error.

}

func ForceGHHostEnv(cmd *exec.Cmd, host string) {
// no-op in Wasm: gh CLI subprocesses are not run
}
Expand Down
Loading