Targeted custom-lint remediation for non-largefunc findings in CLI/workflow paths#47601
largefunc findings in CLI/workflow paths#47601Conversation
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
largefunc findings in CLI/workflow paths
There was a problem hiding this comment.
Pull request overview
Targeted cleanup of non-largefunc lint findings across CLI and workflow code.
Changes:
- Centralizes path and environment access.
- Applies context, error-wrapping, and set-map fixes.
- Makes MCP waits cancellation-aware.
Show a summary per file
| File | Description |
|---|---|
pkg/workflow/workflow_builder.go |
Uses a struct-based set. |
pkg/workflow/safe_outputs_tools_generation.go |
Uses a struct-based supported-values set. |
pkg/workflow/llm_provider.go |
Simplifies empty-provider handling. |
pkg/workflow/github_cli.go |
Replaces nil context with a background context. |
pkg/cli/mcp_inspect_inspector.go |
Adds cancellation-aware waits. |
pkg/cli/copilot_agents.go |
Extracts directory-description constants. |
pkg/cli/bootstrap_profile_manifest.go |
Preserves wrapped error chains. |
pkg/cli/bootstrap_profile_helpers.go |
Routes environment reads through lookupEnv. |
pkg/cli/bootstrap_profile_github_app.go |
Centralizes GitHub App environment reads. |
.github/workflows/daily-team-evolution-insights.lock.yml |
Regenerates strict-mode workflow output. |
Review details
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 10/10 changed files
- Comments generated: 2
- Review effort level: Medium
| case <-gctx.Done(): | ||
| return gctx.Err() |
There was a problem hiding this comment.
Fixed in dad1583. The cleanup defer is now registered immediately after g, gctx := errgroup.WithContext(ctx) (line 52), before any server is started or the startup wait loop runs. Context cancellation at line 215 now correctly triggers the cleanup of any already-started processes and drains monitor goroutines via g.Wait().
| GH_AW_INFO_FIREWALL_TYPE: "squid" | ||
| GH_AW_INFO_FRONTMATTER_EMOJI: "📊" | ||
| GH_AW_COMPILED_STRICT: "true" | ||
| GH_AW_COMPILED_STRICT: "false" |
There was a problem hiding this comment.
The lock file update is required and correct. The .md source has strict: false in its frontmatter — the old lock file's "strict":true metadata was a compiler bug that emitted strict:true regardless of the frontmatter value. The current compiler correctly reflects strict: false. Reverting to the old lock file would cause check-workflow-drift to hard-fail (confirmed locally). This workflow has no source: frontmatter entry, so it is not upstream-managed. The merge with main also brought the updated lock file through the AWF v0.27.39/mcpg v0.4.4 container version bump.
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅ |
|
✅ Test Quality Sentinel completed test quality analysis. No test files were added or modified in this PR. Test Quality Sentinel skipped. |
|
✅ PR Code Quality Reviewer completed the code quality review. |
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. No ADR enforcement needed: PR #47601 does not have the 'implementation' label and has only 44 new lines of code in business logic directories (threshold: 100). |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review Summary
The Go changes are clean and correct — lookupEnv consolidation, %w error wrapping, map[string]struct{} idiom, context.Background(), and the context-aware select/time.After patterns are all good improvements.
Two blocking concerns (covered by existing inline comments):
-
.github/workflows/daily-team-evolution-insights.lock.yml— this generated/source-managed file is changed in a way unrelated to lint remediation:strict:trueis removed from the metadata header andGH_AW_COMPILED_STRICTis flipped to"false". Lock files for source-managed workflows must not be edited manually; usegh aw updateormake recompile. Revert this file. -
pkg/cli/mcp_inspect_inspector.goline 186 — earlyreturn gctx.Err()on cancellation occurs before the cleanup defer is registered; stdio servers that have already started may not be properly terminated.
Please address these two issues before merging.
🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · sonnet46 · 18 AIC · ⌖ 5.7 AIC · ⊞ 5K
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /codebase-design and /diagnosing-bugs — requesting changes on two correctness issues and one process concern.
📋 Key Themes & Highlights
Blocking Issues
- Upstream-managed lock file edited directly (
.github/workflows/daily-team-evolution-insights.lock.yml): Per repo rules, workflows with asource:frontmatter entry are provenance-managed and must be updated viagh aw updateor upstream, not by hand-editing the.lock.yml. This change will be silently clobbered on the next update run. - Process leak on early context cancellation (
mcp_inspect_inspector.go:186): The newgctx.Done()return path exits before the cleanup goroutine is registered, risking orphaned stdio server processes when cancellation arrives during startup.
Minor Improvement
- Timer drain boilerplate (
mcp_inspect_inspector.go:228–239): The manualtimer.Stop()+ drain after theselectis unnecessary here —defer timer.Stop()is the idiomatic pattern and removes ~7 lines.
Positive Highlights
- ✅ All Go-code changes are clean, idiomatic, and behavior-preserving
- ✅
lookupEnvconsolidation improves testability consistently across bootstrap paths - ✅
%werror wrapping inbootstrap_profile_manifest.gocorrectly preserves the error chain - ✅
context.Background()replacement inExecGHremoves a staticcheck suppression comment - ✅
map[string]struct{}conversions are idiomatic and appropriately applied
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · sonnet46 · 39.6 AIC · ⌖ 4.85 AIC · ⊞ 6.7K
Comment /matt to run again
| if !timer.Stop() { | ||
| select { | ||
| case <-timer.C: | ||
| default: |
There was a problem hiding this comment.
[/codebase-design] Timer drain after context cancellation is redundant — once you exit the select, the manual timer.Stop() + drain block is only needed if you reuse the timer (you don't).
💡 Simpler idiom
timer := time.NewTimer(mcpProcessCleanupDelay)
defer timer.Stop()
select {
case <-timer.C:
case <-gctx.Done():
}defer timer.Stop() is the standard Go pattern and eliminates 7 lines of boilerplate.
@copilot please address this.
There was a problem hiding this comment.
Already addressed in 21956c4. The manual timer.Stop() + drain block was replaced with defer timer.Stop() followed by the select, which is the idiomatic Go pattern and removes the extra lines.
| select { | ||
| case <-time.After(mcpStdioServerStartupDelay): | ||
| case <-gctx.Done(): | ||
| return gctx.Err() |
There was a problem hiding this comment.
[/diagnosing-bugs] Early return on context cancellation here (line 186) happens before the cleanup defer at line 219+ is registered, so already-started stdio servers may be left running if cancellation arrives in this window.
💡 Suggestion
Consider registering the cleanup goroutine (or at minimum a defer killAllStartedServers()) before the startup loop so that any early cancellation still terminates spawned processes. The existing review comment from Copilot flags the same gap.
@copilot please address this.
There was a problem hiding this comment.
Fixed in dad1583. The cleanup defer is registered at line 52 (immediately after errgroup.WithContext), before the startup loop. The early return gctx.Err() at line 215 now correctly triggers process cleanup and goroutine draining for any servers that were already started.
| GH_AW_INFO_FIREWALL_TYPE: "squid" | ||
| GH_AW_INFO_FRONTMATTER_EMOJI: "📊" | ||
| GH_AW_COMPILED_STRICT: "true" | ||
| GH_AW_COMPILED_STRICT: "false" |
There was a problem hiding this comment.
[/codebase-design] This lock file is upstream-managed (provenance-managed from a source: frontmatter entry) and should be treated as read-only in this repo — manual edits here will be overwritten on the next gh aw update. The strict-mode policy step should be introduced upstream, not patched directly into the compiled lock file.
💡 Correct path
Per the repo's AGENTS.md operating rules, workflows with a source: frontmatter entry must be updated via gh aw update or by changing the upstream source, not by editing the .lock.yml directly. This change belongs in the upstream bundle, not here.
@copilot please address this.
There was a problem hiding this comment.
The workflow does not have a source: frontmatter entry, so it is not upstream-managed per the repo's AGENTS.md rules. The lock file update is necessary: the .md source has strict: false in its frontmatter, but the old lock file incorrectly had "strict":true in its metadata (a prior compiler bug). The current compiler correctly omits strict:true when not set in the frontmatter. Reverting would break check-workflow-drift (confirmed locally). The merge with main also synchronized this file with the AWF v0.27.39/mcpg v0.4.4 container bump.
There was a problem hiding this comment.
Review: Two issues block merge
The non-largefunc lint fixes are generally clean — context-aware timers, map[string]struct{}, %w wrapping, and lookupEnv abstraction are all correct and idiomatic. However two items in this PR need to be resolved before merging.
🔍 Findings summary
Blocking
- Lock file strict-mode regression (
.github/workflows/daily-team-evolution-insights.lock.yml) —strict: truewas removed from the metadata header andGH_AW_COMPILED_STRICTwas flipped to"false". This file is auto-generated and should not be manually edited. More critically, strict enforcement is now opt-in (requiresGH_AW_POLICY_STRICT=true), inverting the original safety guarantee. - Early context-cancel during MCP inspector startup leaves child processes alive (
pkg/cli/mcp_inspect_inspector.go) — returninggctx.Err()before the cleanup defer fires means already-started stdio servers are orphaned on cancellation.
Non-blocking / correct as-is
context.Background()inExecGHis semantically equivalent to nil for a never-cancelled context; removing thenolintcomment is fine.normalizeLLMProviderrefactor is correct — alias lookup still uses the lowercased string.- Cleanup loop continuing on cancellation is intentional (drain all processes on shutdown).
🔎 Code quality review by PR Code Quality Reviewer · sonnet46 · 45.8 AIC · ⌖ 4.59 AIC · ⊞ 5.7K
Comment /review to run again
|
@copilot run pr-finisher skill |
… file Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Done. Two issues addressed in commit
|
|
@copilot please run the Unresolved review feedback (newest first):
|
Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
All unresolved review threads addressed across two commits:
|
🤖 PR Triage
Score breakdown: Impact 25 + Urgency 18 + Quality 17 Batch: Rationale: Ready (not draft), 10 Go/workflow files with lint remediation. Automated reviewer requested changes. CI data unavailable but related to code quality. Batches well with #47592 (CLI fixes).
|
|
@copilot please run the Unresolved review feedback (newest first):
Run: https://github.com/github/gh-aw/actions/runs/30035273314
|
…targeted-custom-lint-fixes # Conflicts: # .github/workflows/daily-team-evolution-insights.lock.yml Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
All unresolved review threads addressed:
Mergeable: Branch was behind main by 3 commits; merged in Local validation: |
|
@copilot please run the Unresolved requested changes:
|
All issues have been addressed in previous commits and local validation passes. Here's the current state: Code fixes (complete):
Local validation: Blocker requiring human action: The 3 |
|
@copilot please run the Run: https://github.com/github/gh-aw/actions/runs/30046537550
|
Confirmed: the three Staleness evidence:
Branch state: up to date with Remaining blocker: The |
This PR addresses the non-
largefuncfindings from the lint-monster pass by applying narrow, behavior-preserving cleanups in the reported files only. The changes focus on path constant extraction, env coupling reduction, and small API/correctness/perf fixes.Path constant cleanup
pkg/cli/copilot_agents.go(skill and agent path descriptors), removing repeated hard-coded literals.Environment coupling reduction
os.Getenvcalls with the existinglookupEnvabstraction in:pkg/cli/bootstrap_profile_github_app.gopkg/cli/bootstrap_profile_helpers.goCorrectness and API hygiene
context.Background()inpkg/workflow/github_cli.go.map[string]booltomap[string]struct{}in:pkg/workflow/safe_outputs_tools_generation.gopkg/workflow/workflow_builder.go.Error()usage while preserving wrapped error chains inpkg/cli/bootstrap_profile_manifest.go.pkg/workflow/llm_provider.goto idiomatic== "".Context-aware timing in inspector flow
time.Sleep-based waits withselect+ context-aware timer handling inpkg/cli/mcp_inspect_inspector.go, so wait paths can terminate promptly on cancellation.Example pattern applied:
Run: https://github.com/github/gh-aw/actions/runs/30035273314
Run: https://github.com/github/gh-aw/actions/runs/30042541462