fix(lint): resolve 4 targeted golint-custom findings in pkg/cli, cmd/gh-aw, and pkg/linters

[Copilot]

Resolves all 4 non-shared findings reported by make golint-custom: a defer-in-loop, an over-limit parameter count, a hard-coded temp path, and several long functions.

pkg/cli fixes

• mcp_inspect_inspector.go — removed defer timer.Stop() from inside a for loop; replaced with direct call after the select block so cleanup happens per-iteration rather than at function return
• docker_images.go — replaced 9-parameter CheckAndPrepareDockerImages signature with a single DockerImagesOptions struct; all callers updated

  // before
  CheckAndPrepareDockerImages(ctx, true, false, true, false, false, true, false, true)
  // after
  CheckAndPrepareDockerImages(ctx, DockerImagesOptions{Zizmor: true, Actionlint: true, Grype: true, Yamllint: true})

• grant.go — extracted grantContainerPolicyPath = "/tmp/gh-aw-grant-policy.yaml" named constant; replaced bare string literal

cmd/gh-aw/main.go long-function splits

• compileCmd.RunE (was 129 lines) — extracted to named runCompileCmd function; flag parsing isolated into compileFlags struct + parseCompileFlags() + buildCompileConfig()
• SetUsageFunc literal (was 72 lines) — extracted fixUsagePath() and printUsageSubCmds() as package-level helpers
• init() (was 447 lines) — decomposed into 10 focused helpers: setupRootCmdGroups, setupRootCmdMeta, makeCustomHelpCmd, registerCompileFlags, setupSetupGroupCmds, setupDevelopmentGroupCmds, setupExecutionGroupCmds, setupAnalysisGroupCmds, setupUtilityGroupCmds, fixAllSubCmdHelpFlags

pkg/linters/stringbytesroundtrip

• analyzeRoundTrip (was 73 lines) — introduced roundTripTypes struct and extracted unpackConversionPair() helper to reduce the main function below the 60-line limit

• Fixes [lint-monster] targeted custom-lint fixes plus non-shared largefunc backlog (4 findings / 4 locations) #47900

---

Generated by 👨‍🍳 PR Sous Chef · gpt54 · 17.6 AIC · ⌖ 8.18 AIC · ⊞ 7.1K · ◷
_{Comment /souschef to run again}

---

Generated by 👨‍🍳 PR Sous Chef · gpt54 · 12.3 AIC · ⌖ 8.43 AIC · ⊞ 7.1K · ◷
_{Comment /souschef to run again}

[github-actions]

✅ Test Quality Sentinel completed test quality analysis.

[github-actions]

✅ Design Decision Gate 🏗️ completed the design decision gate check.

[github-actions]

🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅

[Copilot]

Pull request overview

Refactors targeted lint findings while preserving CLI and analyzer behavior.

Changes:

• Splits oversized CLI and linter functions into focused helpers.
• Introduces Docker image options and updates callers/tests.
• Fixes timer cleanup and extracts the Grant container path constant.

Show a summary per file

File	Description
cmd/gh-aw/main.go	Decomposes compile, help, and command setup logic.
pkg/cli/docker_images.go	Adds DockerImagesOptions.
pkg/cli/docker_images_test.go	Updates options-based tests.
pkg/cli/mcp_tools_readonly.go	Migrates the Docker-image caller.
pkg/cli/mcp_inspect_inspector.go	Stops timers per iteration.
pkg/cli/grant.go	Names the container policy path.
pkg/linters/stringbytesroundtrip/stringbytesroundtrip.go	Extracts conversion-pair analysis.

Review details

Tip

Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

• Files reviewed: 7/7 changed files
• Comments generated: 0
• Review effort level: Medium

[github-actions]

✅ PR Code Quality Reviewer completed the code quality review.

[github-actions]

Review: fix(lint) — 4 golint-custom findings

All changes are clean, mechanical refactors with no behavioural differences:

