lint: fix 8 targeted golint categories from lint-monster scan#43126
Conversation
…p[string]bool, redundant .Error(), len(s)>0, strings.Split, sort.Slice, time.Sleep) Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
|
✅ Test Quality Sentinel completed test quality analysis. |
|
✅ PR Code Quality Reviewer completed the code quality review. |
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅ |
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. |
There was a problem hiding this comment.
Pull request overview
Targets a batch of small, behavior-preserving refactors to satisfy a lint-monster scan across several focused Go lint categories, with a notable API-level adjustment in the workflow action cache/resolver “set” types.
Changes:
- Replaced several string-emptiness and “line count” anti-patterns (
len(s)>0,len(strings.Split(...))) with clearer/cheaper equivalents (s != "",strings.Count(...)+1). - Updated safe-outputs/action-cache plumbing to use
map[string]struct{}as a set, including exported method/parameter types and corresponding tests. - Improved MCP inspect shutdown/readiness paths (helper extraction for HTTP probes, and
Sleep→selectfor ctx-aware waiting) and tightened test unmarshalling error handling.
Show a summary per file
| File | Description |
|---|---|
| pkg/workflow/xml_comments.go | Avoids len(strings.Split(...)) allocation for line-count logging. |
| pkg/workflow/shell.go | Replaces len(s)>0 loop condition with s != "". |
| pkg/workflow/safe_outputs_messages_config.go | Uses str != "" guard before indexing str[0]. |
| pkg/workflow/safe_jobs.go | Switches sort.Slice to slices.SortFunc for entry ordering. |
| pkg/workflow/runner_topology_validation.go | Converts local “seen” set to map[string]struct{}. |
| pkg/workflow/pip.go | Avoids len(strings.Split(...)) allocation for line-count logging. |
| pkg/workflow/model_alias_validation.go | Removes redundant .Error() in formatted error strings. |
| pkg/workflow/lsp_manager.go | Converts local “seen” set to map[string]struct{}. |
| pkg/workflow/js.go | Uses resultStr != "" guard before indexing the last byte. |
| pkg/workflow/frontmatter_error.go | Removes redundant .Error() when formatting error text. |
| pkg/workflow/dependabot_test.go | Stops discarding json.Unmarshal errors in tests. |
| pkg/workflow/compiler_safe_outputs_job.go | Replaces len(part)>0 with part != "" when building handler names. |
| pkg/workflow/codex_logs.go | Avoids len(strings.Split(...)) allocation for line-count logging. |
| pkg/workflow/claude_tools.go | Uses toolName != "" guard before indexing toolName[0]. |
| pkg/workflow/action_resolver.go | Converts resolver “sets” to map[string]struct{} and updates exported API. |
| pkg/workflow/action_resolver_test.go | Updates tests for map[string]struct{}-based sets. |
| pkg/workflow/action_cache.go | Updates prune API to accept map[string]struct{} referenced-key set. |
| pkg/workflow/action_cache_test.go | Updates pruning tests for map[string]struct{} referenced-key sets. |
| pkg/cli/yaml_frontmatter_utils.go | Replaces len(trimmedLine)>0 with trimmedLine != \"\" in block-exit logic. |
| pkg/cli/workflows.go | Replaces len(word)>0 with word != \"\" during title casing. |
| pkg/cli/runner_guard.go | Replaces len(trimmed)>0 with trimmed != \"\" for non-JSON output handling. |
| pkg/cli/remove_command.go | Replaces len(rest)>0 with rest != \"\" before indexing. |
| pkg/cli/poutine.go | Replaces len(trimmed)>0 with trimmed != \"\" for non-JSON output handling. |
| pkg/cli/mcp_inspect.go | Replaces blocking sleep with ctx-aware select during shutdown. |
| pkg/cli/mcp_inspect_mcp_scripts_server.go | Extracts readiness probe helper to avoid defer in loop + ensure body close. |
| pkg/cli/logs_awinfo_backward_compat_test.go | Stops discarding json.Unmarshal errors in tests. |
| pkg/cli/git.go | Replaces len(strings.TrimSpace(...))>0 with != \"\" checks. |
| pkg/cli/generate_action_metadata_command.go | Removes redundant .Error() and replaces len(word)>0 checks. |
| pkg/cli/codemod_user_rate_limit.go | Replaces len(trimmed)>0 with trimmed != \"\" in indentation detection. |
| pkg/cli/codemod_upload_assets.go | Replaces len(trimmedLine)>0 with trimmedLine != \"\" in block-exit logic. |
| pkg/cli/codemod_slash_command.go | Replaces len(trimmedLine)>0 with trimmedLine != \"\" in block-exit logic. |
| pkg/cli/codemod_serena_import.go | Replaces len(trimmed)>0 with trimmed != \"\" in block-exit logic. |
| pkg/cli/codemod_run_install_scripts.go | Replaces len(ind)>0 with ind != \"\" indentation detection. |
| pkg/cli/codemod_pull_request_target_checkout_false.go | Replaces len(next)>0 with next != \"\" before indexing. |
| pkg/cli/codemod_playwright_domains.go | Replaces len(trimmed)>0 with trimmed != \"\" in block-exit logic. |
| pkg/cli/codemod_permissions_write.go | Replaces len(trimmedLine)>0 with trimmedLine != \"\" in block-exit logic. |
| pkg/cli/codemod_network_firewall.go | Replaces len(trimmed)>0 with trimmed != \"\" in top-level key detection. |
| pkg/cli/codemod_mcp_network.go | Replaces len(trimmedLine)>0 with trimmedLine != \"\" in block-exit logic. |
| pkg/cli/codemod_mcp_mode_to_type.go | Replaces len(trimmedLine)>0 with trimmedLine != \"\" in block-exit logic. |
| pkg/cli/codemod_expires_integer.go | Replaces len(trimmedLine)>0 with trimmedLine != \"\" in block-exit logic. |
| pkg/cli/codemod_engine_to_top_level_helpers.go | Replaces len(trimmed)>0 with trimmed != \"\" in block-exit logic. |
| pkg/cli/codemod_engine_steps.go | Replaces len(trimmed)>0 with trimmed != \"\" in block-exit logic. |
| pkg/cli/codemod_engine_env_secrets.go | Replaces len(trimmed)>0 with trimmed != \"\" in block-exit logic. |
| pkg/cli/codemod_discussion_flag.go | Replaces len(trimmedLine)>0 with trimmedLine != \"\" in block-exit logic. |
| pkg/cli/codemod_difc_proxy.go | Replaces len(trimmed)>0 with trimmed != \"\" in block-exit logic. |
| pkg/cli/codemod_cli_proxy_mode.go | Replaces len(trimmed)>0 with trimmed != \"\" in insertion scanning logic. |
| pkg/cli/codemod_bash_anonymous.go | Replaces len(trimmed)>0 with trimmed != \"\" in block-exit logic. |
| pkg/cli/codemod_assign_to_agent.go | Replaces len(trimmedLine)>0 with trimmedLine != \"\" in block-exit logic. |
| pkg/cli/codemod_agent_session.go | Replaces len(trimmedLine)>0 with trimmedLine != \"\" in block-exit logic. |
| pkg/cli/audit_report.go | Replaces len(line)>0 with line != \"\" before indexing. |
| pkg/cli/actions_build_command.go | Removes redundant .Error() in formatted error strings. |
Review details
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 51/51 changed files
- Comments generated: 2
- Review effort level: Low
| select { | ||
| case <-time.After(mcpScriptsServerShutdownDelay): | ||
| case <-ctx.Done(): | ||
| } |
| data, _ := os.ReadFile(packageJSONPath) | ||
| var pkgJSON PackageJSON | ||
| json.Unmarshal(data, &pkgJSON) | ||
| if err := json.Unmarshal(data, &pkgJSON); err != nil { | ||
| t.Fatalf("Failed to unmarshal package.json: %v", err) | ||
| } |
🏗️ Design Decision Gate — ADR RequiredThis PR makes significant changes to core business logic (137 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 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 ReferenceAn ADR must contain these four sections to be considered complete:
All ADRs are stored in
|
🧪 Test Quality Sentinel Report
📊 Metrics (4 modified test functions)
ContextAll changes in this PR are mechanical lint fixes — not new tests. The four changed test files have only minimal, targeted edits:
No new test functions were added; no test logic was changed. All changes improve test correctness by surfacing previously-swallowed errors. Verdict
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.
|
There was a problem hiding this comment.
Review: lint-monster targeted cleanup ✅
Clean, well-scoped lint fixes. All 8 categories are addressed correctly. Changes are safe mechanical improvements with no functional regressions.
Highlights:
probeServerhelper is a good extraction — correctly scopesdefer resp.Body.Close()and the//nolint:nilerrcomment is appropriate for probe semanticsselect { case <-time.After(...): case <-ctx.Done(): }is the right pattern for interruptible sleepsmap[string]struct{}migration is complete and consistent — struct fields, public API signatures, and all call sites updated in sync with test filesslices.SortFunc+strings.Compareavoids thecmpvariable name collision thoughtfullystrings.Count(s, "\n")+1is the idiomatic line-count replacement- Removing
.Error()from%sformat args is correct (theerrorinterface already formats viaError()when used with%s/%v)
One non-blocking finding (inline): dependabot_test.go line 605 still discards the os.ReadFile error — the json.Unmarshal guard was added but data, _ := os.ReadFile(...) on the preceding line wasn't updated.
🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · 49.6 AIC · ⌖ 6.12 AIC · ⊞ 4.8K
| @@ -605,7 +605,9 @@ func TestGenerateDependabotManifests_WithDependencies(t *testing.T) { | |||
| // Verify package.json content | |||
There was a problem hiding this comment.
The json.Unmarshal error is now correctly handled, but os.ReadFile on the preceding line still discards its error with _. If the file is missing or unreadable, data will be empty and the unmarshal will fail with a confusing unexpected end of JSON input rather than a clear file-not-found message. Consider:
data, err := os.ReadFile(packageJSONPath)
if err != nil {
t.Fatalf("failed to read package.json: %v", err)
}@copilot please address this.
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /codebase-design and /tdd — no blocking correctness issues; leaving as COMMENT with a few targeted observations.
📋 Key Themes & Highlights
Key Themes
- Stale log message (
mcp_inspect_mcp_scripts_server.goline 85):probeServerextraction moved the request-creation logic inside the helper, but the error log at the call site still says"Failed to create request"— should be updated to match the helper's abstraction. - Condition readability (
runner_topology_validation.golines 89, 93): the comma-ok lookup is in theifinit position but the branch condition tests an unrelated predicate — unconventional enough to trip a future reader. - Incomplete error hygiene in tests (
dependabot_test.go):json.Unmarshalerrors are now correctly checked (great!), butos.ReadFileandyaml.Unmarshalon nearby lines still use blank identifiers, leaving silent failure paths. map[string]struct{}API readability: the public API change is correct and memory-efficient, but a named type alias (ActionKeySet) would signal set semantics to readers without adding overhead.
Positive Highlights
- ✅ Clean
probeServerextraction — scopesdefer resp.Body.Close()correctly and simplifieswaitForServerReady. - ✅
select-based shutdown inmcp_inspect.go— proper context propagation, correct behaviour on forced cancellation. - ✅ Full public API chain updated consistently: struct fields, method signatures, all test files.
- ✅
slices.SortFunc+strings.Compareavoids thecmpnaming collision noted in the PR description — good defensive naming. - ✅
strings.Count(...)+1is a clear improvement over allocating a slice just tolenit.
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · 129.4 AIC · ⌖ 5.57 AIC · ⊞ 6.6K
Comment /matt to run again
Comments that could not be inline-anchored
pkg/cli/mcp_inspect_mcp_scripts_server.go:85
[/codebase-design] Misleading log message: the log still says "Failed to create request" but that logic now lives inside probeServer. The error returned is a request-construction failure, not a network failure — but the log label no longer matches.
<details>
<summary>💡 Suggested fix</summary>
Update the error log to match the helper's semantics:
if err != nil {
mcpInspectLog.Printf("Probe error (non-retryable): %v", err)
return err
}This also helps readers distin…
pkg/workflow/runner_topology_validation.go:89
[/codebase-design] The condition if _, ok := seen["sudo"]; containsSudoCommand(trimmed) && !ok separates the map lookup from the predicate it guards — the ok binding is visible in the outer if even when containsSudoCommand is false, which makes this harder to reason about.
<details>
<summary>💡 Suggested rewrite (standard Go idiom)</summary>
The conventional form keeps the lookup co-located with the guard:
if containsSudoCommand(trimmed) {
if _, ok := seen["sudo"]; !ok …
</details>
<details><summary>pkg/workflow/dependabot_test.go:606</summary>
**[/tdd]** `os.ReadFile` error is silently discarded on this line — if the file read fails, `json.Unmarshal` below will see empty `data` and produce a misleading zero-value struct rather than failing fast.
<details>
<summary>💡 Suggested fix</summary>
```go
data, err := os.ReadFile(packageJSONPath)
if err != nil {
t.Fatalf("Failed to read package.json: %v", err)
}The json.Unmarshal error is now correctly checked (good!), but the ReadFile on this line still uses the blank identif…
pkg/workflow/dependabot_test.go:340
[/tdd] Both os.ReadFile and yaml.Unmarshal errors are silently discarded here — this was not touched in the PR but is in the same test file where json.Unmarshal errors were just fixed. A read failure would produce a silent zero-value DependabotConfig causing a false-pass.
<details>
<summary>💡 Suggested fix</summary>
data, err := os.ReadFile(dependabotPath)
if err != nil {
t.Fatalf("Failed to read dependabot.yml: %v", err)
}
if err := yaml.Unmarshal(data, &config); err !…
</details>
<details><summary>pkg/cli/mcp_inspect_mcp_scripts_server.go:59</summary>
**[/codebase-design]** `probeServer` swallows the transport error and returns `(false, nil)` — this is correct for the retry loop, but the `//nolint:nilerr` comment only explains *that* it is intentional, not *why* callers can retry safely. A future reader might conclude the `nilerr` suppression is wrong and add error propagation, breaking the polling loop.
<details>
<summary>💡 Suggested comment improvement</summary>
```go
// probe failure is expected during startup; callers should retry
ret…
</details>
<details><summary>pkg/workflow/action_resolver.go:37</summary>
**[/codebase-design]** The public return type of `GetUsedCacheKeys()` is now `map[string]struct{}` — this is the correct representation, but there is no helper or alias to signal intent (e.g. a `type ActionKeySet = map[string]struct{}`). Callers reading the signature see a raw `map[string]struct{}` with no indication it is a set.
<details>
<summary>💡 Optional improvement</summary>
A type alias in the `workflow` package would make intent explicit and allow future evolution (e.g. swapping to `…
</details>There was a problem hiding this comment.
Non-blocking observations on an otherwise clean lint sweep
The mechanical fixes (.Error() removal, len(s)>0→s!="", strings.Count, sort→slices, map[string]bool→struct{}, json.Unmarshal error handling, time.Sleep→select) are all correct. The map[string]struct{} public API change is complete — every call site updated, no bool-style indexing survives anywhere in the repo.
Two low-signal observations are left inline (neither blocks merge):
runner_topology_validation.go:if init; conditionpattern introduces a maintenance hazard around key-string coupling, where mismatched init/write keys would silently readokfrom the wrong slot.mcp_inspect_mcp_scripts_server.go: theprobeServerhelper is a good extraction, butreturn true, nilwithout draining the body means connection reuse is silently disabled for every successful probe — this refactor is a good spot to close a pre-existing gap.
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.
🔎 Code quality review by PR Code Quality Reviewer · 164.8 AIC · ⌖ 5.76 AIC · ⊞ 5.4K
Comment /review to run again
|
|
||
| if containsSudoCommand(trimmed) && !seen["sudo"] { | ||
| seen["sudo"] = true | ||
| if _, ok := seen["sudo"]; containsSudoCommand(trimmed) && !ok { |
There was a problem hiding this comment.
Condition evaluation order inverted: ok is always bound regardless of whether the check is needed.
💡 Details and suggested fix
The original code short-circuited naturally:
if containsSudoCommand(trimmed) && !seen["sudo"] {The map was only read when containsSudoCommand returned true.
The replacement uses the if init; condition form:
if _, ok := seen["sudo"]; containsSudoCommand(trimmed) && !ok {This unconditionally performs the map lookup on every iteration and binds ok before the condition is evaluated. The semantics are still correct today, but the pattern creates a maintenance hazard: the key string in the init ("sudo") and the key used to write (seen["sudo"] = struct{}{}) are visually separated, so a future copy-paste that changes one but not the other will silently mis-read ok from the wrong key.
A cleaner way to preserve short-circuit while using map[string]struct{}:
if containsSudoCommand(trimmed) {
if _, ok := seen["sudo"]; !ok {
seen["sudo"] = struct{}{}
violations = append(violations, "sudo")
}
}The same applies to the apt-get install block on the following lines.
| mcpInspectLog.Printf("Warning: failed to close response body: %v", closeErr) | ||
| } | ||
| }() | ||
| return true, nil |
There was a problem hiding this comment.
Response body not drained before Close: connection reuse silently disabled for every successful probe.
💡 Details and suggested fix
Go's net/http Transport only reuses a keep-alive connection when the response body is fully read (or explicitly discarded) before Close. When probeServer returns true without draining the body, the TCP connection is abandoned rather than returned to the idle pool.
In a tight polling loop (up to ~25 probes at 200 ms intervals), each probe that hits a non-empty response leaves a half-open connection. The default MaxIdleConnsPerHost is 2, so after the first few probes the client opens a fresh TCP connection every time instead of reusing idle ones.
Fix — drain the body before deferring Close:
resp, err := client.Do(req)
if err != nil {
return false, nil (nolint/redacted):nilerr
}
defer func() {
_, _ = io.Copy(io.Discard, resp.Body)
if closeErr := resp.Body.Close(); closeErr != nil {
mcpInspectLog.Printf("Warning: failed to close response body: %v", closeErr)
}
}()
return true, nilNote: the original inline code had the same issue — this refactor is a good opportunity to close it.
|
/souschef |
|
✅ PR Sous Chef finished PR sous-chef nudges. processed=5; skipped_checks_running=2; skipped_last_comment_from_sous_chef=1; skipped_cooldown=0; nudged=2; branch_update_attempts=0; formatter_pushes=0; merge_main_scheduled=0 |
🤖 PR Triage
Score Breakdown: Impact 12 + Urgency 10 + Quality 13 Rationale: Lint cleanup across 52 files (187+/113−) for 8 golint categories from a lint-monster scan. Includes an ADR doc. Wide blast radius (52 files) but changes are mechanical/formatting. Low urgency; batch with other housekeeping PRs. Recommended Action: Batch review — no rush, schedule with other housekeeping items.
|
|
@copilot run pr-finished skill |
…imer Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
…y if-init pattern Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Done. Addressed all unresolved review threads across two commits:
All local checks pass ( |
|
@copilot please run the
|
…targeted-cleanup Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
Done. The CI failure ( |
|
@copilot merge main , recompile |
…targeted-cleanup Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Done. Merged |
Resolves 77 targeted custom-lint findings across 8 narrow categories. Excludes the shared function-length backlog (tracked separately).
Changes by category
deferinside loop — extractedprobeServer()helper inmcp_inspect_mcp_scripts_server.gosodefer resp.Body.Close()is scoped to a single function call, satisfying bothdeferinloopandhttprespbodyclosetime.Sleep→select— replaced blocking sleep inmcp_inspect.goshutdown path withselect { case <-time.After(...): case <-ctx.Done(): }for proper context cancellationDiscarded
json.Unmarshalerrors — wrapped bare unmarshal calls inlogs_awinfo_backward_compat_test.goanddependabot_test.gowithif err != nil { t.Fatalf(...) }Redundant
.Error()in format strings — removed.Error()from%sformat args in 5 locations (frontmatter_error.go,model_alias_validation.go,actions_build_command.go,generate_action_metadata_command.go)len(strings.Split(...))count anti-pattern — replaced withstrings.Count(...)+1incodex_logs.go,pip.go,xml_comments.gosort.Slice→slices.SortFunc— updatedsafe_jobs.gousingstrings.Compare(avoids name collision with acmpvariable in the same package's test files)map[string]boolas set — converted tomap[string]struct{}for two localseenvariables (lsp_manager.go,runner_topology_validation.go) and the full public API chain inaction_resolver.go/action_cache.go(struct fields,GetUsedCacheKeys()return type,PruneOrphanedEntries()parameter, all read/write sites, and both test files)len(s) > 0→s != ""— 50+ replacements across 34 files