Refactor two parameter-heavy functions to satisfy custom lint parameter-count limits#52817
Conversation
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
|
✅ 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. No ADR enforcement needed: PR does not have the 'implementation' label and has only 60 new lines of code in business logic directories (threshold: 100).
|
|
✅ Ponytail Reviewer completed successfully! Lean already. Ship. Diff is a minimal, mechanical fix for golint-custom's parameter-count limit: two >8-param functions were collapsed into single-purpose options structs (SafeUpdateEnforcementOptions, repositoryPackageLocation), each with exactly one call site and no speculative fields. This is the standard idiomatic Go pattern for excess parameters, not over-engineering. No dead code, reinvented stdlib, or unneeded abstractions found.
|
|
✅ PR Code Quality Reviewer completed the code quality review. Completed PR review; no actionable blocking issues were found in the changed lines, so no PR comments or review submission were necessary.
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Clean refactor — no logic changes, just grouping parameters into structs to satisfy lint limits. All call sites updated consistently. LGTM.
🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · sonnet46 · 13.5 AIC · ⌖ 6.99 AIC · ⊞ 5.4K
There was a problem hiding this comment.
Pull request overview
Refactors two parameter-heavy functions to satisfy lint limits without changing behavior.
Changes:
- Groups safe-update inputs into an options struct.
- Groups repository location inputs into a location struct.
- Updates affected callers and tests.
Show a summary per file
| File | Description |
|---|---|
pkg/workflow/safe_update_enforcement.go |
Adds safe-update options and updates enforcement logic. |
pkg/workflow/safe_update_enforcement_test.go |
Adapts safe-update tests. |
pkg/workflow/compiler.go |
Passes structured enforcement options. |
pkg/workflow/compiler_threat_detection_formal_test.go |
Adapts formal threat-detection tests. |
pkg/cli/add_package_manifest.go |
Groups repository location parameters. |
Review details
💡 Add a code-review agent skill for context-aware, tailored reviews. Learn more in the docs.
- Files reviewed: 5/5 changed files
- Comments generated: 1
- Review effort level: Balanced
| // | ||
| // Returns a structured, actionable error when violations are found. | ||
| func EnforceSafeUpdate(manifest *GHAWManifest, secretNames []string, actionRefs []string, currentRedirect string, oldHasPullRequest bool, oldHasPullRequestTarget bool, currentHasPullRequest bool, currentHasPullRequestTarget bool, currentMemoryValidationScripts []GHAWManifestMemoryValidationScript) error { | ||
| type SafeUpdateEnforcementOptions struct { |
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /codebase-design — approving with one minor suggestion.
📋 Key Themes & Highlights
Positive Highlights
- ✅
SafeUpdateEnforcementOptionscleanly groups the six boolean/string PR-context params — zero ambiguity at call sites - ✅
repositoryPackageLocationis unexported (correctly scoped to theclipackage), consistent with the existingrepositoryPackageExtensionFilespattern - ✅ All call sites and formal tests updated; behaviour-preserving refactor with good scope discipline
- ✅ Test file changes are pure mechanical substitutions — no coverage is lost
One minor suggestion
SafeUpdateEnforcementOptionsis exported but undocumented; a short GoDoc comment on the type and its boolean fields would help reviewers understand the before/after semantics (see inline comment).
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · sonnet46 · 29.6 AIC · ⌖ 8.79 AIC · ⊞ 7K
Comment /matt to run again
| CurrentRedirect string | ||
| OldHasPullRequest bool | ||
| OldHasPullRequestTarget bool | ||
| CurrentHasPullRequest bool |
There was a problem hiding this comment.
[/codebase-design] SafeUpdateEnforcementOptions is exported and used in security-sensitive enforcement logic, but has no GoDoc comment — new contributors need to understand what the boolean fields guard against without diving into the implementation.
💡 Suggested doc comment
// SafeUpdateEnforcementOptions carries the per-compilation context needed by
// EnforceSafeUpdate to detect unsafe changes to pull-request event triggers,
// redirects, and memory-validation scripts relative to the existing manifest.
type SafeUpdateEnforcementOptions struct {
// CurrentRedirect is the redirect value from the current workflow frontmatter.
CurrentRedirect string
// OldHasPullRequest reports whether the previous manifest triggered on pull_request events.
OldHasPullRequest bool
...
}@copilot please address this.
🧪 Test Quality Sentinel Report✅ Test Quality Score: 100/100 — Excellent
📊 Metrics (13 modified tests)
Verdict
|
|
@copilot Quick triage for maintainer-ready follow-up: Please refresh the branch if needed, address the remaining maintainer-facing follow-up, and run the Outstanding review items (newest first):
Failed checks from the compact candidate set:
Branch update was requested automatically for this run when GitHub allows it.
|
Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
Addressed and pushed in
|
PR TriageCategory: refactor | Risk: low | Score: 65/100
Recommended action:
|
|
@copilot Quick triage for maintainer-ready follow-up: Please refresh the branch if needed, address the remaining maintainer-facing follow-up, and run the Outstanding review items (newest first):
Failed checks from the compact candidate set:
Branch update was requested automatically for this run when GitHub allows it.
|
Confirmed on current PR state: the doc-comment follow-up in Branch refresh is not required from current GitHub PR state (not indicated as behind). Also re-triaged the referenced run ( |
PR Triage
Pure parameter-count lint fix, no behavior change. CI passing. Good auto-merge candidate. Automated triage — see [PR Triage Report] for full context.
|
|
@copilot Quick triage for maintainer-ready follow-up: Please refresh the branch if needed, address the remaining maintainer-facing follow-up, and run the Outstanding review items (newest first):
Failed checks from the compact candidate set:
Branch update was requested automatically for this run when GitHub allows it.
|
Triaged on the current branch head ( Branch refresh is not indicated from current PR state, and re-checking the referenced run ( |
|
@copilot resolve the merge conflicts on this branch. |
make golint-customflagged two functions at 9 parameters (limit 8):EnforceSafeUpdateandresolveRepositoryPackageExtensionFiles. This change reduces each signature with minimal surface-area refactors while preserving existing behavior and call flow.Safe update enforcement: collapse event/redirect/script inputs into options
SafeUpdateEnforcementOptionsinpkg/workflow/safe_update_enforcement.go.EnforceSafeUpdate(...)to take the options struct instead of multiple scalar parameters.Repository package extension resolution: group repository location inputs
repositoryPackageLocationinpkg/cli/add_package_manifest.go.resolveRepositoryPackageExtensionFiles(...)to accept the location struct instead of separateowner/repo/path/ref/hostargs.Scope discipline