• defer-in-loop (mcp_inspect_inspector.go): timer.Stop() correctly moved after the select block — each iteration now cleans up immediately instead of deferring to function return.
• over-limit param count (docker_images.go): 9 bool params collapsed into DockerImagesOptions struct; all callers and tests updated consistently. No logic change.
• hardcoded temp path (grant.go): extracted as grantContainerPolicyPath constant — straightforward and correct.
• long functions (main.go, stringbytesroundtrip.go): extracted helpers preserve original logic faithfully; parseCompileFlags correctly propagates the --gh-aw-ref resolution error; unpackConversionPair returns the same early-exit conditions as the original inline code.

No security, correctness, or reliability issues found.

🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · sonnet46 · 34.5 AIC · ⌖ 4.56 AIC · ⊞ 5K

[github-actions]

Skills-Based Review 🧠

Applied /codebase-design — changes approved with no actionable issues.

📋 Key Themes & Highlights

What was reviewed

This is a targeted lint-compliance refactor across 7 files, resolving 4 golint-custom findings:

1. mcp_inspect_inspector.go — defer timer.Stop() correctly moved out of the loop to a per-iteration direct call. Fix is semantically correct.
2. docker_images.go — 9-parameter signature collapsed into DockerImagesOptions struct. All callers and test call sites updated. The struct uses exported boolean fields with safe zero-value defaults — idiomatic Go.
3. grant.go — bare /tmp/gh-aw-grant-policy.yaml string extracted to a named constant. Correct.
4. cmd/gh-aw/main.go — 447-line init() decomposed into 10 focused helpers; compileCmd.RunE extracted to runCompileCmd with compileFlags/parseCompileFlags/buildCompileConfig. The refactoring is faithful — flag names, mutual-exclusion constraints, --gh-aw-ref resolution order, and the --fix pre-pass are all preserved.
5. stringbytesroundtrip.go — unpackConversionPair correctly extracted; roundTripTypes struct cleanly replaces the inline variables. All early-return paths preserved.

Positive Highlights

• ✅ DockerImagesOptions zero-value is a safe default (verified by TestCheckAndPrepareDockerImages_NoToolsRequested)
• ✅ Tests updated 1-for-1 with the new struct signature — no coverage regression
• ✅ parseCompileFlags / buildCompileConfig separation makes the compile path unit-testable without a full Cobra context
• ✅ Changes are surgical: no unrelated behaviour changes detected

🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · sonnet46 · 31.7 AIC · ⌖ 7.48 AIC · ⊞ 6.7K
_{Comment /matt to run again}

[github-actions]

Design Decision Gate - ADR Required

This PR makes significant changes to core business logic (103 new lines in pkg/ directories) but does not have a linked Architecture Decision Record (ADR).

Draft ADR committed: docs/adr/47910-options-struct-pattern-for-multi-boolean-function-signatures.md -- review and complete it before merging.

This PR cannot merge until an ADR is linked in the PR body.

What to do next

1. Review the draft ADR committed to your branch at docs/adr/47910-options-struct-pattern-for-multi-boolean-function-signatures.md
2. Complete the missing sections -- add context the AI could not infer, refine the decision rationale, and list any additional alternatives you considered
3. Commit the finalized ADR to docs/adr/ on your branch
4. Reference the ADR in this PR body by adding a line such as:
   ADR: ADR-47910: Options Struct Pattern for Multi-Boolean Function Signatures

Once an ADR is linked in the PR body, this gate will re-run and verify the implementation matches the decision.

Why ADRs Matter

ADRs create a searchable, permanent record of why the codebase looks the way it does. Future contributors (and your future self) will thank you.

Michael Nygard ADR Format Reference

An ADR must contain these four sections to be considered complete:

• Context -- What is the problem? What forces are at play?
• Decision -- What did you decide? Why?
• Alternatives Considered -- What else could have been done?
• Consequences -- What are the trade-offs (positive and negative)?

All ADRs are stored in docs/adr/ as Markdown files numbered by PR number.

🏗️ ADR gate enforced by Design Decision Gate 🏗️ · sonnet46 · 91.8 AIC · ⌖ 9.92 AIC · ⊞ 8.5K · ◷
_{Comment /review to run again}

[github-actions]

🧪 Test Quality Sentinel Report

✅ Test Quality Score: N/A — Refactoring Only

No new or modified behavioral tests detected. This PR contains only parameter refactoring updates to existing test calls.

