refactor: extract named run handlers, replace globals with option str…#668
Merged
Conversation
…ucts
- Convert gitlab/renovate/enum and github/renovate/enum from package-level
global variables to local EnumOptions structs in the Run handler; business
logic now lives in a pure enumerate(opts) function that is testable without
Cobra
- Extract inline Run: func(...) closures into named handler functions across
all remaining command files:
internal/cmd/docs/docs.go
internal/cmd/gitea/secrets/secrets.go
internal/cmd/gitea/variables/variables.go
internal/cmd/github/container/artipacked/artipacked.go
internal/cmd/github/ghtoken/exploit/exploit.go
internal/cmd/github/renovate/autodiscovery/autodiscovery.go
internal/cmd/gitlab/jobToken/exploit/exploit.go
internal/cmd/gitlab/register/register.go
internal/cmd/gitlab/runners/exploit/exploit.go
(plus earlier-refactored files: cicd/yaml, container/artipacked,
renovate/autodiscovery, renovate/bots, renovate/privesc,
runners/list, shodan, renovate/lab, renovate/privesc)
- Add/update unit tests asserting Run field references the named handler
via reflect.ValueOf pointer comparison:
internal/cmd/gitlab/renovate/enum/enum_test.go
internal/cmd/github/renovate/enum/enum_unit_test.go
- Update .github/copilot-instructions.md to codify the Named Run Handler
Pattern as MANDATORY: named handler rule, options-struct rule, inline-func
and global-variable anti-patterns, checklist items 6-7, and DO NOT bullets
All unit and e2e tests pass.
Contributor
There was a problem hiding this comment.
Pull request overview
This PR refactors Pipeleek’s Cobra command implementations to standardize on named Run handlers and (for the Renovate enum commands) to move command state into local options structs, making core logic callable/testable without Cobra and reducing shared mutable state.
Changes:
- Replaces inline anonymous
Run: func(...) { ... }closures with named handler functions across multiple commands. - Refactors GitLab/GitHub Renovate
enumcommands to build localEnumOptionsand call a pureenumerate(opts)function. - Adds/updates unit tests to assert that
cmd.Runreferences the named handler viareflect.ValueOf(...).Pointer(), and updates Copilot instructions to mandate the pattern.
Reviewed changes
Copilot reviewed 25 out of 25 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/e2e/gitlab/jobtoken/secure-files-dc58/secret.txt | Adds an e2e fixture secure-file payload. |
| tests/e2e/gitlab/jobtoken/secure-files-2ae5/secret.txt | Adds an e2e fixture secure-file payload. |
| internal/cmd/gitlab/shodan/shodan.go | Extracts Run into RunShodan. |
| internal/cmd/gitlab/runners/list/list.go | Extracts Run into RunListRunners. |
| internal/cmd/gitlab/runners/exploit/exploit.go | Extracts Run into RunRunnersExploit. |
| internal/cmd/gitlab/renovate/privesc/privesc.go | Extracts Run into RunPrivesc and removes package-level flag globals. |
| internal/cmd/gitlab/renovate/enum/enum.go | Introduces EnumOptions + pure enumerate(opts) and a named RunEnumerate. |
| internal/cmd/gitlab/renovate/enum/enum_test.go | Adds reflect-based assertion that cmd.Run is RunEnumerate. |
| internal/cmd/gitlab/renovate/bots/bots.go | Extracts Run into RunBots and removes package-level flag global. |
| internal/cmd/gitlab/renovate/autodiscovery/autodiscovery.go | Extracts Run into RunAutodiscovery and removes package-level flag globals. |
| internal/cmd/gitlab/register/register.go | Extracts Run into RunRegister. |
| internal/cmd/gitlab/jobToken/exploit/exploit.go | Extracts Run into RunExploit. |
| internal/cmd/gitlab/container/artipacked/artipacked.go | Extracts Run into RunArtipacked (still uses package-level flag globals). |
| internal/cmd/gitlab/cicd/yaml/yaml.go | Extracts Run into RunYamlCommand (flag shorthand behavior changed). |
| internal/cmd/github/renovate/privesc/privesc.go | Extracts Run into RunPrivesc and removes package-level flag globals. |
| internal/cmd/github/renovate/lab/lab.go | Extracts Run into RunLabSetupCommand and removes package-level flag global. |
| internal/cmd/github/renovate/enum/enum.go | Introduces EnumOptions + pure enumerate(opts) and a named RunEnumerate. |
| internal/cmd/github/renovate/enum/enum_unit_test.go | Adds reflect-based assertion that cmd.Run is RunEnumerate. |
| internal/cmd/github/renovate/autodiscovery/autodiscovery.go | Extracts Run into RunAutodiscovery (still uses package-level flag globals). |
| internal/cmd/github/ghtoken/exploit/exploit.go | Extracts Run into RunExploit. |
| internal/cmd/github/container/artipacked/artipacked.go | Extracts Run into RunArtipacked (still uses package-level flag globals). |
| internal/cmd/gitea/variables/variables.go | Extracts Run into RunVariables. |
| internal/cmd/gitea/secrets/secrets.go | Extracts Run into RunSecrets. |
| internal/cmd/docs/docs.go | Refactors docs command to use a bound method value as Run (not a package-level handler function). |
| .github/copilot-instructions.md | Documents the “Named Run Handler Pattern” as mandatory and forbids package-level flag globals. |
Unit tests (handler-pointer assertions): - Add reflect.ValueOf pointer assertion to 13 command test files that had no run-handler check: gitea/secrets, gitea/variables, gitlab/cicd/yaml, gitlab/runners/list, gitlab/shodan, gitlab/renovate/bots, github/container/artipacked, github/ghtoken/exploit, github/renovate/lab, gitlab/register, gitlab/runners/exploit, gitlab/container/artipacked, gitlab/jobToken/exploit - Upgrade 4 existing TestXxxCmdHasRun tests from assert.NotNil(cmd.Run) to the stronger reflect.ValueOf pointer comparison: github/renovate/autodiscovery, github/renovate/privesc, gitlab/renovate/autodiscovery, gitlab/renovate/privesc Config loader (DRY): - Extract duplicated 10-line home-directory resolution logic in pkg/config/loader.go into a single resolveHomeDir() helper - Replace both call sites (LoadConfig and GetEffectiveConfigPath) with the helper; behaviour is identical All unit tests pass.
added 2 commits
July 1, 2026 11:50
A - gitlab/cicd/yaml: restore -r shorthand on --repo flag (breaking change fix) B - gitlab/container/artipacked: replace 7 package-level globals with local artipackedOptions struct; RunArtipacked builds opts locally and calls pure scan(opts); NewArtipackedCmd declares flag vars locally C - github/container/artipacked: same pattern as B; replace 9 package-level globals (owned/member/public/etc.) with local artipackedOptions struct; Scan() replaced by pure scan(opts) D - github/renovate/autodiscovery: remove autodiscoveryRepoName/ autodiscoveryUsername package-level globals; RunAutodiscovery now reads config values into local vars and passes them directly E - docs: replace bound-method runner.Run with package-level RunDocs handler; docsRoot package var holds root command (set once on init, never mutated); flags read via cmd.Flags().GetBool() inside RunDocs F - pkg/config/loader.go: resolveHomeDir() now logs the os.UserHomeDir() error at debug level instead of silently discarding it All unit tests pass.
Item 2 - copilot-instructions.md: document docsRoot as intentional exception to the no-package-level-globals rule. The docs command must hold a root *cobra.Command reference that is only available at construction time (not via config); the variable is set once in NewDocsCmd and never mutated, so it does not introduce shared flag state between runs. Item 3 - scanner interfaces: already aligned (all 8 platform scanner packages embed pkgscanner.BaseScanner). No code change required; confirmed by audit. Item 4 - gitlab/runners/exploit: consolidate post-MustBind URL/token validation into the AddValidator chain. ValidateURL and ValidateToken are now called inside the validator closure (guarded by the existing dry-run check) and MustBind exits early on failure. The handler body is simplified: gitlabURL/gitlabAPIToken are declared as empty strings and populated only when dry=false, then passed to ExploitRunners unconditionally.
A+B - github/container/artipacked: remove DangerousPatterns field from artipackedOptions struct and from the pkgcontainer.ScanOptions literal in scan(). The field was never populated (no flag binding or config read) and the underlying pkg implementation ignores it, making it misleading. C - docs: replace BoolVarP(new(bool),...) with BoolP(...). Since RunDocs reads flag values via cmd.Flags().GetBool(), the destination pointer from BoolVarP was never used; BoolP is the idiomatic form when the handler reads the value directly from the command.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
…ucts
Convert gitlab/renovate/enum and github/renovate/enum from package-level global variables to local EnumOptions structs in the Run handler; business logic now lives in a pure enumerate(opts) function that is testable without Cobra
Extract inline Run: func(...) closures into named handler functions across all remaining command files: internal/cmd/docs/docs.go internal/cmd/gitea/secrets/secrets.go internal/cmd/gitea/variables/variables.go internal/cmd/github/container/artipacked/artipacked.go internal/cmd/github/ghtoken/exploit/exploit.go internal/cmd/github/renovate/autodiscovery/autodiscovery.go internal/cmd/gitlab/jobToken/exploit/exploit.go internal/cmd/gitlab/register/register.go internal/cmd/gitlab/runners/exploit/exploit.go (plus earlier-refactored files: cicd/yaml, container/artipacked, renovate/autodiscovery, renovate/bots, renovate/privesc, runners/list, shodan, renovate/lab, renovate/privesc)
Add/update unit tests asserting Run field references the named handler via reflect.ValueOf pointer comparison: internal/cmd/gitlab/renovate/enum/enum_test.go internal/cmd/github/renovate/enum/enum_unit_test.go
Update .github/copilot-instructions.md to codify the Named Run Handler Pattern as MANDATORY: named handler rule, options-struct rule, inline-func and global-variable anti-patterns, checklist items 6-7, and DO NOT bullets
All unit and e2e tests pass.