Skip to content
789 changes: 400 additions & 389 deletions cmd/gh-aw/main.go

Large diffs are not rendered by default.

Original file line number Diff line number Diff line change
@@ -0,0 +1,58 @@
# ADR-47910: Options Struct Pattern for Multi-Boolean Function Signatures

**Date**: 2026-07-25
**Status**: Draft
**Deciders**: Unknown (automated lint-compliance PR)

---

### Context

The codebase enforces coding standards via a custom linter (`make golint-custom`) that includes rules for maximum function parameter count and maximum function length. Over time, `CheckAndPrepareDockerImages` in `pkg/cli/docker_images.go` accumulated 9 positional boolean parameters — one per Docker-based static-analysis tool (zizmor, poutine, actionlint, runner-guard, syft, grype, grant, yamllint). Positional booleans at call sites are indistinguishable without inspecting the signature, making misorderings silent bugs. Simultaneously, `cmd/gh-aw/main.go` had an `init()` function of ~447 lines and a `compileCmd.RunE` closure of ~129 lines, both far exceeding the linter's function-length threshold. These violations were surfaced as non-shared findings in `make golint-custom`, blocking CI.

### Decision

We will adopt the **Options Struct pattern** for functions that accept more boolean parameters than the linter's configured threshold, and decompose functions that exceed the length limit into focused named helpers.

Concretely:
- `CheckAndPrepareDockerImages` now accepts a single `DockerImagesOptions` struct instead of 9 positional booleans; all callers are updated.
- `compileCmd.RunE` is extracted to `runCompileCmd`, with flag parsing moved to `parseCompileFlags` (returning a `compileFlags` struct) and config assembly moved to `buildCompileConfig`.
- `init()` is decomposed into 10 focused helpers: `setupRootCmdGroups`, `setupRootCmdMeta`, `makeCustomHelpCmd`, `registerCompileFlags`, `setupSetupGroupCmds`, `setupDevelopmentGroupCmds`, `setupExecutionGroupCmds`, `setupAnalysisGroupCmds`, `setupUtilityGroupCmds`, `fixAllSubCmdHelpFlags`.
- The hardcoded container path `/tmp/gh-aw-grant-policy.yaml` is extracted to the named constant `grantContainerPolicyPath`.
- The `defer timer.Stop()` inside a `for` loop in `spawnMCPInspector` is moved outside the `select` block to fix a resource-leak bug.

### Alternatives Considered

#### Alternative 1: Lint suppression directives (`//nolint`)

Add `//nolint:param-count` or equivalent suppression comments at the offending functions to silence the linter without changing the code. This avoids churn and keeps call sites unchanged.

Why not chosen: Suppression discards the signal the lint rule is trying to send. The 9-boolean signature is a genuine readability and correctness hazard — callers cannot verify argument order without reading the signature. Suppression would also set a precedent for silencing violations rather than resolving them.

#### Alternative 2: Raise the linter thresholds

Increase the maximum parameter count and function length limits in the linter configuration so that the existing code passes without modification.

Why not chosen: The existing limits reflect deliberate standards for the project. Relaxing them to accommodate one function would weaken the rules for the entire codebase and invite future growth of already over-complex functions.

### Consequences

#### Positive
- Named struct fields at call sites are self-documenting; readers no longer need to look up parameter order to understand `DockerImagesOptions{Zizmor: true, Grype: true}`.
- Adding a new tool to `DockerImagesOptions` does not require updating every call site (zero-value defaults to `false`).
- Smaller, focused helper functions (`setupSetupGroupCmds`, etc.) are individually testable and easier to review in isolation.
- The defer-in-loop fix eliminates a resource-management bug where `timer.Stop()` would only run at function return rather than per iteration.

#### Negative
- Existing callers of `CheckAndPrepareDockerImages` must be updated to use the struct literal; this is a breaking API change within the package.
- The `compileFlags` struct and its associated `parseCompileFlags`/`buildCompileConfig` functions add an intermediate layer of indirection to the compile path that readers must traverse.
- The `init()` decomposition significantly increases line count in `main.go`, as each helper requires its own function signature, making the file longer despite each function being shorter.

#### Neutral
- The options struct pattern is idiomatic Go; future contributors familiar with the language will recognize it immediately.
- All lint findings fixed in this PR are enforcement of pre-existing rules, not the introduction of new rules or tooling.
- The named constant `grantContainerPolicyPath` is only used in one place today; its value as a constant will become apparent if the path needs to change or be referenced from tests.

