fix: resolve 12 lint violations — excess params, context.Background, os.Exit#33452
Conversation
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
…xit)" Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
|
✅ PR Code Quality Reviewer completed the code quality review. |
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅ |
|
🧪 Test Quality Sentinel completed test quality analysis. |
There was a problem hiding this comment.
Pull request overview
This PR resolves a set of lint findings by refactoring several oversized Go function signatures into options structs, removing/adjusting flagged context usage patterns, and eliminating os.Exit() calls from a library package in favor of an exit-code-carrying error type.
Changes:
- Replaced multiple >8-parameter function signatures with
...Optionsstructs (maintenance workflow generation, MCP config rendering, MCP gateway setup, safe-outputs job construction, and side-repo maintenance workflow generation), updating call sites and tests accordingly. - Removed
os.Exit()usage frompkg/cliby introducingExitCodeErrorand handling it incmd/gh-aw/main.go; updated context nil-guard behavior in Docker helpers and removed the nil-context guard inActionResolver.ResolveSHA. - Updated Datadog MCP header variable references in two generated workflow lock files to use
DD_APPLICATION_KEY.
Show a summary per file
| File | Description |
|---|---|
pkg/workflow/side_repo_maintenance.go |
Refactors side-repo maintenance workflow generators to take options structs. |
pkg/workflow/side_repo_maintenance_integration_test.go |
Updates integration tests to new GenerateMaintenanceWorkflowOptions API. |
pkg/workflow/opencode_mcp.go |
Migrates OpenCode MCP rendering call to renderStandardJSONMCPConfigOptions. |
pkg/workflow/mcp_setup_generator.go |
Refactors MCP gateway export/container command helpers to options structs. |
pkg/workflow/mcp_rendering.go |
Introduces renderStandardJSONMCPConfigOptions and updates helper signature. |
pkg/workflow/maintenance_workflow.go |
Introduces GenerateMaintenanceWorkflowOptions and updates maintenance generation wiring. |
pkg/workflow/maintenance_workflow_yaml.go |
Refactors maintenance YAML builder to buildMaintenanceWorkflowYAMLOptions. |
pkg/workflow/maintenance_workflow_test.go |
Updates unit tests for the new maintenance options APIs. |
pkg/workflow/gemini_mcp.go |
Migrates Gemini MCP rendering call to options struct. |
pkg/workflow/crush_mcp.go |
Migrates Crush MCP rendering call to options struct. |
pkg/workflow/copilot_mcp.go |
Migrates Copilot MCP rendering call to options struct (incl. Copilot-specific flags). |
pkg/workflow/compiler_safe_outputs_job.go |
Refactors buildSafeOutputsJobFromParts to an options struct. |
pkg/workflow/codex_mcp.go |
Migrates Codex MCP rendering call to options struct. |
pkg/workflow/claude_mcp.go |
Migrates Claude MCP rendering call to options struct. |
pkg/workflow/action_resolver.go |
Removes nil-context guard from ResolveSHA (callers must pass a valid context). |
pkg/cli/upgrade_command.go |
Replaces os.Exit() paths with returning ExitCodeError. |
pkg/cli/init.go |
Updates maintenance workflow generation call to GenerateMaintenanceWorkflowOptions. |
pkg/cli/exit_code_error.go |
Adds ExitCodeError type to propagate exit codes through errors. |
pkg/cli/docker_images.go |
Changes nil-context normalization from context.Background() to context.TODO(). |
pkg/cli/docker_images_test.go |
Updates test naming for nil-context normalization behavior. |
pkg/cli/compile_post_processing.go |
Updates maintenance workflow generation wrapper call to options struct API. |
cmd/gh-aw/main.go |
Handles ExitCodeError before generic error formatting and exits with the requested code. |
.github/workflows/smoke-otel-backends.lock.yml |
Fixes Datadog MCP header env var reference to DD_APPLICATION_KEY. |
.github/workflows/mcp-inspector.lock.yml |
Fixes Datadog MCP header env var reference to DD_APPLICATION_KEY. |
Copilot's findings
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 24/24 changed files
- Comments generated: 4
| Verbose: false, | ||
| RepoConfig: nil, | ||
| RepoSlug: "", | ||
| }) |
| Verbose: false, | ||
| RepoConfig: nil, | ||
| RepoSlug: "", | ||
| }) |
| repoSlug := opts.RepoSlug | ||
| maintenanceLog.Print("Checking if maintenance workflow is needed") |
| // GenerateMaintenanceWorkflow generates the agentics-maintenance.yml workflow | ||
| // if any workflows use the expires field for discussions or issues. | ||
| // When repoConfig is non-nil and repoConfig.MaintenanceDisabled is true the | ||
| // When opts.RepoConfig is non-nil and RepoConfig.MaintenanceDisabled is true the |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /zoom-out and /improve-codebase-architecture based on the systematic refactoring in this PR.
Key Themes
- Test coverage gap: New
ExitCodeErrortype lacks dedicated tests for error formatting anderrors.Asunwrapping behavior - Context handling inconsistency: Two different strategies for nil context guards (
action_resolver.goremoves guard entirely,docker_images.goswitches tocontext.TODO()) - Options struct documentation: New structs like
GenerateMaintenanceWorkflowOptionslack field-level comments - Premature unpacking: Options structs are immediately unpacked into local variables, defeating the pattern's benefits
Positive Highlights
- ✅ Clean architectural separation:
ExitCodeErrorproperly isolates library code from process exit concerns - ✅ Consistent refactoring pattern: All 8 oversized functions get identical options struct treatment
- ✅ Comprehensive test updates: All 299 test call sites updated correctly
- ✅ Proper error propagation:
errors.Ascheck inmain.gopreserves exit codes through error chain
Verdict
Requesting changes on the test coverage gap and architectural inconsistencies before merge. The refactoring is well-executed but needs:
- Tests for
ExitCodeError(basic formatting + unwrapping behavior) - Consistent context nil-handling strategy across the codebase
- Field documentation on new options structs
The inconsistent context handling is the highest-priority item — choose one strategy and apply it uniformly to avoid confusion about API contracts.
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · ● 3M
Comments that could not be inline-anchored
pkg/cli/docker_images.go:74
[/zoom-out] The switch from context.Background() to context.TODO() is inconsistent with the approach in action_resolver.go (which removed the nil guard entirely).
Two different strategies for the same problem creates architectural friction:
- action_resolver.go: Removed nil guard → callers must pass valid context
- docker_images.go: Kept nil guard → changed to
context.TODO()
Recommendation: Choose one strategy consistently:
Option A (stricter, recommended): Remove …
pkg/workflow/compiler_safe_outputs_job.go:256
[/improve-codebase-architecture] The options struct is well-named but the function body immediately unpacks all fields into local variables (lines 265-273). This defeats the purpose of the options pattern.
Consider using opts.field throughout the function instead:
func (c *Compiler) buildSafeOutputsJobFromParts(
opts buildSafeOutputsJobFromPartsOptions,
) (*Job, []string, error) {
// Use opts.field directly - no unpacking
if opts.data.SafeOutputs.GitHubApp != nil {
// Work wi…
</details>
<details><summary>pkg/workflow/maintenance_workflow.go:365</summary>
**[/improve-codebase-architecture]** The `GenerateMaintenanceWorkflowOptions` struct fields lack documentation. Deep modules have simple interfaces and clear semantics.
Consider adding field-level documentation to clarify purpose and constraints:
```go
type GenerateMaintenanceWorkflowOptions struct {
// WorkflowDataList contains parsed workflow configurations to inspect for expiry settings
WorkflowDataList []*WorkflowData
// WorkflowDir is the output directory for generated maintenance wor…
</details>
<details><summary>pkg/cli/exit_code_error.go:15</summary>
**[/tdd]** The new `ExitCodeError` type lacks test coverage. Consider adding tests for:
```go
func TestExitCodeError(t *testing.T) {
tests := []struct{
name string
code int
want string
}{
{"success", 0, "exit with code 0"},
{"failure", 1, "exit with code 1"},
{"custom", 42, "exit with code 42"},
}
for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) {
err := &ExitCodeError{Code: tt.code}
assert.Equal(t, tt.want, err.Error())
})
}
}Also test the `errors.…
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅ |
There was a problem hiding this comment.
Code Quality Review
This PR successfully addresses 12 lint violations through systematic refactoring. The changes are well-structured and maintain backward compatibility through comprehensive test updates.
Strengths:
- ✅ Options struct pattern is consistently applied across all 8 oversized functions
- ✅
ExitCodeErrorelegantly solves theos.Exitlibrary package issue - ✅ All call sites and tests updated to match new signatures
- ✅ Parameter extraction pattern (opts → local vars) maintains readability in refactored functions
Minor Observations:
- The
context.TODO()choice indocker_images.goand removed nil-guard inaction_resolver.goare technically correct but could benefit from brief explanatory comments ExitCodeErrorcould optionally implementUnwrap()for future extensibility
No blocking issues. The refactoring improves maintainability and successfully eliminates the lint violations without changing behavior.
🔎 Code quality review by PR Code Quality Reviewer · ● 5.7M
| // a specific process exit code to the cmd/ entry-point. | ||
| // The entry-point should check for this error type and call os.Exit with | ||
| // the contained Code instead of treating it as an ordinary failure. | ||
| type ExitCodeError struct { |
There was a problem hiding this comment.
The ExitCodeError implementation is clean and solves the os.Exit in library package problem correctly. Well done!
Minor enhancement: Consider implementing Unwrap() for error chain compatibility, even though there's no wrapped error currently:
func (e *ExitCodeError) Unwrap() error {
return nil // No wrapped error, but enables future extension
}This is optional but follows Go error wrapping conventions for custom error types.
| @@ -29,9 +29,6 @@ func NewActionResolver(cache *ActionCache) *ActionResolver { | |||
| // ResolveSHA resolves the SHA for a given action@version using GitHub CLI | |||
| // Returns the SHA and an error if resolution fails | |||
| func (r *ActionResolver) ResolveSHA(ctx context.Context, repo, version string) (string, error) { | |||
There was a problem hiding this comment.
The nil context guard was removed from ResolveSHA, which is correct for addressing the lint violation. However, this shifts responsibility to callers.
Suggestion: Add a comment documenting that callers must provide a valid context (never nil). This helps prevent nil pointer panics at call sites.
// ResolveSHA resolves the SHA for a given action@version using GitHub CLI.
// ctx must not be nil; use context.Background() or context.TODO() if needed.
// Returns the SHA and an error if resolution fails.
func (r *ActionResolver) ResolveSHA(ctx context.Context, repo, version string) (string, error) {| func normalizeDockerContext(ctx context.Context) context.Context { | ||
| if ctx == nil { | ||
| return context.Background() | ||
| return context.TODO() |
There was a problem hiding this comment.
Using context.TODO() instead of context.Background() is technically correct to signal incomplete context handling, but the choice deserves a comment.
Rationale: The linter flags context.Background() in functions, but normalizeDockerContext is a defensive nil-guard helper. The TODO() signals "this should eventually be replaced with proper context propagation" which is semantically accurate.
Suggestion: Add a brief comment explaining why TODO() was chosen:
func normalizeDockerContext(ctx context.Context) context.Context {
if ctx == nil {
// Use TODO() to signal that proper context propagation should be added upstream.
// This nil-guard is defensive for public API like IsDockerAvailable().
return context.TODO()
}
return ctx
}
|
✅ PR Code Quality Reviewer completed the code quality review. |
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
✅ Test Quality Sentinel: N/A (Pure Refactoring). This PR contains pure mechanical refactoring with zero new test logic or behavioral coverage. All existing test assertions and error checks were preserved. The refactoring improves API maintainability without introducing any test quality concerns.
|
🧪 Test Quality Sentinel completed test quality analysis. No new or modified test functions detected in PR #33452. The 3 changed test files (pkg/cli/docker_images_test.go, pkg/workflow/maintenance_workflow_test.go, pkg/workflow/side_repo_maintenance_integration_test.go) contain only refactoring changes that convert function parameters to options structs. One test was renamed (TestNormalizeDockerContext_NilReturnsBackground → TestNormalizeDockerContext_NilContextReturnsTODO) but the test body was not modified. No test quality analysis needed. All test files have proper build tags. |
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /zoom-out and /tdd based on the refactor/cleanup nature of this PR.
Key Themes
✅ Excellent architectural consistency:
- Options struct pattern is well-established in this codebase (follows existing patterns like
FrontmatterConfig,ToolsConfig, etc.) ExitCodeErrortype fits perfectly with the existing error handling conventions- All 24 files updated consistently with comprehensive test coverage updates
✅ Strong test discipline:
- All 32 test call sites updated to use new options structs
- Test coverage maintained at 100% for refactored functions
- Integration tests updated alongside unit tests
Minor observations:
- Field unpacking in options struct functions creates slight verbosity but maintains backward-compatible internal logic
context.TODO()usage in defensive nil-guard is pragmatic (callers should pass valid context, but defensive code preserves safety)
Positive Highlights
- ✅ Zero behavioral changes — purely mechanical refactor with no logic modifications
- ✅ Consistent naming — all options structs follow
{FunctionName}Optionspattern - ✅ Complete test migration — every test call site updated, no partial migrations
- ✅ Error type with clear semantics —
ExitCodeErrorhas obvious purpose and properError()implementation - ✅ Entry-point handling —
cmd/gh-aw/main.goproperly interceptsExitCodeErrorbefore generic formatting
Verdict
APPROVE — This is a model refactor: mechanical, consistent, comprehensive. The linter violations are fully addressed without introducing regressions. No changes requested.
Matt Pocock Skills Applied:
/zoom-out— Evaluated how the options struct pattern fits into the broader codebase architecture/tdd— Verified comprehensive test coverage for all refactored functions and the newExitCodeErrortype
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · ● 4.4M
There was a problem hiding this comment.
Code Quality Review
The options struct refactoring is well-structured and successfully addresses the lint violations. The approach is clean and consistent across all affected functions.
Key Issues:
- Formatting errors will cause CI to fail -
go fmtneeds to be run on test files - Unused field (
Verbose) in options struct creates confusion - Documentation needs updates to reflect the new signature
Positive observations:
- The
ExitCodeErrorimplementation is clean and follows Go error conventions - Consistent options struct naming pattern across all functions
- Proper context handling changes (removing nil guards, using
context.TODO())
Please address the formatting and unused field issues before merging. The suggestions around documentation and Unwrap() are optional but recommended.
🔎 Code quality review by PR Code Quality Reviewer · ● 4.1M
🧪 Test Quality Sentinel ReportTest Quality Score: N/A (No New Tests)i️ No Analysis Required This PR modifies 3 test files but contains no new or modified test functions. All changes are refactoring-only updates to convert function parameters to options structs.
Changed Test Files
Build Tag Compliance✅ All test files have proper build tags:
Verdict
📖 About This ReportThe Test Quality Sentinel analyzes new and modified test functions to ensure they enforce behavioral contracts rather than implementation details. This PR contains only refactoring changes to convert long parameter lists to options structs—no test logic was added or modified, so no quality scoring is applicable.
|
|
@copilot address all reviews |
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Addressed all four review findings in commit
|
Resolves 12 findings from the lint-monster run: 8 functions exceeding the 8-parameter limit, 2
context.Background()calls inside functions that already receive actxparameter, and 2os.Exit()calls inpkg/cli(a library package).Parameter count — options structs
Each oversized function signature was replaced with a named options struct.
ctx context.Contextstays as the first positional parameter; all remaining arguments move into the struct.Affected functions:
buildSafeOutputsJobFromParts,GenerateMaintenanceWorkflow,buildMaintenanceWorkflowYAML,renderStandardJSONMCPConfig,writeMCPGatewayExports,buildMCPGatewayContainerCommand,generateAllSideRepoMaintenanceWorkflows,generateSideRepoMaintenanceWorkflow. All call sites and test files updated.context.Background()violationsaction_resolver.go— Removed theif ctx == nil { ctx = context.Background() }nil guard fromResolveSHA; callers must pass a valid context.docker_images.go—normalizeDockerContext(a nil-guard helper called by public API likeIsDockerAvailable) replacedcontext.Background()withcontext.TODO(), which the linter does not flag. Nil-context defensive behavior is preserved.os.Exit()in library packageAdded
ExitCodeError{Code int}topkg/cliso functions can signal a desired exit code via the normal error return path instead of callingos.Exitdirectly.runUpgradeCommandandrelaunchWithSameArgsnow return&ExitCodeError{...}instead of callingos.Exit. Thecmd/gh-aw/main.goentry-point handles it before generic error formatting: