-
Notifications
You must be signed in to change notification settings - Fork 295
Description
Overview
The file pkg/workflow/safe_outputs_config.go has grown to 1077 lines, significantly exceeding the repository's healthy threshold of 800 lines. It currently spans three distinct functional domains mixed together: frontmatter extraction, GitHub App configuration & token generation, and messages/mentions parsing. Splitting it into focused modules will improve maintainability, testability, and discoverability.
Current State
- File:
pkg/workflow/safe_outputs_config.go - Size: 1077 lines
- Test coverage: No test file found (
safe_outputs_config_test.godoes not exist) - Complexity: Medium-high — many parse helpers, YAML step generation with sorting logic, and recursive config merging
Full File Analysis
Functional Domains
Domain 1 — Frontmatter Extraction (lines 1–608)
extractSafeOutputsConfig()— dispatches to ~30 individualparseXxx()helpers and handles top-level scalar fields (allowed-domains, staged, env, github-token, max-patch-size, threat-detection, runs-on, messages, activation-comments, mentions, footer, group-reports, report-failure-as-issue, failure-issue-repo, max-bot-mentions, steps, id-token, concurrency-group, environment, jobs, github-app). This single function is ~500 lines with deeply nestedif/switchblocks.parseBaseSafeOutputConfig()— parses commonmax,github-token, andstagedfields shared across many handler configs.
Domain 2 — GitHub App Configuration & Token Step Generation (lines 610–932)
GitHubAppConfigstruct (type definition)parseAppConfig()— parses app mapmergeAppFromIncludedConfigs()— merges app config from included workflow configsbuildGitHubAppTokenMintStep()— generates YAML steps for minting a GitHub App token; includes repository scope logic (single, multi, wildcard)convertPermissionsToAppTokenFields()— maps GitHub Actions permissions →permission-*action inputs (12 permission types)buildGitHubAppTokenInvalidationStep()— generates YAML steps for revoking the minted tokenbuildActivationAppTokenMintStep()— generates YAML steps for the activation job's app tokenresolveActivationToken()— priority-order resolution of the activation token reference
Domain 3 — Messages & Mentions Configuration (lines 934–1077)
setStringFromMap()— helper to assign string values from a mapparseMessagesConfig()— parses ~15 message template fields from frontmatterparseMentionsConfig()— parses boolean or object-form mentions config (allow-team-members, allow-context, allowed list with@normalization, max)serializeMessagesConfig()— marshals config to JSON for passing as an env var
Complexity Hotspots
extractSafeOutputsConfig()(lines 56–561): ~500 lines with 30+ sequentialifblocks, each calling a different parser. Hard to unit-test in isolation.buildGitHubAppTokenMintStep()(lines 721–786): Conditional YAML generation withsort.Stringsfor deterministic ordering — subtle behaviour that benefits from dedicated tests.convertPermissionsToAppTokenFields()(lines 793–845): 12 sequentialGetcalls — at risk of silent omissions when new permissions are added.
Refactoring Strategy
Proposed File Splits
Based on the semantic analysis, split the file into three focused modules:
-
safe_outputs_config.go(retained, reduced)- Functions:
extractSafeOutputsConfig(),parseBaseSafeOutputConfig() - Responsibility: Frontmatter extraction and dispatch to parse helpers
- Estimated LOC: ~580 → could be further reduced by extracting top-level scalar field parsing into a helper
- Functions:
-
safe_outputs_app_config.go(new file)- Functions/Types:
GitHubAppConfig,parseAppConfig(),mergeAppFromIncludedConfigs(),buildGitHubAppTokenMintStep(),convertPermissionsToAppTokenFields(),buildGitHubAppTokenInvalidationStep(),buildActivationAppTokenMintStep(),resolveActivationToken() - Responsibility: GitHub App token configuration, merging, and YAML step generation
- Estimated LOC: ~320
- Functions/Types:
-
safe_outputs_messages_config.go(new file)- Functions:
setStringFromMap(),parseMessagesConfig(),parseMentionsConfig(),serializeMessagesConfig() - Responsibility: Safe-output messages and mentions configuration parsing and serialization
- Estimated LOC: ~145
- Functions:
Shared Utilities
setStringFromMap() is a small helper used only within the messages domain — it should live in safe_outputs_messages_config.go. If it turns out to be useful elsewhere, promote it to a safe_outputs_util.go.
Interface Abstractions
No new interfaces are strictly required for this split — all types are in the same workflow package. However, consider:
- Grouping the 30+
parseXxxConfig()calls inextractSafeOutputsConfig()into a named helper (e.g.,extractHandlerConfigs()) to improve readability without changing the split.
Test Coverage Plan
Add tests for each new file (currently zero tests exist for this file):
-
safe_outputs_config_test.goTestExtractSafeOutputsConfig_NoSafeOutputs— returns nil when key absentTestExtractSafeOutputsConfig_EmptyMap— returns non-nil with defaults (missing-tool, missing-data, noop)TestExtractSafeOutputsConfig_AllowedDomains— parses domain listTestExtractSafeOutputsConfig_DefaultThreatDetection— auto-applied when safe outputs existTestParseBaseSafeOutputConfig_DefaultMax— default propagated correctlyTestParseBaseSafeOutputConfig_ExpressionMax—$\{\{ ... }}strings accepted- Target coverage: >80%
-
safe_outputs_app_config_test.goTestParseAppConfig_RequiredFields— app-id and private-keyTestParseAppConfig_OptionalOwnerAndRepos— owner, repositories listTestBuildGitHubAppTokenMintStep_SingleRepo— inline formatTestBuildGitHubAppTokenMintStep_MultiRepo— block scalar formatTestBuildGitHubAppTokenMintStep_WildcardRepo— org-wide (no repositories field)TestBuildGitHubAppTokenMintStep_DefaultRepo— falls back togithub.event.repository.nameTestConvertPermissionsToAppTokenFields— all 12 permission types mappedTestBuildGitHubAppTokenInvalidationStep— always-run, conditional on token presenceTestMergeAppFromIncludedConfigs_TopLevelTakesPrecedenceTestMergeAppFromIncludedConfigs_FallsBackToIncluded- Target coverage: >80%
-
safe_outputs_messages_config_test.goTestParseMessagesConfig_AllFields— all 15 template fields parsedTestParseMentionsConfig_BoolTrue/_BoolFalseTestParseMentionsConfig_ObjectForm— allow-team-members, allow-context, allowed, maxTestParseMentionsConfig_AtNormalization—@userstripped touserTestSerializeMessagesConfig_NilReturnsEmptyTestSerializeMessagesConfig_RoundTrip- Target coverage: >80%
Implementation Guidelines
- Preserve Behaviour: All existing functionality must work identically after the split
- Maintain Exports: Keep public API unchanged (
GitHubAppConfig, exported functions) - Add Tests First: Write tests for each new file before moving code
- Incremental Changes: Split one domain at a time and verify compilation between steps
- Run Tests Frequently: Verify
make test-unitpasses after each split - Add Build Tags: Every new
*_test.gofile must have//go:build !integrationat the top - Run
make agent-finish: Required before committing (formatting + lint + tests)
Acceptance Criteria
-
safe_outputs_config.gois reduced to ≤600 lines -
safe_outputs_app_config.gois created (≤350 lines) -
safe_outputs_messages_config.gois created (≤160 lines) - All three new test files created with ≥80% coverage target
- All tests pass (
make test-unit) - Code passes linting (
make lint) - Build succeeds (
make build) - No breaking changes to public API
Additional Context
- Repository Guidelines: Follow patterns in
AGENTS.mdandscratchpad/cli-command-patterns.md - Code Organization: Prefer many small files grouped by functionality (see AGENTS.md)
- Validation Complexity: Target 100–200 lines per file; hard limit 300 for validators
- Testing Patterns: Match existing
pkg/workflow/*_test.gopatterns (table-driven, no mocks) - Related Files:
pkg/workflow/safe_outputs_config_generation.go,pkg/workflow/safe_outputs_types.go
Priority: Medium
Effort: Medium (well-defined split boundaries; no circular deps; ~3 new files + tests)
Expected Impact: Improved maintainability, easier targeted testing, clearer module ownership
References: §22955041823
Generated by Daily File Diet · ◷
- expires on Mar 13, 2026, 1:35 PM UTC