Remediate 14 collection/perf custom-lint findings - #51507
Conversation
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Modernizes collection and sorting patterns to resolve 14 custom-lint findings without changing behavior.
Changes:
- Replaces set-like boolean maps with
struct{}sets. - Uses
strings.Builderand direct empty-string comparisons. - Replaces
sort.Slicewith type-safeslices.SortFunc.
Show a summary per file
| File | Description |
|---|---|
pkg/workflow/samples_validation.go |
Modernizes sidecar field sets. |
pkg/workflow/samples_replay.go |
Updates sidecar membership checks. |
pkg/workflow/engine_definition.go |
Simplifies empty-string checks. |
pkg/workflow/copilot_logs.go |
Uses direct content emptiness check. |
pkg/workflow/copilot_engine_execution.go |
Builds unique heredoc delimiters efficiently. |
pkg/workflow/central_slash_command_workflow.go |
Modernizes event-type sets. |
pkg/workflow/central_slash_command_workflow_test.go |
Updates the set test helper. |
pkg/linters/globwalkignorederror/globwalkignorederror.go |
Modernizes checked-function sets. |
pkg/cli/runner_guard_activation_gate.go |
Modernizes gated-job sets. |
pkg/cli/runner_guard_activation_gate_test.go |
Updates set membership assertions. |
pkg/cli/mcp_tools_readonly.go |
Uses a builder for diagnostics. |
pkg/cli/compile_pipeline.go |
Uses type-safe feature sorting. |
Review details
Tip
Add a code-review agent skill for context-aware, tailored reviews. Learn more in the docs.
- Files reviewed: 12/12 changed files
- Comments generated: 0
- Review effort level: Balanced
PR Triage
Ready (non-draft), CI green, reviewer commented, small footprint (12 files), no-behavior-change lint remediation - good fast-track candidate.
|
|
✅ PR Code Quality Reviewer completed the code quality review. Warning Threat Detection Engine Failure — The analysis engine could not complete. This is a tooling failure, not a security finding. What happenedThe threat detection engine failed to produce results. Review the workflow run logs for details. Warning Firewall blocked 1 domainThe following domain was blocked by the firewall during workflow execution:
To allow these domains, add them to the network:
allowed:
- defaults
- "api.individual.githubcopilot.com"See Network Configuration for more information.
|
|
✅ Test Quality Sentinel completed test quality analysis.
|
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅
|
|
✅ Ponytail Reviewer completed successfully!
|
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. No ADR enforcement needed: PR does not have the 'implementation' label and has ≤100 new lines of code in business logic directories (61 additions detected, threshold is 100).
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
All 14 linter remediations are correct mechanical changes: map[string]bool to map[string]struct{} set idiom applied consistently, slices.SortFunc preserves sort order, strings.Builder usage is functionally equivalent, and test assertions are correctly updated. LGTM.
🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · sonnet46 · 15.7 AIC · ⌖ 7.12 AIC · ⊞ 5.5K
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /codebase-design — approving with one minor observation.
📋 Key Themes & Highlights
Positive Highlights
- ✅ Consistent mechanical transformation: every
map[string]boolset becomesmap[string]struct{}with comma-ok membership tests — no missed sites - ✅ Test assertions updated correctly to
assert.Contains(the right semantic match for struct-set membership) - ✅
strings.Builderswap inextractShellcheckDiagnosticsis genuinely a hot path and the refactor is clean - ✅
slices.SortFunc+cmp.Compareis the idiomatic modern Go replacement forsort.Slice - ✅ Empty-string simplifications (
id == "",content.Content != "") improve clarity with no behaviour change - ✅
globwalkignorederrorsplit into two early returns improves readability without altering logic
Minor Observation
One inline comment on copilot_engine_execution.go: the strings.Builder for the heredoc delimiter conflict loop is over-engineered for a cold/trivial path — a nolint directive would be more readable.
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · sonnet46 · 20.1 AIC · ⌖ 7.51 AIC · ⊞ 7.1K
Comment /matt to run again
| delimiter.WriteString("_X") | ||
| } | ||
| heredocDelimiter := delimiter.String() | ||
|
|
There was a problem hiding this comment.
[/codebase-design] The strings.Builder here is over-engineering for a loop that runs 0 times in normal operation. The linter rule targets hot-path accumulations; this is a cold-path conflict-avoidance suffix loop where string concatenation is perfectly readable.
💡 Consider a targeted nolint instead
heredocDelimiter := "GH_AW_ENGINE_COMMAND_EOF"
for strings.Contains(scriptContent, heredocDelimiter) { (nolint/redacted):loop-concat -- trivial cold path
heredocDelimiter += "_X"
}This keeps the intent clear and documents why the deviation is intentional.
@copilot please address this.
There was a problem hiding this comment.
Reverted to plain string concatenation with //nolint:stringsconcatloop as suggested. Commit: pushed in the latest update.
🧪 Test Quality Sentinel Report✅ Test Quality Score: N/A — Test Maintenance
📊 Analysis Summary
Changed Files:
Changes Made:
Verdict
|
There was a problem hiding this comment.
Ponytail review — over-engineering pass only.
One finding: the heredoc-delimiter uniquifier switched from plain string concatenation to strings.Builder, but the loop appends _X at most a couple of times to a short constant string — strings.Builder buys nothing here over +=.
Everything else in this diff (set-like map[string]bool → map[string]struct{}, sort.Slice → slices.SortFunc, len(s)==0 → s=="") is a net simplification with no added abstraction, matching the PR title.
net: -3 lines possible.
Generated by ✂️ Ponytail Reviewer for #51507 · auto · 30.1 AIC · ⌖ 5.26 AIC · ⊞ 6.8K
Comment /ponytail to run again
| for strings.Contains(scriptContent, delimiter.String()) { | ||
| delimiter.WriteString("_X") | ||
| } | ||
| heredocDelimiter := delimiter.String() |
| for strings.Contains(scriptContent, delimiter.String()) { | ||
| delimiter.WriteString("_X") | ||
| } | ||
| heredocDelimiter := delimiter.String() |
There was a problem hiding this comment.
L756-761: shrink: strings.Builder for a delimiter that appends _X at most a couple of times. Plain string concat is shorter and just as clear: heredocDelimiter := "GH_AW_ENGINE_COMMAND_EOF"; for strings.Contains(scriptContent, heredocDelimiter) { heredocDelimiter += "_X" }.
…ith nolint Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
The CI failures are all infrastructure cache misses (stale |
make golint-customflagged 14 findings in the "collections and small perf/style" group: set-likemap[string]bool, loop string concatenation,len(s) == 0empty-string checks, and onesort.Slice. This applies the narrow fix at each flagged site with no behavior change.Set-like maps →
map[string]struct{}(8)pkg/workflow/central_slash_command_workflow.go—mergedEventsevent-type sets, threaded throughcollectCentralCommandRoutes,collectCentralSlashCommandRoutes,collectCentralLabelCommandRoutes,writeCentralSlashEventsYAML,writeCentralSlashRoutePermissions,needsPullRequestsPermissionpkg/workflow/samples_validation.go,samples_replay.go—sampleSidecarFieldsandstripSidecarFieldspkg/cli/runner_guard_activation_gate.go—gatedJobsByFile,authorAssociationGatedJobs,anyJobGatedpkg/linters/globwalkignorederror/globwalkignorederror.go—checkedFuncsMembership tests become comma-ok lookups; test assertions move from
assert.True(t, gated["x"])toassert.Contains(t, gated, "x").Loop concatenation →
strings.Builder(2)pkg/workflow/copilot_engine_execution.go— heredoc-delimiter uniquifying looppkg/cli/mcp_tools_readonly.go— shellcheck diagnostic accumulator;flushnow resets the builder andcurrent != ""becomescurrent.Len() > 0Empty-string checks (3)
pkg/workflow/copilot_logs.go—content.Content != "", withoutputSizecomputed inside the blockpkg/workflow/engine_definition.go—id == "" || importPath == ""Type-safe sort (1)
pkg/cli/compile_pipeline.goswaps thesortimport forcmp/slices:Note on scope
check-workflow-driftcurrently fails onmain: 284.lock.ymlfiles regenerate with newprocess_safe_outputs_items_*outputs. This reproduces on a clean tree, so the regenerated lock files are excluded from this PR.Run: https://github.com/github/gh-aw/actions/runs/31310371321> Generated by 👨🍳 PR Sous Chef · gpt54 · 4.98 AIC · ⌖ 5.48 AIC · ⊞ 6.1K · ◷