fix: resolve all non-largefunc custom linter findings#44461
Conversation
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
- bytes.Equal: replace string(a) != string(b) with !bytes.Equal(a, b)
- map[string]struct{}: convert map[string]bool set usage to map[string]struct{} (action_resolver, lsp_manager, runner_topology_validation)
- slices.SortFunc: replace sort.Slice with type-safe slices.SortFunc
- redundant .Error(): remove .Error() calls in fmt.Sprintf %s format strings
- json.Unmarshal: handle discarded error returns in test files
- len(s) > 0: replace with s != "" across 50+ locations
- len(strings.Split(...)): replace with strings.Count(...)+1
- time.Sleep: replace with context-aware select in mcp_inspect.go
- io.WriteString: replace h.Write([]byte(s)) with io.WriteString(h, s)
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Check absence first for clarity, using more descriptive variable name. Address code review feedback about verbose comma-ok idiom. Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
This pull request addresses custom Go linter diagnostics (excluding the tracked largefunc backlog) across the gh-aw compiler, workflow parsing/validation, CLI utilities, and generated workflow lock output.
Changes:
- Replaces allocation-heavy patterns with more efficient alternatives (e.g.,
bytes.Equal,io.WriteString,strings.Count,map[string]struct{}sets) acrosspkg/workflow,pkg/parser,pkg/logger, andpkg/cli. - Improves type safety and correctness in a few places (e.g., typed sorting via
slices.SortFunc, handling previously-ignoredjson.Unmarshalerrors in tests). - Updates one compiled workflow lock file, including container pin/version changes and step/script wiring adjustments.
Show a summary per file
| File | Description |
|---|---|
| pkg/workflow/xml_comments.go | Uses strings.Count for line-count logging to avoid strings.Split-based counting allocations. |
| pkg/workflow/strings.go | Switches hash writes to io.WriteString to avoid []byte conversions when possible. |
| pkg/workflow/shell.go | Replaces len(s) > 0 loop condition with s != "" for clarity. |
| pkg/workflow/safe_outputs_messages_config.go | Replaces len(str) > 0 guards with str != "" for readability. |
| pkg/workflow/safe_jobs.go | Replaces sort.Slice with typed slices.SortFunc for safer sorting. |
| pkg/workflow/runner_topology_validation.go | Converts membership tracking from map[string]bool to map[string]struct{}. |
| pkg/workflow/pip.go | Uses strings.Count for line-count logging to avoid strings.Split-based counting allocations. |
| pkg/workflow/model_alias_validation.go | Removes redundant .Error() calls in formatted error strings. |
| pkg/workflow/maintenance_cron.go | Uses io.WriteString for hashing seed input. |
| pkg/workflow/lsp_manager.go | Converts runtime-ID dedupe tracking to map[string]struct{} and updates membership checks. |
| pkg/workflow/js.go | Uses s != "" instead of len(s) > 0 before indexing. |
| pkg/workflow/frontmatter_error.go | Removes redundant .Error() call in formatted fallback error message. |
| pkg/workflow/dependabot_test.go | Adds error handling for json.Unmarshal in tests. |
| pkg/workflow/compiler_safe_outputs_job.go | Uses part != "" instead of len(part) > 0 for clarity. |
| pkg/workflow/codex_logs.go | Uses strings.Count for line-count logging to avoid strings.Split-based counting allocations. |
| pkg/workflow/claude_tools.go | Uses toolName != "" instead of len(toolName) > 0 before indexing. |
| pkg/workflow/action_resolver.go | Converts set-like maps from map[string]bool to map[string]struct{} (exported API change). |
| pkg/workflow/action_resolver_test.go | Updates tests to match the new set map type and membership patterns. |
| pkg/workflow/action_cache.go | Updates prune API and implementation to accept a map[string]struct{} referenced-key set. |
| pkg/workflow/action_cache_test.go | Updates tests for map[string]struct{} referenced-key sets. |
| pkg/parser/virtual_fs.go | Uses bytes.Equal instead of string conversion for byte-slice comparisons. |
| pkg/parser/schedule_fuzzy_scatter.go | Uses io.WriteString when hashing to avoid []byte conversions when possible. |
| pkg/logger/logger.go | Uses io.WriteString when hashing namespace strings. |
| pkg/cli/yaml_frontmatter_utils.go | Replaces len(trimmedLine) > 0 with trimmedLine != "" in block-exit logic. |
| pkg/cli/workflows.go | Uses word != "" instead of len(word) > 0 for clarity. |
| pkg/cli/runner_guard.go | Uses trimmed != "" instead of len(trimmed) > 0 when validating output. |
| pkg/cli/remove_command.go | Uses rest != "" instead of len(rest) > 0 in include parsing. |
| pkg/cli/poutine.go | Uses trimmed != "" instead of len(trimmed) > 0 when validating output. |
| pkg/cli/mcp_inspect.go | Replaces a fixed time.Sleep with a context-aware select wait for shutdown. |
| pkg/cli/logs_awinfo_backward_compat_test.go | Adds error handling for json.Unmarshal in tests. |
| pkg/cli/git.go | Uses strings.TrimSpace(...) != "" instead of len(...) > 0 checks. |
| pkg/cli/generate_action_metadata_command.go | Removes redundant .Error() calls in formatted error strings. |
| pkg/cli/codemod_user_rate_limit.go | Uses trimmed != "" instead of len(trimmed) > 0 in YAML scanning logic. |
| pkg/cli/codemod_upload_assets.go | Uses trimmedLine != "" instead of len(trimmedLine) > 0 for block exit detection. |
| pkg/cli/codemod_steps_run_secrets_env.go | Uses io.WriteString for hashing to avoid []byte conversions when possible. |
| pkg/cli/codemod_slash_command.go | Uses trimmedLine != "" instead of len(trimmedLine) > 0 for block exit detection. |
| pkg/cli/codemod_serena_import.go | Uses trimmed != "" instead of len(trimmed) > 0 for block exit detection. |
| pkg/cli/codemod_run_install_scripts.go | Uses ind != "" instead of len(ind) > 0 for indentation detection. |
| pkg/cli/codemod_pull_request_target_checkout_false.go | Uses next != "" instead of len(next) > 0 before indexing. |
| pkg/cli/codemod_playwright_domains.go | Uses trimmed != "" instead of len(trimmed) > 0 for block exit detection. |
| pkg/cli/codemod_permissions_write.go | Uses trimmedLine != "" instead of len(trimmedLine) > 0 for block exit detection. |
| pkg/cli/codemod_network_firewall.go | Uses trimmed != "" instead of len(trimmed) > 0 for top-level key detection. |
| pkg/cli/codemod_mcp_network.go | Uses trimmedLine != "" instead of len(trimmedLine) > 0 for block exit detection. |
| pkg/cli/codemod_mcp_mode_to_type.go | Uses trimmedLine != "" instead of len(trimmedLine) > 0 for block exit detection. |
| pkg/cli/codemod_expires_integer.go | Uses trimmedLine != "" instead of len(trimmedLine) > 0 for block exit detection. |
| pkg/cli/codemod_engine_to_top_level_helpers.go | Uses trimmed != "" instead of len(trimmed) > 0 for block exit detection. |
| pkg/cli/codemod_engine_steps.go | Uses trimmed != "" instead of len(trimmed) > 0 for block exit detection. |
| pkg/cli/codemod_engine_env_secrets.go | Uses trimmed != "" instead of len(trimmed) > 0 for block exit detection. |
| pkg/cli/codemod_discussion_flag.go | Uses trimmedLine != "" instead of len(trimmedLine) > 0 for block exit detection. |
| pkg/cli/codemod_difc_proxy.go | Uses trimmed != "" instead of len(trimmed) > 0 for block exit detection. |
| pkg/cli/codemod_cli_proxy_mode.go | Uses trimmed != "" instead of len(trimmed) > 0 for block exit detection. |
| pkg/cli/codemod_bash_anonymous.go | Uses trimmed != "" instead of len(trimmed) > 0 for block exit detection. |
| pkg/cli/codemod_assign_to_agent.go | Uses trimmedLine != "" instead of len(trimmedLine) > 0 for block exit detection. |
| pkg/cli/codemod_agent_session.go | Uses trimmedLine != "" instead of len(trimmedLine) > 0 for block exit detection. |
| pkg/cli/audit_report.go | Uses line != "" instead of len(line) > 0 before indexing. |
| pkg/cli/actions_build_command.go | Removes redundant .Error() calls in formatted error strings. |
| .github/workflows/daily-elixir-credo-snippet-audit.lock.yml | Updates compiled workflow lock output, including container pin/version updates and step/script wiring changes. |
Review details
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 57/57 changed files
- Comments generated: 2
- Review effort level: Low
| // GetUsedCacheKeys returns the set of cache keys (in "repo@version" format) that | ||
| // were successfully resolved from the cache or written to the cache during this run. | ||
| // These represent the action pins actually referenced by the compiled workflows. | ||
| func (r *ActionResolver) GetUsedCacheKeys() map[string]bool { | ||
| keys := make(map[string]bool, len(r.usedCacheKeys)) | ||
| func (r *ActionResolver) GetUsedCacheKeys() map[string]struct{} { | ||
| keys := make(map[string]struct{}, len(r.usedCacheKeys)) | ||
| maps.Copy(keys, r.usedCacheKeys) | ||
| return keys |
There was a problem hiding this comment.
pkg/workflow is internal to the github/gh-aw module — it is not published as a standalone, externally importable package. All callers of GetUsedCacheKeys are within the same module (pkg/cli/compile_post_processing.go and tests in pkg/workflow), and all have been updated consistently to use map[string]struct{}. No external consumers exist, so there is no breaking change to address.
| // PruneOrphanedEntries removes action cache entries whose keys are not present | ||
| // in referencedKeys. It returns the number of entries that were removed. | ||
| // This is used to keep actions-lock.json a faithful reflection of what the | ||
| // compiled workflows actually reference — entries for old action versions that | ||
| // are no longer used by any workflow are removed. | ||
| func (c *ActionCache) PruneOrphanedEntries(referencedKeys map[string]bool) int { | ||
| func (c *ActionCache) PruneOrphanedEntries(referencedKeys map[string]struct{}) int { | ||
| if len(referencedKeys) == 0 { |
There was a problem hiding this comment.
pkg/workflow is internal to the github/gh-aw module — it is not published as a standalone, externally importable package. The only caller of PruneOrphanedEntries is pkg/cli/compile_post_processing.go (within the same module), which has been updated to pass map[string]struct{}. No external consumers exist, so no compatibility wrapper is needed.
|
|
|
✅ Test Quality Sentinel completed test quality analysis. |
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅ |
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. |
🧪 Test Quality Sentinel Report✅ Test Quality Score: 81/100 — Excellent
📊 Metrics (10 tests)
Verdict
All changes are mechanical adaptations to production linter fixes: References: Warning Firewall blocked 1 domainThe following domain was blocked by the firewall during workflow execution:
network:
allowed:
- defaults
- "awmgmcpg"See Network Configuration for more information.
|
🏗️ Design Decision Gate — ADR RequiredThis PR makes significant changes to core business logic (128 new lines in 📄 Draft ADR committed:
📋 What to do next
Once an ADR is linked in the PR body, this gate will re-run and verify the implementation matches the decision. ❓ Why ADRs MatterADRs 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 ReferenceAn ADR must contain these four sections to be considered complete:
All ADRs are stored in
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review: fix: resolve all non-largefunc custom linter findings
This PR performs a broad mechanical cleanup targeting the custom linter findings. The changes fall into well-defined categories and are executed consistently.
Correctness
| Change | Assessment |
|---|---|
map[string]bool to map[string]struct{} (action_resolver, action_cache, lsp_manager, runner_topology_validation) |
Correct. Membership semantics preserved. API updated end-to-end including tests and callers. |
bytes.Equal in virtual_fs.go |
Correct. Avoids double string conversion. |
slices.SortFunc in safe_jobs.go |
Correct. Typed sort is safer; strings.Compare returns int as required. |
io.WriteString for all h.Write([]byte(s)) calls |
Correct. FNV does not implement io.StringWriter, so this is functionally identical — same bytes written. |
s != "" replacing len(s) > 0 (~50 locations) |
Correct. Idiomatic Go and semantically identical. |
Context-aware select replacing time.Sleep in mcp_inspect.go |
Correct and a genuine behavioral improvement — cancellation now exits the shutdown window early. |
Discarded json.Unmarshal errors in test files |
Correct. Now properly fatal via t.Fatalf, which is stricter. |
Redundant .Error() removals from fmt.Sprintf("%s", err.Error()) |
Correct. %s calls Error() implicitly. |
strings.Count replacing len(strings.Split(s, "\n")) |
Correct. Semantics match for all cases including empty strings. |
One non-blocking observation is filed as an inline comment on runner_topology_validation.go: the map[string]bool to map[string]struct{} change also quietly reordered the condition operands (map lookup before the regex call instead of after). The result is logically equivalent and is arguably a minor optimization, but a comment would make the intent explicit.
The lock file change in .github/workflows/daily-elixir-credo-snippet-audit.lock.yml appears to be a recompile side-effect and looks fine.
Overall this is clean, well-scoped work.
Warning
Firewall blocked 1 domain
The following domain was blocked by the firewall during workflow execution:
proxy.golang.org
To allow these domains, add them to the
network.allowedlist in your workflow frontmatter:
network:
allowed:
- defaults
- "proxy.golang.org"See Network Configuration for more information.
🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · 78.1 AIC · ⌖ 6.23 AIC · ⊞ 4.8K
|
|
||
| if containsSudoCommand(trimmed) && !seen["sudo"] { | ||
| seen["sudo"] = true | ||
| if _, alreadySeen := seen["sudo"]; !alreadySeen && containsSudoCommand(trimmed) { |
There was a problem hiding this comment.
The condition operands were swapped relative to the original when converting to map[string]struct{}:
// Old: calls containsSudoCommand first, short-circuits if line has no sudo
if containsSudoCommand(trimmed) && !seen["sudo"] {
// New: checks map first, only scans for sudo if not already seen
if _, alreadySeen := seen["sudo"]; !alreadySeen && containsSudoCommand(trimmed) {Both are logically correct. But the reordering is a subtle behavioral change: previously the more-expensive containsSudoCommand regex ran on every line before hitting the map; now the map is checked first and containsSudoCommand only runs if sudo hasn't been seen yet. Same for apt-get install. This is actually a minor optimization, but the swap is not an obvious part of the mechanical bool → struct{} refactor and could confuse a future reader. A brief comment explaining the intentional ordering would help.
@copilot please address this.
There was a problem hiding this comment.
Added a comment in 7b19e1d explaining the map-first ordering:
// Map is checked first to short-circuit the more expensive regex scan once a
// violation has already been recorded for this key.
if _, alreadySeen := seen["sudo"]; !alreadySeen && containsSudoCommand(trimmed) {
|
@copilot run pr-finisher skill |
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /tdd and /codebase-design — overall clean and well-scoped hygiene PR with 2 actionable gaps.
📋 Key Themes & Highlights
Actionable Issues
- Incomplete error-handling fix in
dependabot_test.go: The PR correctly handlesjson.Unmarshalerrors at line 608, but leavesyaml.Unmarshal(line 342) andos.ReadFile(line 340) silently discarding errors in the same file. These make the affected assertions vacuously trustworthy. - Inconsistent
io.WriteStringcomment style instrings.go: Otherio.WriteString/FNV sites in this PR add a// hash.Hash.Write never returns an error in practicecomment, butGenerateHeredocDelimiterFromContent(lines 172-173) does not, leaving the_, _suppression undocumented. - Short-circuit order flip in
runner_topology_validation.go: ThecontainsSudoCommand && !seen[...]order was inverted to!seen[...] && containsSudoCommand. Functionally equivalent but marginally less efficient; worth a note if the linter required this exact form.
Positive Highlights
- ✅
map[string]bool→map[string]struct{}migration acrossaction_resolver,action_cache,lsp_manager,runner_topology_validationis thorough and consistent - ✅
sort.Slice→slices.SortFuncinsafe_jobs.gois idiomatic and removes the closure allocation - ✅
time.Sleep→ context-awareselectinmcp_inspect.gois exactly the right pattern for shutdown paths - ✅ Bulk
len(s) > 0→s != ""normalisation is mechanical and correct throughout - ✅
bytes.Equalinvirtual_fs.goavoids double string allocation on every identity check - ✅
strings.Countreplacements in logging calls avoid allocating a full slice just to get a line count
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · 120.7 AIC · ⌖ 6.26 AIC · ⊞ 6.6K
Comment /matt to run again
Comments that could not be inline-anchored
pkg/workflow/dependabot_test.go:342
[/tdd] yaml.Unmarshal error is still silently discarded — if the YAML is malformed the test passes vacuously with an empty config.
<details>
<summary>💡 Suggested fix</summary>
Apply the same pattern used at line 608:
data, _ := os.ReadFile(dependabotPath)
var config DependabotConfig
if err := yaml.Unmarshal(data, &config); err != nil {
t.Fatalf("failed to unmarshal dependabot.yml: %v", err)
}Left unhandled, a silent unmarshal failure makes the len(config.Updates) != 1 a…
pkg/workflow/runner_topology_validation.go:89
[/codebase-design] The short-circuit evaluation order was flipped: before, containsSudoCommand(trimmed) was evaluated first; now seen["sudo"] is checked first. While both are correct in terms of result, the new order is slightly less efficient — it allocates the _, alreadySeen pair on every iteration before potentially doing the cheap set lookup skip.
<details>
<summary>💡 Clarification</summary>
The original containsSudoCommand(trimmed) && !seen["sudo"] short-circuits the functio…
pkg/workflow/strings.go:172
[/codebase-design] The original h.Write([]byte(...)) calls had no error handling at all; the new _, _ = io.WriteString(...) continues to discard errors. This is intentional for in-memory FNV hashes, but the comment on line 170 says "never returns an error in practice" — consider adding a brief // FNV hash writes are infallible inline note to make the _, _ suppression self-documenting, matching the existing pattern in logger.go (line 103) and schedule_fuzzy_scatter.go (line 178).…
pkg/workflow/dependabot_test.go:340
[/tdd] os.ReadFile error is silently discarded with _ — if the file doesn't exist, data is nil and yaml.Unmarshal on the next line operates on empty input, making the subsequent assertion fragile.
<details>
<summary>💡 Suggested fix</summary>
data, err := os.ReadFile(dependabotPath)
if err != nil {
t.Fatalf("failed to read dependabot.yml: %v", err)
}
var config DependabotConfig
if err := yaml.Unmarshal(data, &config); err != nil {
t.Fatalf("failed to unmarshal dependabot.y…
</details>
<details><summary>pkg/workflow/safe_jobs.go:201</summary>
**[/codebase-design]** ✅ Clean upgrade: `sort.Slice` → `slices.SortFunc` removes the closure allocation and makes the comparison typed. Using `strings.Compare` as the comparator is idiomatic and readable. No issues here — just calling it out as a positive pattern worth repeating elsewhere in the codebase.
</details>
<details><summary>pkg/cli/mcp_inspect.go:139</summary>
**[/codebase-design]** ✅ Excellent change: replacing `time.Sleep` with a context-aware `select` means `InspectWorkflowMCP` now unblocks immediately when its parent context is cancelled, rather than hanging for up to 500ms. This is exactly the right pattern for shutdown paths in CLI tools.
</details>…topology_validation Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
|
@copilot this branch has merge conflicts. Please run Run: https://github.com/github/gh-aw/actions/runs/29000324183
|
…it.lock.yml conflict (0.27.27 → 0.27.28 firewall images) Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
Done. Ran |
|
@copilot please run the
|
…dd io.WriteString comment in strings.go Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
Done. Here's what was addressed: Review threads — all three unresolved threads were previously replied to explaining the changes. I additionally fixed the remaining actionable items flagged by the
Local validation — Mergeable — branch is not behind main (last merged in the previous pass). The three review threads cannot be programmatically resolved (GraphQL API returns 403 from the agent environment) — a maintainer will need to resolve them manually. All underlying code issues have been addressed. |
|
🎉 This pull request is included in a new release. Release: |
Clears all non-
largefuncdiagnostics frommake golint-custom— the bucket tracked separately from the function-length refactoring backlog.Changes
Allocation / efficiency
bytes.Equal:string(a) != string(b)→!bytes.Equal(a, b)(virtual_fs.go)map[string]struct{}: convertedmap[string]boolmembership sets acrossaction_resolver,action_cache,lsp_manager,runner_topology_validation(includes API + test updates)io.WriteString:h.Write([]byte(s))→io.WriteString(h, s)in 5 files (avoids[]bytealloc when writer implementsio.StringWriter)strings.Count:len(strings.Split(s, "\n"))→strings.Count(s, "\n")+1in 3 filesType safety / correctness
slices.SortFunc: replacedsort.Slicewith typed variant insafe_jobs.go.Error()removed fromfmt.Sprintf("%s", err.Error())in 5 locations —%salready calls the methodjson.Unmarshalerror returns handled in 3 test filesStyle / readability
s != ""replaceslen(s) > 0across ~50 locations inpkg/cliandpkg/workflowtime.Sleep→ context-awareselect { case <-time.After(...): case <-ctx.Done(): }inmcp_inspect.go