diff --git a/docs/adr/52982-options-struct-for-parameter-heavy-functions.md b/docs/adr/52982-options-struct-for-parameter-heavy-functions.md new file mode 100644 index 00000000000..7da0b2c7ce1 --- /dev/null +++ b/docs/adr/52982-options-struct-for-parameter-heavy-functions.md @@ -0,0 +1,50 @@ +# ADR-52982: Options Structs for Parameter-Heavy Functions + +**Date**: 2026-08-15 +**Status**: Accepted +**Deciders**: pelikhan, copilot-swe-agent + +--- + +### Context + +Two functions in the `pkg/workflow` and `pkg/cli` packages accumulated more parameters than the project's custom linter permits. `EnforceSafeUpdate` had six positional parameters (manifest, secret names, action refs, redirect string, PR transition, memory-validation scripts), and `resolveRepositoryPackageExtensionFiles` had nine (context plus eight domain values). The custom parameter-count linter flagged both as violations in CI, requiring a fix before the branch could merge. + +Positional parameter lists of this length are brittle: callers must supply all arguments in the correct order, zero values (empty string, nil slice) are indistinguishable from intentional omissions, and adding a new parameter requires updating every call site. + +### Decision + +We will consolidate the excess parameters of each function into a dedicated options struct (`SafeUpdateOptions` for `EnforceSafeUpdate`, and `repositoryPackageExtensionFilesOptions` for `resolveRepositoryPackageExtensionFiles`). Each function now accepts a context (where applicable) and a single options value. All existing call sites are updated to use named struct-literal syntax. The security semantics and internal logic of both functions remain unchanged. + +### Alternatives Considered + +#### Alternative 1: Raise or disable the linter parameter-count threshold + +The linter threshold could be increased or the specific functions could be annotated to suppress the check. This would silence the CI failure without changing the API surface. + +Not chosen because it defeats the purpose of the lint rule: long positional parameter lists remain a maintenance liability regardless of whether the linter ignores them, and relaxing the threshold for individual functions normalizes an anti-pattern that is already causing issues. + +#### Alternative 2: Decompose functions into smaller units + +`EnforceSafeUpdate` could be split into separate functions for secret enforcement, action enforcement, redirect enforcement, and memory-script enforcement, each taking only the parameters it needs. + +Not chosen because the six checks are tightly coupled — they share the same manifest baseline and must all pass before a safe-update review can be approved. Splitting them would require callers to coordinate multiple calls and aggregate errors, increasing complexity at every call site without a clear architectural gain. + +### Consequences + +#### Positive +- Function signatures are stable: adding a new enforcement input only requires a new field on the options struct; existing call sites compile unchanged. +- Named fields at call sites make arguments self-documenting and eliminate ordering errors. +- Zero-value fields express deliberate omission clearly (e.g., `nil` Manifest conveys "no lock file" by struct default). + +#### Negative +- Struct-literal call sites are more verbose than the previous positional style, especially for callers that pass most fields. +- The options struct is a public type (`SafeUpdateOptions`), so it becomes part of the package API surface; future field additions are additive but field removals are breaking changes. + +#### Neutral +- Test files required mechanical updates to switch from positional arguments to named struct fields; test coverage and assertions are unchanged. +- The `repositoryPackageExtensionFilesOptions` struct is unexported, so its scope is limited to the `cli` package and carries no external API obligations. + +--- + +*ADR created by [adr-writer agent]. Finalized and accepted as part of PR #52982.* diff --git a/pkg/cli/add_package_manifest.go b/pkg/cli/add_package_manifest.go index a31e3ec6199..d5326107b69 100644 --- a/pkg/cli/add_package_manifest.go +++ b/pkg/cli/add_package_manifest.go @@ -105,7 +105,16 @@ func resolveRepositoryPackage(ctx context.Context, repoSpec *RepoSpec, host stri return nil, err } - extensionFiles, err := resolveRepositoryPackageExtensionFiles(ctx, owner, repo, packagePath, ref, host, manifest, includeSkillDirs, includeAgentFiles) + extensionFiles, err := resolveRepositoryPackageExtensionFiles(ctx, repositoryPackageExtensionFilesOptions{ + owner: owner, + repo: repo, + packagePath: packagePath, + ref: ref, + host: host, + manifest: manifest, + includeSkillDirs: includeSkillDirs, + includeAgentFiles: includeAgentFiles, + }) if err != nil { return nil, err } @@ -172,19 +181,30 @@ type repositoryPackageExtensionFiles struct { warnings []string } -func resolveRepositoryPackageExtensionFiles(ctx context.Context, owner, repo, packagePath, ref, host string, manifest *repositoryPackageManifest, includeSkillDirs, includeAgentFiles []string) (*repositoryPackageExtensionFiles, error) { +type repositoryPackageExtensionFilesOptions struct { + owner string + repo string + packagePath string + ref string + host string + manifest *repositoryPackageManifest + includeSkillDirs []string + includeAgentFiles []string +} + +func resolveRepositoryPackageExtensionFiles(ctx context.Context, options repositoryPackageExtensionFilesOptions) (*repositoryPackageExtensionFiles, error) { // Resolve skill files: explicit from manifest or auto-scanned. - explicitSkillDirs := append([]string{}, manifest.Skills...) - explicitSkillDirs = append(explicitSkillDirs, includeSkillDirs...) - skillFiles, skillWarnings, err := resolvePackageSkillFiles(ctx, owner, repo, packagePath, ref, host, explicitSkillDirs) + explicitSkillDirs := append([]string{}, options.manifest.Skills...) + explicitSkillDirs = append(explicitSkillDirs, options.includeSkillDirs...) + skillFiles, skillWarnings, err := resolvePackageSkillFiles(ctx, options.owner, options.repo, options.packagePath, options.ref, options.host, explicitSkillDirs) if err != nil { return nil, err } // Resolve agent files: explicit from manifest or auto-scanned. - explicitAgentFiles := append([]string{}, manifest.Agents...) - explicitAgentFiles = append(explicitAgentFiles, includeAgentFiles...) - agentFiles, agentWarnings, err := resolvePackageAgentFiles(ctx, owner, repo, packagePath, ref, host, explicitAgentFiles) + explicitAgentFiles := append([]string{}, options.manifest.Agents...) + explicitAgentFiles = append(explicitAgentFiles, options.includeAgentFiles...) + agentFiles, agentWarnings, err := resolvePackageAgentFiles(ctx, options.owner, options.repo, options.packagePath, options.ref, options.host, explicitAgentFiles) if err != nil { return nil, err } diff --git a/pkg/workflow/compiler.go b/pkg/workflow/compiler.go index 15d09bc19f7..ba08dcea2ff 100644 --- a/pkg/workflow/compiler.go +++ b/pkg/workflow/compiler.go @@ -574,7 +574,14 @@ func (c *Compiler) CompileWorkflowData(workflowData *WorkflowData, markdownPath CurrentHasPullRequest: currentHasPR, CurrentHasPullRequestTarget: currentHasPRTarget, } - if enforceErr := EnforceSafeUpdate(oldManifest, bodySecrets, bodyActions, workflowData.Redirect, prTransition, collectMemoryValidationScripts(workflowData)); enforceErr != nil { + if enforceErr := EnforceSafeUpdate(SafeUpdateOptions{ + Manifest: oldManifest, + SecretNames: bodySecrets, + ActionRefs: bodyActions, + CurrentRedirect: workflowData.Redirect, + PullRequestTransition: prTransition, + MemoryValidationScripts: collectMemoryValidationScripts(workflowData), + }); enforceErr != nil { warningMsg := buildSafeUpdateWarningPrompt(enforceErr.Error()) c.AddSafeUpdateWarning(warningMsg) fmt.Fprintln(os.Stderr, formatCompilerMessage(markdownPath, "warning", enforceErr.Error())) diff --git a/pkg/workflow/compiler_threat_detection_formal_test.go b/pkg/workflow/compiler_threat_detection_formal_test.go index 9215978bbff..76f511b516d 100644 --- a/pkg/workflow/compiler_threat_detection_formal_test.go +++ b/pkg/workflow/compiler_threat_detection_formal_test.go @@ -9,47 +9,68 @@ import ( ) func TestFormal_CTR016_NilManifestSkipsEnforcement(t *testing.T) { - err := EnforceSafeUpdate(nil, []string{"MY_SECRET"}, []string{"evil-org/action@deadbeef # v1"}, "", PullRequestEventTransition{}, nil) + err := EnforceSafeUpdate(SafeUpdateOptions{ + SecretNames: []string{"MY_SECRET"}, + ActionRefs: []string{"evil-org/action@deadbeef # v1"}, + }) require.NoError(t, err) } func TestFormal_CTR016_EmptyManifestRejectsNewSecret(t *testing.T) { - err := EnforceSafeUpdate(&GHAWManifest{Version: currentGHAWManifestVersion}, []string{"MY_SECRET"}, nil, "", PullRequestEventTransition{}, nil) + err := EnforceSafeUpdate(SafeUpdateOptions{ + Manifest: &GHAWManifest{Version: currentGHAWManifestVersion}, + SecretNames: []string{"MY_SECRET"}, + }) require.Error(t, err) require.ErrorContains(t, err, "MY_SECRET") } func TestFormal_CTR016_GitHubTokenExempt_BareForm(t *testing.T) { - err := EnforceSafeUpdate(&GHAWManifest{Version: currentGHAWManifestVersion}, []string{"GITHUB_TOKEN"}, nil, "", PullRequestEventTransition{}, nil) + err := EnforceSafeUpdate(SafeUpdateOptions{ + Manifest: &GHAWManifest{Version: currentGHAWManifestVersion}, + SecretNames: []string{"GITHUB_TOKEN"}, + }) require.NoError(t, err) } func TestFormal_CTR016_GitHubTokenExempt_PrefixedForm(t *testing.T) { - err := EnforceSafeUpdate(&GHAWManifest{Version: currentGHAWManifestVersion}, []string{"secrets.GITHUB_TOKEN"}, nil, "", PullRequestEventTransition{}, nil) + err := EnforceSafeUpdate(SafeUpdateOptions{ + Manifest: &GHAWManifest{Version: currentGHAWManifestVersion}, + SecretNames: []string{"secrets.GITHUB_TOKEN"}, + }) require.NoError(t, err) } func TestFormal_CTR016_GhAwInternalSecretExempt(t *testing.T) { - err := EnforceSafeUpdate(&GHAWManifest{Version: currentGHAWManifestVersion}, []string{"GH_AW_GITHUB_TOKEN"}, nil, "", PullRequestEventTransition{}, nil) + err := EnforceSafeUpdate(SafeUpdateOptions{ + Manifest: &GHAWManifest{Version: currentGHAWManifestVersion}, + SecretNames: []string{"GH_AW_GITHUB_TOKEN"}, + }) require.NoError(t, err) } func TestFormal_CTR016_SecretPrefixNormalization(t *testing.T) { manifest := &GHAWManifest{Version: currentGHAWManifestVersion, Secrets: []string{"MY_SECRET"}} - err := EnforceSafeUpdate(manifest, []string{"secrets.MY_SECRET"}, nil, "", PullRequestEventTransition{}, nil) + err := EnforceSafeUpdate(SafeUpdateOptions{ + Manifest: manifest, + SecretNames: []string{"secrets.MY_SECRET"}, + }) require.NoError(t, err) } func TestFormal_CTR016_NewActionDriftRejected(t *testing.T) { manifest := &GHAWManifest{Version: currentGHAWManifestVersion, Actions: []GHAWManifestAction{{Repo: "actions/checkout", SHA: "abc1234", Version: "v4"}}} - err := EnforceSafeUpdate(manifest, nil, []string{"actions/checkout@abc1234 # v4", "evil-org/steal@deadbeef # v1"}, "", PullRequestEventTransition{}, nil) + err := EnforceSafeUpdate(SafeUpdateOptions{ + Manifest: manifest, + ActionRefs: []string{"actions/checkout@abc1234 # v4", "evil-org/steal@deadbeef # v1"}, + }) require.Error(t, err) require.ErrorContains(t, err, "evil-org/steal") } func TestFormal_CTR016_RemovedActionDriftRejected(t *testing.T) { manifest := &GHAWManifest{Version: currentGHAWManifestVersion, Actions: []GHAWManifestAction{{Repo: "my-org/approved-action", SHA: "abc1234", Version: "v1"}}} - err := EnforceSafeUpdate(manifest, nil, []string{}, "", PullRequestEventTransition{}, nil) + err := EnforceSafeUpdate(SafeUpdateOptions{Manifest: manifest}) require.Error(t, err) require.ErrorContains(t, err, "Previously-approved action") require.ErrorContains(t, err, "my-org/approved-action") @@ -57,19 +78,28 @@ func TestFormal_CTR016_RemovedActionDriftRejected(t *testing.T) { func TestFormal_CTR016_KnownActionPinUpdateAllowed(t *testing.T) { manifest := &GHAWManifest{Version: currentGHAWManifestVersion, Actions: []GHAWManifestAction{{Repo: "my-org/action", SHA: "abc1234", Version: "v1"}}} - err := EnforceSafeUpdate(manifest, nil, []string{"my-org/action@def5678 # v2"}, "", PullRequestEventTransition{}, nil) + err := EnforceSafeUpdate(SafeUpdateOptions{ + Manifest: manifest, + ActionRefs: []string{"my-org/action@def5678 # v2"}, + }) require.NoError(t, err) } func TestFormal_CTR016_RedirectWhitespaceNormalization(t *testing.T) { manifest := &GHAWManifest{Version: currentGHAWManifestVersion, Redirect: "owner/repo/workflows/new.md@main"} - err := EnforceSafeUpdate(manifest, nil, nil, " owner/repo/workflows/new.md@main ", PullRequestEventTransition{}, nil) + err := EnforceSafeUpdate(SafeUpdateOptions{ + Manifest: manifest, + CurrentRedirect: " owner/repo/workflows/new.md@main ", + }) require.NoError(t, err) } func TestFormal_CTR016_RedirectChangeRejected(t *testing.T) { manifest := &GHAWManifest{Version: currentGHAWManifestVersion, Redirect: "owner/repo/workflows/old.md@main"} - err := EnforceSafeUpdate(manifest, nil, nil, "owner/repo/workflows/new.md@main", PullRequestEventTransition{}, nil) + err := EnforceSafeUpdate(SafeUpdateOptions{ + Manifest: manifest, + CurrentRedirect: "owner/repo/workflows/new.md@main", + }) require.Error(t, err) require.ErrorContains(t, err, "New redirect configured") require.ErrorContains(t, err, "Previously-approved redirect removed") diff --git a/pkg/workflow/safe_update_enforcement.go b/pkg/workflow/safe_update_enforcement.go index 234491fb219..0875e6ef768 100644 --- a/pkg/workflow/safe_update_enforcement.go +++ b/pkg/workflow/safe_update_enforcement.go @@ -39,10 +39,20 @@ type PullRequestEventTransition struct { CurrentHasPullRequestTarget bool } +// SafeUpdateOptions contains the inputs used to validate a safe update. +type SafeUpdateOptions struct { + Manifest *GHAWManifest + SecretNames []string + ActionRefs []string + CurrentRedirect string + PullRequestTransition PullRequestEventTransition + MemoryValidationScripts []GHAWManifestMemoryValidationScript +} + // EnforceSafeUpdate validates that no new restricted secrets or unapproved action // changes have been introduced compared to those recorded in the existing manifest. // -// manifest is the gh-aw-manifest extracted from the current lock file before +// Manifest is the gh-aw-manifest extracted from the current lock file before // recompilation. // // - nil means a lock file was found but it predates the safe-updates feature @@ -52,31 +62,31 @@ type PullRequestEventTransition struct { // baseline to compare against. Pass &GHAWManifest{} when no lock file // exists yet (first compilation); all new secrets/actions will be flagged. // -// secretNames contains the raw names produced by CollectSecretReferences (i.e. +// SecretNames contains the raw names produced by CollectSecretReferences (i.e. // they may or may not carry the "secrets." prefix; both forms are normalized // via normalizeSecretName before comparison). // -// actionRefs contains the raw action reference strings produced by CollectActionReferences, +// ActionRefs contains the raw action reference strings produced by CollectActionReferences, // e.g. "actions/checkout@abc1234 # v4". // // Returns a structured, actionable error when violations are found. -func EnforceSafeUpdate(manifest *GHAWManifest, secretNames []string, actionRefs []string, currentRedirect string, prTransition PullRequestEventTransition, currentMemoryValidationScripts []GHAWManifestMemoryValidationScript) error { - if manifest == nil { +func EnforceSafeUpdate(options SafeUpdateOptions) error { + if options.Manifest == nil { // Lock file exists but predates the safe-updates feature (no gh-aw-manifest // section). Skip enforcement so legacy lock files are not flagged on upgrade. safeUpdateLog.Print("Lock file has no gh-aw-manifest; skipping safe update enforcement (legacy lock file)") return nil } - secretViolations := collectSecretViolations(manifest, secretNames) - addedActions, removedActions := collectActionViolations(manifest, actionRefs) - addedRedirect, removedRedirect := collectRedirectViolations(manifest, currentRedirect) - memoryValidationScriptChanges := collectMemoryValidationScriptChanges(manifest, currentMemoryValidationScripts) - pullRequestTargetEscalation := hasPullRequestTargetEscalation(prTransition.OldHasPullRequest, prTransition.OldHasPullRequestTarget, prTransition.CurrentHasPullRequest, prTransition.CurrentHasPullRequestTarget) + secretViolations := collectSecretViolations(options.Manifest, options.SecretNames) + addedActions, removedActions := collectActionViolations(options.Manifest, options.ActionRefs) + addedRedirect, removedRedirect := collectRedirectViolations(options.Manifest, options.CurrentRedirect) + memoryValidationScriptChanges := collectMemoryValidationScriptChanges(options.Manifest, options.MemoryValidationScripts) + pullRequestTargetEscalation := hasPullRequestTargetEscalation(options.PullRequestTransition.OldHasPullRequest, options.PullRequestTransition.OldHasPullRequestTarget, options.PullRequestTransition.CurrentHasPullRequest, options.PullRequestTransition.CurrentHasPullRequestTarget) if len(secretViolations) == 0 && len(addedActions) == 0 && len(removedActions) == 0 && addedRedirect == "" && removedRedirect == "" && len(memoryValidationScriptChanges) == 0 && !pullRequestTargetEscalation { safeUpdateLog.Printf("Safe update check passed (%d secret(s), %d action(s) verified)", - len(secretNames), len(actionRefs)) + len(options.SecretNames), len(options.ActionRefs)) return nil } diff --git a/pkg/workflow/safe_update_enforcement_test.go b/pkg/workflow/safe_update_enforcement_test.go index 1710c4b050c..151c1106ce3 100644 --- a/pkg/workflow/safe_update_enforcement_test.go +++ b/pkg/workflow/safe_update_enforcement_test.go @@ -369,7 +369,13 @@ func TestEnforceSafeUpdate(t *testing.T) { CurrentHasPullRequest: tt.currentHasPR, CurrentHasPullRequestTarget: tt.currentHasPRTarget, } - err := EnforceSafeUpdate(tt.manifest, tt.secretNames, tt.actionRefs, tt.redirect, prTransition, nil) + err := EnforceSafeUpdate(SafeUpdateOptions{ + Manifest: tt.manifest, + SecretNames: tt.secretNames, + ActionRefs: tt.actionRefs, + CurrentRedirect: tt.redirect, + PullRequestTransition: prTransition, + }) if tt.wantErr { require.Error(t, err, "expected safe update enforcement error") for _, msg := range tt.wantErrMsgs { @@ -476,7 +482,11 @@ func TestMemoryValidationScriptChangesRequireSafeUpdateReview(t *testing.T) { "repo-memory:removed (removed)", }, changes) - err := EnforceSafeUpdate(manifest, nil, nil, "", PullRequestEventTransition{}, current) + err := EnforceSafeUpdate(SafeUpdateOptions{ + Manifest: manifest, + PullRequestTransition: PullRequestEventTransition{}, + MemoryValidationScripts: current, + }) require.Error(t, err) require.ErrorContains(t, err, "Memory validation script changes") require.ErrorContains(t, err, "cache-memory:added (added)")