Refactor safe-outputs config pipeline to eliminate largefunc hotspots#53278
Conversation
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
|
✅ Design Decision Gate 🏗️ completed the design decision gate check.
|
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅
|
|
✅ PR Code Quality Reviewer completed the code quality review.
|
|
✅ Ponytail Reviewer completed successfully!
|
|
✅ Test Quality Sentinel completed test quality analysis. No test files were added or modified in this PR. Test Quality Sentinel skipped.
|
Comment MemoryNote This comment is managed by comment memory.It stores persistent context for this thread in the code block at the top of this comment.
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Request changes
This cleanup is not behavior-preserving: the safe script config path now drops script-level max, which removes a runtime guard that workflows can rely on to limit tool invocations.
Blocking theme
pkg/workflow/safe_outputs_config_generation.go:addSafeScriptsConfignow calls the shared builder withmaxhard-coded to0, so generated config no longer carriessafe-outputs.scripts.<name>.max.- That changes runtime enforcement, not just structure, because the generated config is what the safe-output machinery uses to cap calls.
Everything else in this PR looked like straightforward extraction/refactoring, but this regression is enough to block merge.
🔎 Code quality review by PR Code Quality Reviewer · gpt54 · 11.1 AIC · ⌖ 7.36 AIC · ⊞ 6.9K
Comment /review to run again
| for scriptName, scriptConfig := range safeOutputs.Scripts { | ||
| normalizedName := stringutil.NormalizeSafeOutputIdentifier(scriptName) | ||
| safeOutputsConfigLog.Printf("Generating config for safe script: %s (normalized: %s)", scriptName, normalizedName) | ||
| safeOutputsConfig[normalizedName] = buildSafeOutputInputsConfig(scriptConfig.Description, "", 0, scriptConfig.Inputs) |
There was a problem hiding this comment.
This refactor silently drops max from every safe script config because addSafeScriptsConfig hard-codes buildSafeOutputInputsConfig(..., "", 0, ...), so any script-level invocation limit in frontmatter will stop being enforced at runtime.
💡 Why this blocks merge
Before this change, safe scripts copied their input definitions inline and preserved the rest of the script config structure. After the extraction, the shared helper is called with max forced to 0, and buildSafeOutputInputsConfig only emits max when it is greater than zero. That means a workflow author can still declare safe-outputs.scripts.<name>.max, but the generated config.json will never contain it.
That is not cosmetic: max is the abuse-prevention guard for custom tools. Losing it changes runtime behavior and can let an agent call a script more times than the workflow author allowed.
A minimal fix is to thread scriptConfig.Max through the helper exactly like jobs do:
safeOutputsConfig[normalizedName] = buildSafeOutputInputsConfig(
scriptConfig.Description,
"",
scriptConfig.Max,
scriptConfig.Inputs,
)
There was a problem hiding this comment.
This PR is a well-structured refactoring that breaks the monolithic extractSafeOutputsConfig and generateSafeOutputsConfig functions into focused, named helpers — a clear improvement in readability and maintainability with no behavior changes.
One stale comment to update: the validateSafeOutputsMax docstring still says "direct struct field access — no reflection, no heap allocation" but the new implementation builds a []maxFieldCheck slice (a heap allocation). The comment should be updated to reflect the new structure.
Otherwise the refactoring is correct and safe.
🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · sonnet46 · 44.8 AIC · ⌖ 7.88 AIC · ⊞ 5.6K
There was a problem hiding this comment.
Pull request overview
Refactors the safe-outputs configuration pipeline into smaller helpers while preserving existing interfaces and behavior.
Changes:
- Decomposes configuration extraction, parsing, generation, and validation.
- Replaces enabled-tool conditionals with declarative checks.
- Retains deterministic validation and generated configuration structure.
Show a summary per file
| File | Description |
|---|---|
pkg/workflow/safe_outputs_config_extraction.go |
Splits handler extraction and defaulting phases. |
pkg/workflow/safe_outputs_config_global.go |
Separates global configuration parsing by domain. |
pkg/workflow/safe_outputs_max_validation.go |
Groups standard and repository max validation. |
pkg/workflow/safe_outputs_config_generation.go |
Extracts focused configuration builders. |
pkg/workflow/safe_outputs_tools_computation.go |
Introduces declarative enabled-tool checks. |
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: 2
- Review effort level: Balanced
| } | ||
|
|
||
| func buildStandardMaxChecks(config *SafeOutputsConfig) []maxFieldCheck { | ||
| checks := []maxFieldCheck{} |
| func predefinedToolChecks(safeOutputs *SafeOutputsConfig) []toolEnabledCheck { | ||
| return []toolEnabledCheck{ |
🏗️ Design Decision Gate — ADR RequiredThis PR makes significant changes to core business logic (730 new lines in 📄 Draft ADR committed:
📋 What to do next
Once an ADR is linked in the PR body, this gate will re-run and verify the implementation matches the decision. ❓ Why ADRs Matter
ADRs create a searchable, permanent record of why the codebase looks the way it does. Future contributors (and your future self) will thank you. 📋 Michael Nygard ADR Format ReferenceAn ADR must contain these four sections to be considered complete:
All ADRs are stored in
|
There was a problem hiding this comment.
Ponytail review — over-engineering only. This PR is a mechanical size-reduction refactor and mostly reduces net lines well; findings below are places where the split introduces new duplication/indirection instead of just moving code.
net: -75 lines possible (fold appendChecksGroup1-5 back into one function, and reuse safeOutputHandlers/safeOutputFieldMapping instead of predefinedToolChecks/toolEnabledCheck, ~75 lines combined).
Generated by ✂️ Ponytail Reviewer for #53278 · auto · 69.1 AIC · ⌖ 5.1 AIC · ⊞ 7.2K
Comment /ponytail to run again
| return nil | ||
| } | ||
|
|
||
| func buildStandardMaxChecks(config *SafeOutputsConfig) []maxFieldCheck { |
There was a problem hiding this comment.
L66-225: yagni: buildStandardMaxChecks split into appendChecksGroup1..5, purely to keep function size down, not by domain. Inline as one loop over safeOutputFieldMapping (already maps struct field name -> tool name) instead of 5 arbitrarily-numbered functions + a maxFieldCheck slice.
| } | ||
| } | ||
|
|
||
| func predefinedToolChecks(safeOutputs *SafeOutputsConfig) []toolEnabledCheck { |
There was a problem hiding this comment.
L20-56: yagni: predefinedToolChecks() manually re-lists every SafeOutputsConfig field -> tool-name mapping that safeOutputHandlers/safeOutputFieldMapping already encodes (buildSafeOutputFieldMapping in safe_output_handlers.go). Drive computeEnabledToolNames off that existing registry instead of a second hand-maintained []toolEnabledCheck.
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /codebase-design — requesting changes on two actionable issues.
📋 Key Themes & Highlights
Key Themes
- Opaque group names (
appendChecksGroup1–appendChecksGroup5): the numeric suffixes don't communicate the alphabetical ranges they cover, making it hard to place new tools without reading all five functions. The existingpredefinedToolCheckspattern (a single flat declarative slice) is the better model — the max-check table should follow suit. - Missing
merge_pull_requestinpredefinedToolChecks: the tool exists in max validation and inSafeOutputsConfig, but is absent from the enabled-tool computation list. The new flat-list structure makes this gap visible for the first time; it should be fixed here. - Thin indirection in
extractAdditionalSafeOutputHandlers: the function is a pure pass-through with no logic of its own, adding a naming layer that readers must trace through. trueValrepetition inextractFallbackSafeOutputHandlers: three separatetrueVal := "true"declarations for what is semantically one constant.
Positive Highlights
- ✅
predefinedToolChecksinsafe_outputs_tools_computation.gois excellent — declarative, flat, easy to audit and extend. - ✅
addStandardHandlerConfigs/addProtectedFilesConfigextraction is clean: each function has a single, well-named concern. - ✅
validateDispatchRepositoryMaxcorrectly isolated the structurally different dispatch-repository path. - ✅
generateSafeOutputsConfigis dramatically more readable without the large inline blocks.
Skill selection: used /codebase-design (PR title signals refactor/cleanup).
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · sonnet46 · 101 AIC · ⌖ 7.31 AIC · ⊞ 7.7K
Comment /matt to run again
| return nil | ||
| } | ||
|
|
||
| func buildStandardMaxChecks(config *SafeOutputsConfig) []maxFieldCheck { |
There was a problem hiding this comment.
[/codebase-design] The group helpers appendChecksGroup1–appendChecksGroup5 use opaque numeric suffixes — the grouping logic (alphabetical ranges: a–c, c–c, c–m, m–r, s–u) is invisible from the call site in buildStandardMaxChecks. When a new tool is added, a contributor must read all five functions to know which group to extend.
💡 Suggested fix
Consider collapsing into a single declarative slice like predefinedToolChecks does in safe_outputs_tools_computation.go. The two tables (tool-enabled checks and max-field checks) are structurally identical — a shared toolMaxCheck struct or just inlining all checks into one function would eliminate the arbitrary grouping entirely and keep the alphabetical ordering self-evident.
@copilot please address this.
| } | ||
| } | ||
|
|
||
| func predefinedToolChecks(safeOutputs *SafeOutputsConfig) []toolEnabledCheck { |
There was a problem hiding this comment.
[/codebase-design] predefinedToolChecks is a package-level function but receives *SafeOutputsConfig — it has no receiver and no connection to the compiler. This is fine for a pure data transformer, but the function name's "predefined" qualifier leaks an implementation detail (the distinction between predefined vs dynamic tools).
💡 Suggested naming
Consider renaming to staticToolEnabledChecks or toolEnabledChecksFrom so the name describes what the function does (maps config fields → enabled flags) rather than categorising its output relative to dynamic tools. The comment on computeEnabledToolNames already explains the predefined/dynamic split.
@copilot please address this.
| } | ||
| } | ||
| } | ||
| func (c *Compiler) extractAdditionalSafeOutputHandlers(outputMap map[string]any, config *SafeOutputsConfig) { |
There was a problem hiding this comment.
[/codebase-design] extractAdditionalSafeOutputHandlers is a thin pass-through that only delegates to three sub-helpers with no logic of its own. It adds a naming layer ("additional") without adding clarity — readers must trace through one extra call to reach the actual work.
💡 Suggested fix
Inline the three calls directly into extractSafeOutputsConfig, replacing the intermediate function:
c.extractCoreSafeOutputHandlers(outputMap, config)
c.extractReviewAndSecurityHandlers(outputMap, config)
c.extractIssueAndPRMutationHandlers(outputMap, config)
c.extractWorkflowDispatchHandlers(outputMap, config)
c.extractFallbackSafeOutputHandlers(outputMap, config)This removes the "core" vs "additional" split that readers have to mentally map, while keeping the semantic grouping visible at the top level.
@copilot please address this.
| } else if _, exists := outputMap["missing-tool"]; !exists { | ||
| trueVal := "true" | ||
| config.MissingTool = &MissingToolConfig{CreateIssue: &trueVal, TitlePrefix: "", Labels: nil} | ||
| } |
There was a problem hiding this comment.
[/codebase-design] trueVal := "true" is re-declared three times in extractFallbackSafeOutputHandlers (lines ~244, ~252, ~271) for separate stack allocations. Each default block also hard-codes the same CreateIssue: &trueVal, TitlePrefix: "", Labels: nil pattern.
💡 Suggested fix
Hoist a single trueVal to the top of the function, or introduce a tiny helper:
func defaultFallbackConfig() (createIssue *string) {
v := "true"
return &v
}This avoids the repeated trueVal := "true" pattern and makes it clear that all three fallback handlers share the same default semantics.
@copilot please address this.
| } | ||
| } | ||
|
|
||
| func predefinedToolChecks(safeOutputs *SafeOutputsConfig) []toolEnabledCheck { |
There was a problem hiding this comment.
[/codebase-design] merge_pull_request is validated in safe_outputs_max_validation.go (as config.MergePullRequest) but is absent from predefinedToolChecks. This asymmetry between the two tables is a pre-existing gap, but this PR is the right moment to close it — the declarative list now makes the omission structurally visible. A missing entry means merge_pull_request is never added to enabledTools even when configured, which can silently affect tool-allowlist generation.
💡 Suggested fix
Add the missing entry to predefinedToolChecks:
{name: "merge_pull_request", enabled: safeOutputs.MergePullRequest != nil},Also audit whether call_workflow and dispatch_workflow are intentionally excluded here (they're in max validation but not in this list — per the function comment they're "generated separately", which should be confirmed).
@copilot please address this.
|
@copilot this PR is ready for the next finishing pass. Please address these items, newest first:
Run: https://github.com/github/gh-aw/actions/runs/31996014734
|
…tation Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
This change addresses the safe-outputs large-function cluster flagged by
golint-customin config extraction, global field parsing, max validation, config generation, and enabled-tool computation. The goal is strictly structural: reduce function size while preserving safe-outputs behavior and interfaces.Safe-outputs extraction decomposition
extractSafeOutputsConfiginto focused internal phases:missing-tool,missing-data,noop,report-incomplete)Global config parsing decomposition
extractGlobalConfigFieldsinto domain-specific helpers:Max validation refactor
validateSafeOutputsMaxinto:Config generation refactor
generateSafeOutputsConfiginto targeted builders for:Enabled tool computation refactor
ifcascade incomputeEnabledToolNameswith declarative check lists + shared add helper.