---

*ADR created by [adr-writer agent]. Review and finalize before changing status from Draft to Accepted.*
48 changes: 30 additions & 18 deletions pkg/cli/docker_images.go
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,18 @@ const (
YamllintImage = "pipelinecomponents/yamllint:latest"
)

// DockerImagesOptions specifies which static-analysis Docker images are required.
type DockerImagesOptions struct {
Zizmor bool
Poutine bool
Actionlint bool
RunnerGuard bool
Syft bool
Grype bool
Grant bool
Yamllint bool
}

// dockerPullState tracks the state of docker pull operations
type dockerPullState struct {
mu sync.RWMutex
Expand Down Expand Up @@ -228,52 +240,52 @@ func StartDockerImageDownload(ctx context.Context, image string) bool {
// Returns:
// - nil if all required images are available
// - error if Docker is unavailable or images are downloading/need to be downloaded
func CheckAndPrepareDockerImages(ctx context.Context, useZizmor, usePoutine, useActionlint, useRunnerGuard, useSyft, useGrype, useGrant, useYamllint bool) error {
func CheckAndPrepareDockerImages(ctx context.Context, opts DockerImagesOptions) error {
// If no tools requested, nothing to do
if !useZizmor && !usePoutine && !useActionlint && !useRunnerGuard && !useSyft && !useGrype && !useGrant && !useYamllint {
if !opts.Zizmor && !opts.Poutine && !opts.Actionlint && !opts.RunnerGuard && !opts.Syft && !opts.Grype && !opts.Grant && !opts.Yamllint {
return nil
}

// Check if Docker daemon is available before attempting any image operations
if !IsDockerAvailable(ctx) {
var requestedTools []string
var paramsList []string
if useZizmor {
if opts.Zizmor {
tool := "zizmor"
requestedTools = append(requestedTools, tool)
paramsList = append(paramsList, tool+": false")
}
if usePoutine {
if opts.Poutine {
tool := "poutine"
requestedTools = append(requestedTools, tool)
paramsList = append(paramsList, tool+": false")
}
if useActionlint {
if opts.Actionlint {
tool := "actionlint"
requestedTools = append(requestedTools, tool)
paramsList = append(paramsList, tool+": false")
}
if useRunnerGuard {
if opts.RunnerGuard {
tool := "runner-guard"
requestedTools = append(requestedTools, tool)
paramsList = append(paramsList, tool+": false")
}
if useSyft {
if opts.Syft {
tool := "syft"
requestedTools = append(requestedTools, tool)
paramsList = append(paramsList, tool+": false")
}
if useGrype {
if opts.Grype {
tool := "grype"
requestedTools = append(requestedTools, tool)
paramsList = append(paramsList, tool+": false")
}
if useGrant {
if opts.Grant {
tool := "grant"
requestedTools = append(requestedTools, tool)
paramsList = append(paramsList, tool+": false")
}
if useYamllint {
if opts.Yamllint {
tool := "yamllint"
requestedTools = append(requestedTools, tool)
paramsList = append(paramsList, tool+": false")
Expand All @@ -296,14 +308,14 @@ func CheckAndPrepareDockerImages(ctx context.Context, useZizmor, usePoutine, use
image string
name string
}{
{useZizmor, ZizmorImage, "zizmor"},
{usePoutine, PoutineImage, "poutine"},
{useActionlint, ActionlintImage, "actionlint"},
{useRunnerGuard, RunnerGuardImage, "runner-guard"},
{useSyft, SyftImage, "syft"},
{useGrype, GrypeImage, "grype"},
{useGrant, GrantImage, "grant"},
{useYamllint, YamllintImage, "yamllint"},
{opts.Zizmor, ZizmorImage, "zizmor"},
{opts.Poutine, PoutineImage, "poutine"},
{opts.Actionlint, ActionlintImage, "actionlint"},
{opts.RunnerGuard, RunnerGuardImage, "runner-guard"},
{opts.Syft, SyftImage, "syft"},
{opts.Grype, GrypeImage, "grype"},
{opts.Grant, GrantImage, "grant"},
{opts.Yamllint, YamllintImage, "yamllint"},
}

for _, img := range imagesToCheck {
Expand Down
22 changes: 11 additions & 11 deletions pkg/cli/docker_images_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,7 @@ func TestCheckAndPrepareDockerImages_NoToolsRequested(t *testing.T) {
ResetDockerPullState()

// When no tools are requested, should return nil
err := CheckAndPrepareDockerImages(context.Background(), false, false, false, false, false, false, false, false)
err := CheckAndPrepareDockerImages(context.Background(), DockerImagesOptions{})
if err != nil {
t.Errorf("Expected no error when no tools requested, got: %v", err)
}
Expand All @@ -31,7 +31,7 @@ func TestCheckAndPrepareDockerImages_ImageAlreadyDownloading(t *testing.T) {
SetDockerImageDownloading(ZizmorImage, true)

// Should return an error indicating to retry
err := CheckAndPrepareDockerImages(context.Background(), true, false, false, false, false, false, false, false)
err := CheckAndPrepareDockerImages(context.Background(), DockerImagesOptions{Zizmor: true})
if err == nil {
t.Error("Expected error when image is downloading, got nil")
}
Expand Down Expand Up @@ -146,7 +146,7 @@ func TestCheckAndPrepareDockerImages_MultipleImages(t *testing.T) {
SetDockerImageDownloading(PoutineImage, true)

// Request all tools
err := CheckAndPrepareDockerImages(context.Background(), true, true, true, false, false, false, false, false)
err := CheckAndPrepareDockerImages(context.Background(), DockerImagesOptions{Zizmor: true, Poutine: true, Actionlint: true})
if err == nil {
t.Error("Expected error when images are downloading, got nil")
}
Expand All @@ -172,7 +172,7 @@ func TestCheckAndPrepareDockerImages_RetryMessageFormat(t *testing.T) {
// Simulate zizmor downloading
SetDockerImageDownloading(ZizmorImage, true)

err := CheckAndPrepareDockerImages(context.Background(), true, false, false, false, false, false, false, false)
err := CheckAndPrepareDockerImages(context.Background(), DockerImagesOptions{Zizmor: true})
if err == nil {
t.Fatal("Expected error when image is downloading")
}
Expand Down Expand Up @@ -207,7 +207,7 @@ func TestCheckAndPrepareDockerImages_StartedDownloadingMessage(t *testing.T) {
// when the image is marked as downloading
SetDockerImageDownloading(ZizmorImage, true)

err := CheckAndPrepareDockerImages(context.Background(), true, false, false, false, false, false, false, false)
err := CheckAndPrepareDockerImages(context.Background(), DockerImagesOptions{Zizmor: true})
if err == nil {
t.Fatal("Expected error when image is downloading")
}
Expand All @@ -231,7 +231,7 @@ func TestCheckAndPrepareDockerImages_ImageAlreadyAvailable(t *testing.T) {
SetMockImageAvailable(ZizmorImage, true)

// Should not return an error since the image is available
err := CheckAndPrepareDockerImages(context.Background(), true, false, false, false, false, false, false, false)
err := CheckAndPrepareDockerImages(context.Background(), DockerImagesOptions{Zizmor: true})
if err != nil {
t.Errorf("Expected no error when image is available, got: %v", err)
}
Expand Down Expand Up @@ -538,7 +538,7 @@ func TestCheckAndPrepareDockerImages_DockerUnavailable(t *testing.T) {
SetMockDockerAvailable(false)

// Should return a clear error about Docker not being available
err := CheckAndPrepareDockerImages(context.Background(), true, false, false, false, false, false, false, false)
err := CheckAndPrepareDockerImages(context.Background(), DockerImagesOptions{Zizmor: true})
if err == nil {
t.Fatal("Expected error when Docker is unavailable, got nil")
}
Expand Down Expand Up @@ -576,7 +576,7 @@ func TestCheckAndPrepareDockerImages_DockerUnavailable_MultipleTools(t *testing.
SetMockDockerAvailable(false)

// Request multiple tools
err := CheckAndPrepareDockerImages(context.Background(), true, false, true, false, false, false, false, false)
err := CheckAndPrepareDockerImages(context.Background(), DockerImagesOptions{Zizmor: true, Actionlint: true})
if err == nil {
t.Fatal("Expected error when Docker is unavailable, got nil")
}
Expand Down Expand Up @@ -615,7 +615,7 @@ func TestCheckAndPrepareDockerImages_DockerUnavailable_NoTools(t *testing.T) {
SetMockDockerAvailable(false)

// When no tools requested, should return nil even if Docker is unavailable
err := CheckAndPrepareDockerImages(context.Background(), false, false, false, false, false, false, false, false)
err := CheckAndPrepareDockerImages(context.Background(), DockerImagesOptions{})
if err != nil {
t.Errorf("Expected no error when no tools requested (even with Docker unavailable), got: %v", err)
}
Expand Down Expand Up @@ -647,7 +647,7 @@ func TestCheckAndPrepareDockerImages_DockerUnavailable_ReturnsTypedError(t *test
ResetDockerPullState()
SetMockDockerAvailable(false)

err := CheckAndPrepareDockerImages(context.Background(), false, false, true, false, false, false, false, false)
err := CheckAndPrepareDockerImages(context.Background(), DockerImagesOptions{Actionlint: true})
if err == nil {
t.Fatal("Expected error when Docker is unavailable, got nil")
}
Expand Down Expand Up @@ -676,7 +676,7 @@ func TestCheckAndPrepareDockerImages_RunnerGuardImageDownloading(t *testing.T) {
SetDockerImageDownloading(RunnerGuardImage, true)

// Request all tools, including runner-guard
err := CheckAndPrepareDockerImages(context.Background(), true, true, true, true, false, false, false, false)
err := CheckAndPrepareDockerImages(context.Background(), DockerImagesOptions{Zizmor: true, Poutine: true, Actionlint: true, RunnerGuard: true})
if err == nil {
t.Error("Expected error when images are downloading, got nil")
}
Expand Down
6 changes: 5 additions & 1 deletion pkg/cli/grant.go
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,10 @@ var grantLog = logger.New("cli:grant")

const grantPolicyFilename = ".grant.yaml"

// grantContainerPolicyPath is the path inside the Docker container where the
// grant policy file is mounted.
const grantContainerPolicyPath = "/tmp/gh-aw-grant-policy.yaml"

type grantOutput struct {
Tool string `json:"tool"`
Run struct {
Expand Down Expand Up @@ -149,7 +153,7 @@ func grantPolicyFile() (string, error) {
}

func grantRunOnImage(imageRef, policyFile string, verbose bool) (*grantOutput, error) {
containerPolicyPath := "/tmp/gh-aw-grant-policy.yaml"
containerPolicyPath := grantContainerPolicyPath

// #nosec G204 -- imageRef and policyFile are derived from compiled lock files and the
// current repository checkout. exec.Command passes arguments directly without a shell.
Expand Down
2 changes: 1 addition & 1 deletion pkg/cli/mcp_inspect_inspector.go
Original file line number Diff line number Diff line change
Expand Up @@ -62,11 +62,11 @@ func spawnMCPInspector(ctx context.Context, workflowFile string, serverFilter st
// Give each process a chance to clean up
if i < len(serverProcesses)-1 {
timer := time.NewTimer(mcpProcessCleanupDelay)
defer timer.Stop()
select {
case <-timer.C:
case <-gctx.Done():
}
timer.Stop()
}
}
if err := g.Wait(); err != nil {
Expand Down
11 changes: 10 additions & 1 deletion pkg/cli/mcp_tools_readonly.go
Original file line number Diff line number Diff line change
Expand Up @@ -144,7 +144,16 @@ Returns JSON array with validation results for each workflow:
// Check if any static analysis tools are requested that require Docker images
if args.Zizmor || args.Poutine || args.Actionlint || args.RunnerGuard || args.Syft || args.Grype || args.Grant || args.Yamllint {
// Check if Docker images are available; if not, start downloading and return retry message
if err := CheckAndPrepareDockerImages(ctx, args.Zizmor, args.Poutine, args.Actionlint, args.RunnerGuard, args.Syft, args.Grype, args.Grant, args.Yamllint); err != nil {
if err := CheckAndPrepareDockerImages(ctx, DockerImagesOptions{
Zizmor: args.Zizmor,
Poutine: args.Poutine,
Actionlint: args.Actionlint,
RunnerGuard: args.RunnerGuard,
Syft: args.Syft,
Grype: args.Grype,
Grant: args.Grant,
Yamllint: args.Yamllint,
}); err != nil {
var dockerUnavailableErr *DockerUnavailableError
if errors.As(err, &dockerUnavailableErr) {
// Docker daemon is not running. Instead of failing every workflow,
Expand Down
Loading
Loading