refactor: reorganize workflow and parser packages (4 semantic findings)#35091
refactor: reorganize workflow and parser packages (4 semantic findings)#35091Copilot wants to merge 3 commits into
Conversation
Finding 1: Move misplaced functions out of compiler_safe_outputs.go - parseOnSection, mergeCommandOtherEvents, mergeEventConfig, parseEventTypes → trigger_parser.go - applyDefaultTools → tools.go - isSandboxEnabled → sandbox.go - Retain mergeSafeJobsFromIncludedConfigs and needsGitCommands in compiler_safe_outputs.go Finding 2: Consolidate compiler_safe_outputs_* and safe_outputs_* files - Merge compiler_safe_outputs_env.go into safe_outputs_env.go - Merge compiler_safe_outputs_config.go, compiler_safe_outputs_core.go, and compiler_safe_outputs_handlers.go into safe_outputs_config.go - Delete the three now-empty source files Finding 3: Align MCP rendering file names - Rename mcp_rendering.go → mcp_renderer_factory.go - Move renderSafeOutputsMCPConfigWithOptions and renderAgenticWorkflowsMCPConfigWithOptions from mcp_config_builtin.go into mcp_renderer_builtin.go; leave mcp_config_builtin.go for future type/constant declarations Finding 4: Remove cross-package name collision in pkg/parser - Rename containsExpression → frontmatterValueContainsExpression in include_processor.go to make type-recursive intent explicit Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
This PR reorganizes workflow compiler, safe-outputs, MCP rendering, and parser helpers into more domain-appropriate files without intending functional behavior changes.
Changes:
- Moves trigger parsing, sandbox detection, default tools, and safe-output environment/config helpers into focused files.
- Consolidates
compiler_safe_outputs_*files intosafe_outputs_*files and aligns MCP renderer naming. - Renames the parser frontmatter expression helper to avoid a cross-package semantic name collision.
Show a summary per file
| File | Description |
|---|---|
pkg/workflow/trigger_parser.go |
Receives parseOnSection and related event merge helpers. |
pkg/workflow/tools.go |
Receives applyDefaultTools near tool defaulting logic. |
pkg/workflow/sandbox.go |
Receives isSandboxEnabled. |
pkg/workflow/safe_outputs_env.go |
Receives safe-output env var setup. |
pkg/workflow/safe_outputs_config.go |
Receives safe-output step config, handler registry, and handler manager config helpers. |
pkg/workflow/mcp_renderer_factory.go |
Adds shared MCP renderer factory/helper code under the new file name. |
pkg/workflow/mcp_renderer_builtin.go |
Receives built-in MCP renderer functions. |
pkg/workflow/mcp_config_builtin.go |
Retains only the architectural package documentation. |
pkg/workflow/compiler_safe_outputs.go |
Reduced to safe-job merge and git-command helper logic. |
pkg/workflow/compiler_safe_outputs_handlers.go |
Deleted after handler registry consolidation. |
pkg/workflow/compiler_safe_outputs_env.go |
Deleted after env helper consolidation. |
pkg/workflow/compiler_safe_outputs_core.go |
Deleted after type consolidation. |
pkg/workflow/compiler_safe_outputs_config.go |
Deleted after config helper consolidation. |
pkg/parser/include_processor.go |
Renames the recursive frontmatter expression helper. |
Copilot's findings
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 13/14 changed files
- Comments generated: 1
| compilerSafeOutputsLog.Printf("Merging safe-jobs from included configs: includedCount=%d", len(includedConfigs)) | ||
| result := topSafeJobs | ||
| if result == nil { | ||
| result = make(map[string]*SafeJobConfig) | ||
| } |
|
🧪 Test Quality Sentinel completed test quality analysis. No test files were added or modified in this PR (PR #35091: refactor: reorganize workflow and parser packages). The PR contains only production code reorganization (moving functions between files, renaming/deleting files in pkg/workflow and pkg/parser). Test Quality Sentinel skipped. |
|
✅ PR Code Quality Reviewer completed the code quality review. |
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅ |
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · sonnet46 1.7M
| "maps" | ||
| "path/filepath" | ||
| "strings" | ||
| "encoding/json" |
There was a problem hiding this comment.
[/zoom-out] The import block and function body in this file are missing tab indentation — gofmt -l flags this file. This will cause the make fmt CI gate to fail immediately.
💡 Fix
Run gofmt -w pkg/workflow/compiler_safe_outputs.go (or make fmt) before pushing. The imports should be:
import (
"encoding/json"
"fmt"
"github.com/github/gh-aw/pkg/logger"
)And the function body lines need a leading tab at every level of indentation.
There was a problem hiding this comment.
Warning
Firewall blocked 1 domain
The following domain was blocked by the firewall during workflow execution:
proxy.golang.org
To allow these domains, add them to the
network.allowedlist in your workflow frontmatter:
network:
allowed:
- defaults
- "proxy.golang.org"See Network Configuration for more information.
🔎 Code quality review by PR Code Quality Reviewer · sonnet46 2.9M
| return merged | ||
| // mergeSafeJobsFromIncludedConfigs merges safe-jobs from included safe-outputs configurations | ||
| func (c *Compiler) mergeSafeJobsFromIncludedConfigs(topSafeJobs map[string]*SafeJobConfig, includedConfigs []string) (map[string]*SafeJobConfig, error) { | ||
| compilerSafeOutputsLog.Printf("Merging safe-jobs from included configs: includedCount=%d", len(includedConfigs)) |
There was a problem hiding this comment.
gofmt failure: unindented function body will break CI formatting check.
gofmt -l already flags this file. Every statement inside mergeSafeJobsFromIncludedConfigs and each import path is flush-left (no leading tab), violating Go formatting rules.
💡 Fix
Run:
gofmt -w pkg/workflow/compiler_safe_outputs.goOr manually add a tab before every import path and every statement in the function body. The needsGitCommands function at the bottom of the file has the same problem.
This is the only remaining content in the file after the refactor, so the fix is small but mandatory — make fmt / CI will reject the PR in its current state.
🏗️ Design Decision Gate — ADR RequiredThis PR makes significant changes to core business logic (1,907 new lines across 📄 Draft ADR committed:
📋 What to do next
Once the ADR is linked in the PR body, this gate will re-run and verify the implementation matches the decision. 📌 What the draft ADR captures
Each finding is encoded as a ❓ Why ADRs MatterADRs create a searchable, permanent record of why the codebase looks the way it does. Reorganization decisions in particular benefit from explicit records — without them, the file-naming convention drifts again over time and the next contributor has no anchor explaining which prefix or directory a new function belongs in. 📋 Michael Nygard ADR Format ReferenceAn ADR must contain these four sections to be considered complete:
All ADRs are stored in References:
|
Semantic clustering of 858 non-test Go files identified four organizational issues: functions in files that don't match their domain, inconsistent file-name prefixes creating a split-brain between
compiler_safe_outputs_*andsafe_outputs_*, mixed MCP rendering file-name patterns, and a cross-package unexported function name collision with different semantics.Finding 1 — Misplaced functions in
compiler_safe_outputs.goSix functions moved to domain-appropriate files:
parseOnSection,mergeCommandOtherEvents,mergeEventConfig,parseEventTypescompiler_safe_outputs.gotrigger_parser.goapplyDefaultToolscompiler_safe_outputs.gotools.go(adjacent toapplyDefaultswhich calls it)isSandboxEnabledcompiler_safe_outputs.gosandbox.go(callers:firewall.go,strict_mode_permissions_validation.go)compiler_safe_outputs.gonow contains onlymergeSafeJobsFromIncludedConfigsandneedsGitCommands.Finding 2 — Consolidate
compiler_safe_outputs_*intosafe_outputs_*Three zero-
funcfiles and two thin files eliminated:compiler_safe_outputs_env.go(1 fn) → merged intosafe_outputs_env.gocompiler_safe_outputs_config.go(5 fns) +compiler_safe_outputs_core.go(SafeOutputStepConfigtype) +compiler_safe_outputs_handlers.go(handlerRegistryvar) → merged intosafe_outputs_config.gocompiler_safe_outputs_builder.goretained as-isFinding 3 — Align MCP rendering file names
mcp_rendering.go→mcp_renderer_factory.gorenderSafeOutputsMCPConfigWithOptionsandrenderAgenticWorkflowsMCPConfigWithOptionsout of the misnamedmcp_config_builtin.gointomcp_renderer_builtin.gomcp_config_builtin.gonow holds only the architectural doc comment, reserved for future type/constant declarationsFinding 4 — Remove cross-package name collision in
pkg/parsercontainsExpression(v any) boolinpkg/parser/include_processor.gohad the same name but different semantics fromcontainsExpression(s string) boolinpkg/workflow/expression_patterns.go— the parser version recursively walksmap[string]any/[]anyrather than matching a single string. Renamed tofrontmatterValueContainsExpressionto make the type-recursive intent explicit.