📊 Analysis Details

Test File Analyzed:

• pkg/cli/docker_images_test.go (697 lines, 27 total test functions)

Changes:

• Modified test functions: 11 (parameter refactoring only — updated CheckAndPrepareDockerImages() calls to use DockerImagesOptions{} struct)
• New test functions: 0
• Modified assertions: 0
• Lines added to test file: 0

Quality Signals:

Signal	Result
Build tag present	✅ Yes (//go:build !integration)
Forbidden mocking patterns	✅ None
Test inflation	✅ None (0 lines added)
Violations	✅ None

Test Functions Updated (parameter refactoring only):

1. TestCheckAndPrepareDockerImages_NoToolsRequested — DockerImagesOptions{}
2. TestCheckAndPrepareDockerImages_ImageAlreadyDownloading — DockerImagesOptions{Zizmor: true}
3. TestCheckAndPrepareDockerImages_MultipleImages — struct with multiple fields
4. TestCheckAndPrepareDockerImages_RetryMessageFormat — DockerImagesOptions{Zizmor: true}
5. TestCheckAndPrepareDockerImages_StartedDownloadingMessage — struct update
6. TestCheckAndPrepareDockerImages_ImageAlreadyAvailable — struct update
7. TestCheckAndPrepareDockerImages_DockerUnavailable — struct update
8. TestCheckAndPrepareDockerImages_DockerUnavailable_MultipleTools — struct with multiple fields
9. TestCheckAndPrepareDockerImages_DockerUnavailable_NoTools — DockerImagesOptions{}
10. TestCheckAndPrepareDockerImages_DockerUnavailable_ReturnsTypedError — struct update
11. TestCheckAndPrepareDockerImages_RunnerGuardImageDownloading — struct with multiple fields

Verdict

✅ passed. Refactoring-only PR with no new behavioral tests. All existing test assertions remain unchanged; no violations detected.

[github-actions]

Two issues require fixes before merge

The refactoring is generally clean and the individual fixes (defer-in-loop, option struct, named constant) are correct. Two problems need attention:

🔴 Blocking findings

1. Linter hot-path performance regression (pkg/linters/stringbytesroundtrip/stringbytesroundtrip.go): The cheap len(outer.Args) != 1 guard was moved inside unpackConversionPair, placed after the more expensive PositionFor (fileset lock) and ShouldSkipFilename (map lookup) calls. Every multi-argument CallExpr in analyzed codebases now pays this unnecessary cost. The guard must be restored before those calls in analyzeRoundTrip.

2. newCmd flag registration inconsistency (cmd/gh-aw/main.go): newCmd flags are registered inline in setupSetupGroupCmds, unlike compileCmd which has a dedicated registerCompileFlags() helper. Since newCmd is a package-level var, any double-invocation of the setup functions (test harnesses, reset-and-reinit patterns) will panic with flag redefined. Extract registerNewCmdFlags() to match the established pattern.

🔎 Code quality review by PR Code Quality Reviewer · sonnet46 · 74.4 AIC · ⌖ 5.02 AIC · ⊞ 5.7K
_{Comment /review to run again}

Comments that could not be inline-anchored

pkg/linters/stringbytesroundtrip/stringbytesroundtrip.go:1302

Performance regression in linter hot-path: the cheap arg-count guard was moved behind two more expensive operations, causing unnecessary work on every multi-argument call expression.

<details>
<summary>💡 Details and fix</summary>

In the original analyzeRoundTrip, the first check was:

if len(outer.Args) != 1 || outer.Ellipsis.IsValid() {
    return
}

That guard fired before pass.Fset.PositionFor (acquires fileset lock) and filecheck.ShouldSkipFilename (map lookup). After …

cmd/gh-aw/main.go:657

newCmd flag registration inside setupSetupGroupCmds is inconsistent with registerCompileFlags() pattern: if setupSetupGroupCmds is called more than once (e.g., in tests), Cobra panics with flag redefined.

<details>
<summary>💡 Details and fix</summary>

The PR correctly extracted registerCompileFlags() as a standalone helper to isolate compileCmd flag registration. But newCmd flags are still registered inline inside setupSetupGroupCmds:

func setupSetupGroupCmds() {…

</details>

[gh-aw-bot]

@copilot please run the pr-finisher skill, address the unresolved blocking review feedback, complete the ADR link/body follow-up if still needed, and rerun checks once the branch is up to date. Priority review items:

• github-actions review summary on PR fix(lint): resolve 4 targeted golint-custom findings in pkg/cli, cmd/gh-aw, and pkg/linters #47910: fix(lint): resolve 4 targeted golint-custom findings in pkg/cli, cmd/gh-aw, and pkg/linters #47910 (comment)
• blocking review says analyzeRoundTrip regressed hot-path performance by moving the arg-count guard too late, and newCmd flag registration should be extracted to avoid flag redefined panics on repeated setup.

Generated by 👨‍🍳 PR Sous Chef · gpt54 · 17.6 AIC · ⌖ 8.18 AIC · ⊞ 7.1K · ◷
_{Comment /souschef to run again}

[Copilot]

@copilot please run the pr-finisher skill, address the unresolved blocking review feedback, complete the ADR link/body follow-up if still needed, and rerun checks once the branch is up to date. Prio...

Addressed both blocking items in commit fix: restore analyzeRoundTrip arg-count guard; extract registerNewCmdFlags/registerRemoveCmdFlags:

1. analyzeRoundTrip hot-path regression — restored the cheap len(outer.Args) != 1 || outer.Ellipsis.IsValid() guard to the top of analyzeRoundTrip, before the expensive filecheck.ShouldSkipFilename and nolint.HasDirectiveForLinter calls. The unpackConversionPair helper retains its own defensive check, but the early exit now fires on the hot path as it did before the refactor.

2. newCmd/removeCmd flag registration — extracted registerNewCmdFlags() and registerRemoveCmdFlags() dedicated helpers (matching the existing registerCompileFlags() pattern), and replaced the inline registrations inside setupSetupGroupCmds() with calls to these functions.

Local validation: make fmt, make lint, make test-unit all pass.

[github-actions]

🤖 PR Triage

Field	Value
Category	bug
Risk	medium
Score	58 / 100
Score breakdown	impact 38 + urgency 12 + quality 8
Action	fast_track

Rationale: Fixes 4 targeted lint findings (defer-in-loop, over-limit params, hard-coded temp path, long functions) across pkg/cli, cmd/gh-aw, and pkg/linters. Correctness bugs + linter hygiene. Latest review is CHANGES_REQUESTED — resolve before merge.

Next step: Address CHANGES_REQUESTED, then expedite review.

Generated by 🔧 PR Triage Agent · sonnet46 · 44.5 AIC · ⌖ 5.13 AIC · ⊞ 5.7K · ◷

Done

[pelikhan]

@copilot resolve the merge conflicts on this branch.

[gh-aw-bot]

@copilot please run the pr-finisher skill, address any unresolved review feedback, fix any failing checks listed below, and rerun checks once the branch is up to date.

Generated by 👨‍🍳 PR Sous Chef · gpt54 · 12.3 AIC · ⌖ 8.43 AIC · ⊞ 7.1K · ◷
_{Comment /souschef to run again}

[Copilot]

@copilot resolve the merge conflicts on this branch.

Merge conflicts resolved. stringbytesroundtrip.go had a content conflict where main updated the []byte(string(b)) diagnostic and added the defensive-copy comment while our branch had refactored the same block to use the roundTripTypes struct. The resolution keeps our unpackConversionPair/roundTripTypes refactor and rtt.* field accesses, and incorporates main's defensive-copy comment and updated diagnostic message. make fmt, make lint, and make test-unit all pass.

[Copilot]

@copilot please run the pr-finisher skill, address any unresolved review feedback, fix any failing checks listed below, and rerun checks once the branch is up to date.

...

Branch is up to date with main (commit a7da7ae). All local validation passes (make fmt, make lint, make test-unit). The three CI runs showing action_required (CGO, CWI, Doc Build - Deploy) require human approval to run — they have no failed jobs. The remaining make golint-custom findings are from the stringsconcatloop linter (added to main in #47894) in files this PR never touched; they are pre-existing in main and out of scope. CI must be re-triggered by a maintainer before merge.