Refactor parameter-heavy safe-update and package resolution APIs

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.*

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
 	}

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()))

pkg/workflow/compiler_threat_detection_formal_test.go

@@ -9,67 +9,97 @@ 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")
 }
 
 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")

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
 	}
 

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)")