refactor: split compiler_types.go grab-bag into workflow_data.go and safe_outputs_config.go#43071
Conversation
…safe_outputs_config.go 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. ✅ |
|
✅ Test Quality Sentinel completed test quality analysis. No test files were added or modified in this PR. Test Quality Sentinel skipped. |
|
✅ PR Code Quality Reviewer completed the code quality review. |
There was a problem hiding this comment.
Pull request overview
This PR refactors pkg/workflow/compiler_types.go by splitting out two clusters of types into more focused files: core workflow domain data (WorkflowData + related skip-if configs) and safe-output configuration types, aligning type definitions with their primary parsing/extraction logic.
Changes:
- Introduced
pkg/workflow/workflow_data.gocontainingWorkflowData,PinContext(), andSkipIf*Configtypes. - Moved safe-output config types (e.g.,
SafeOutputsConfig,MentionsConfig,SecretMaskingConfig) into the top ofpkg/workflow/safe_outputs_config.go. - Reduced
pkg/workflow/compiler_types.goto compiler construction/options and removed the unusedactionpinsimport.
Show a summary per file
| File | Description |
|---|---|
| pkg/workflow/workflow_data.go | New home for WorkflowData and closely-related workflow frontmatter domain structs/methods. |
| pkg/workflow/safe_outputs_config.go | Co-locates safe-output configuration type definitions with the existing parsing/extraction code. |
| pkg/workflow/compiler_types.go | Slimmed down to compiler types/options; removes moved domain/config definitions. |
Review details
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comments suppressed due to low confidence (1)
pkg/workflow/compiler_types.go:462
- Dangling comment at end of file refers to SkipIfMatchConfig, but the type was moved to workflow_data.go. This leaves an orphaned/misleading comment in compiler_types.go after the refactor.
// SkipIfMatchConfig holds the configuration for skip-if-match conditions
- Files reviewed: 3/3 changed files
- Comments generated: 0
- Review effort level: Low
🏗️ Design Decision Gate — ADR RequiredThis PR makes significant changes to core business logic (388 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
📋 Michael Nygard ADR Format ReferenceAn ADR must contain these four sections to be considered complete:
All ADRs are stored in
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review: Pure mechanical file split ✅
This is a clean refactor with no logic changes — splitting an 835-line grab-bag into three focused files. All imports in the new files are used and correct; getDefaultGHHost() referenced in PinContext() is available within the same package.
One minor nit (non-blocking, inline): add a blank line between SkipIfCheckFailingConfig and WorkflowData in workflow_data.go — the new file is a good chance to tidy what the original skipped.
No logic issues, no correctness concerns.
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.
🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · 68 AIC · ⌖ 5.96 AIC · ⊞ 4.9K
| Branch string // optional branch name to check (defaults to triggering ref or PR base branch) | ||
| AllowPending bool // if true, pending/in-progress checks are not treated as failing (default: treat pending as failing) | ||
| } | ||
| type WorkflowData struct { |
There was a problem hiding this comment.
Missing blank line between SkipIfCheckFailingConfig and WorkflowData struct definitions. The original compiler_types.go also lacked it, but extracting into a new file is a good opportunity to fix:\n\ngo\n}\n\n// WorkflowData holds all compiler-time data for a single workflow.\ntype WorkflowData struct {\n\n\n@copilot please address this.
There was a problem hiding this comment.
Warning
Firewall blocked 2 domains
The following domains were blocked by the firewall during workflow execution:
patchdiff.githubusercontent.comproxy.golang.org
To allow these domains, add them to the
network.allowedlist in your workflow frontmatter:
network:
allowed:
- defaults
- "patchdiff.githubusercontent.com"
- "proxy.golang.org"See Network Configuration for more information.
🔎 Code quality review by PR Code Quality Reviewer · 108.8 AIC · ⌖ 5.38 AIC · ⊞ 5.4K
Comment /review to run again
| @@ -460,376 +459,3 @@ func (c *Compiler) GetSharedActionResolver() *ActionResolver { | |||
| } | |||
|
|
|||
| // SkipIfMatchConfig holds the configuration for skip-if-match conditions | |||
There was a problem hiding this comment.
Orphaned doc comment at EOF — the type it annotated was moved to workflow_data.go.
💡 Details
Line 461 is:
// SkipIfMatchConfig holds the configuration for skip-if-match conditionsThe SkipIfMatchConfig type was correctly moved to workflow_data.go (line 11), but this doc comment was left behind with no declaration following it. In Go, a comment immediately before a declaration becomes godoc for that declaration; a free-floating comment at EOF implies to every future reader (and refactoring tools) that a declaration is missing or was accidentally deleted.
Fix: Delete line 461 from compiler_types.go.
| Branch string // optional branch name to check (defaults to triggering ref or PR base branch) | ||
| AllowPending bool // if true, pending/in-progress checks are not treated as failing (default: treat pending as failing) | ||
| } | ||
| type WorkflowData struct { |
There was a problem hiding this comment.
Missing blank line between SkipIfCheckFailingConfig and WorkflowData type declarations.
💡 Details
SkipIfCheckFailingConfig's closing } is on line 33 and type WorkflowData struct opens immediately on line 34 with no blank separator. Go convention (per gofmt and the official style guide) requires a blank line between top-level declarations. The missing blank line makes a 150-field struct visually run together with the preceding type, making the file harder to scan.
Fix: Add a blank line between the two type declarations.
Review SummaryApplied 5 inline suggestions:
@copilot please address the review comments above.
|
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /codebase-design — commenting (no blocking issues) on minor navigability improvements.
📋 Key Themes & Highlights
Key Themes
- Inconsistent co-location:
SafeOutputsConfigand its parser are now co-located insafe_outputs_config.go(✅ great), butSkipIf*Configtypes land inworkflow_data.gowhile their parsing lives instop_after.go— the principle is applied unevenly. - Missing navigation aids:
WorkflowDatais the central DTO for the entire compiler but has no file-level doc comment or internal section headers;safe_outputs_config.gosets a higher standard with its## Schema Generation Architectureblock. - Section banner ordering: New types in
safe_outputs_config.goare inserted before the first existing// ========banner rather than inside a clearly labelled section. - Minor Go style nit: No blank line between
SkipIfCheckFailingConfigandWorkflowData(pre-existing inmain, but this PR is the right time to fix it).
Positive Highlights
- ✅ The split is purely mechanical — diff confirms no logic changes.
- ✅ Excellent PR description: lists exactly what moved where and why.
- ✅ Co-locating
SafeOutputsConfigtypes withextractSafeOutputsConfigis exactly the right instinct. - ✅ Removing the now-unused
actionpinsimport fromcompiler_types.gois a clean bonus. - ✅
compiler_types.gois now focused: options,Compilerstruct, constructor, accessors — single responsibility achieved.
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · 161.4 AIC · ⌖ 7.51 AIC · ⊞ 6.6K
Comment /matt to run again
| Branch string // optional branch name to check (defaults to triggering ref or PR base branch) | ||
| AllowPending bool // if true, pending/in-progress checks are not treated as failing (default: treat pending as failing) | ||
| } | ||
| type WorkflowData struct { |
There was a problem hiding this comment.
[/codebase-design] Missing blank line between SkipIfCheckFailingConfig and WorkflowData — Go convention uses a blank line to visually separate top-level declarations.
💡 Suggested fix
Add a blank line after the closing } of SkipIfCheckFailingConfig (line 33) before type WorkflowData struct:
AllowPending bool // ...
}
type WorkflowData struct {This is consistent with the rest of the file and is what gofmt-adjacent tooling (e.g. goimports) expects.
@copilot please address this.
| ModelPolicyAllowed []string // merged models.allowed policy list (union across imports + main frontmatter) | ||
| ModelPolicyBlocked []string // merged models.blocked policy list (union across imports + main frontmatter) | ||
| ActionPinMappings map[string]string // action-pin redirect table from aw.json action_pins: maps "owner/repo@version" → "owner/repo@version" | ||
| } |
There was a problem hiding this comment.
[/codebase-design] WorkflowData has 155+ fields in a flat struct with no internal section grouping, while safe_outputs_config.go uses // ======== banners to separate logical groups. Adding section headers here would make this 236-line file much easier to navigate.
💡 Suggested grouping
Logical groups that already exist but are invisible:
- Identity (
Name,WorkflowID,FrontmatterName,FrontmatterEmoji, ...) - Triggers & Skip conditions (
SkipIfMatch,SkipIfNoMatch,SkipIfCheckFailing,SkipRoles,SkipBots, ...) - Execution / runtime (
RunsOn,Container,Services,Tools,AI,EngineConfig, ...) - Safe-outputs (
SafeOutputs,MCPScripts, ...) - Caches / performance (
CachedPermissions,CachedConcurrencyGroupExpr,CachedAllowedDomainsStr, ...) - Action pinning (
ActionCache,ActionResolver,ActionPinWarnings,ActionPinMappings, ...)
This is a documentation-only change with no logic impact.
@copilot please address this.
| "github.com/github/gh-aw/pkg/parser" | ||
| ) | ||
|
|
||
| // SkipIfMatchConfig holds the configuration for skip-if-match conditions |
There was a problem hiding this comment.
[/codebase-design] The SkipIf*Config types live in workflow_data.go but their parsing logic lives in stop_after.go. The PR co-locates SafeOutputsConfig with its parser (good!), but applies a different principle here. Consider whether SkipIfMatchConfig, SkipIfNoMatchConfig, and SkipIfCheckFailingConfig belong in stop_after.go — or at minimum, add a file-header comment explaining why they live here.
💡 Options to consider
Option A — Move SkipIf*Config to stop_after.go (co-location with parsing):
- Matches the co-location principle used for
safe_outputs_config.go stop_after.goalready hasextractSkipIfMatchFromOn,processSkipIfMatchConfiguration, etc.
Option B — Keep them in workflow_data.go but add a comment:
// SkipIfMatchConfig, SkipIfNoMatchConfig, and SkipIfCheckFailingConfig are trigger-filter
// types referenced directly by WorkflowData. Their parsing logic is in stop_after.go.Either is fine — the key is that the choice is intentional and discoverable.
@copilot please address this.
| @@ -0,0 +1,236 @@ | |||
| package workflow | |||
There was a problem hiding this comment.
[/codebase-design] workflow_data.go has no file-level architecture comment, while safe_outputs_config.go has a rich ## Schema Generation Architecture block (lines 168–207) explaining how the pieces fit together. A brief doc comment on this file explaining the role of WorkflowData (central data transfer object populated by the compiler and consumed by all job builders) would make the codebase more navigable for new contributors.
💡 Example
// Package workflow provides the compiler that transforms markdown workflow
// definitions into GitHub Actions YAML.
//
// This file defines WorkflowData, the central data transfer object that carries
// the fully-parsed state of a single workflow through the compilation pipeline.
// It is populated by the Compiler (compiler_types.go) and consumed by all
// job builders (compiler_jobs.go, compiler_activation_job_builder.go, etc.).
//
// SkipIfMatchConfig, SkipIfNoMatchConfig, and SkipIfCheckFailingConfig are
// also here because they are direct fields of WorkflowData; their parsing
// logic lives in stop_after.go.
package workflow@copilot please address this.
|
|
||
| var safeOutputsConfigLog = logger.New("workflow:safe_outputs_config") | ||
|
|
||
| // ======================================== |
There was a problem hiding this comment.
[/codebase-design] The // ======== banners (lines 16–18 and 168–170) were already present in safe_outputs_config.go on main. Adding the types above line 16 puts them before the first banner rather than inside a named section. Consider adding an explicit // Safe Output Configuration Types banner at the very top of the new block (or restructuring so types sit inside the existing banner) for visual parity with the extraction section below.
💡 Example
var safeOutputsConfigLog = ...
// ========================================
// Safe Output Configuration Types
// ========================================
// BaseSafeOutputConfig ...
type BaseSafeOutputConfig struct {The new types currently land before the first banner — the banner that was already there — which means the section header appears to belong to the types below it rather than the new ones.
@copilot please address this.
|
🎉 This pull request is included in a new release. Release: |
pkg/workflow/compiler_types.gohad grown to 835 lines mixing unrelated concerns: theCompilertype, the centralWorkflowDatadomain struct, and five safe-output config types that belong alongside their extraction logic.Changes
workflow_data.go(new) — extractsWorkflowData, itsPinContext()method, and the threeSkipIf*Configtypes it directly referencessafe_outputs_config.go(prepended) — receivesBaseSafeOutputConfig,SafeOutputsConfig,SafeOutputMessagesConfig,MentionsConfig,SecretMaskingConfig, co-locating them with the parsing logic that was already therecompiler_types.go(reduced) — retains onlyCompilerOption/With*constructors,FileCreationTracker,Compilerstruct,NewCompiler, and its accessor methods; unusedactionpinsimport removedPure mechanical split — no logic changes.