Conversation
Replace deprecated NewCompilerWithVersion(v) with NewCompiler(WithVersion(v)) across all test files. Add WithVersion functional option to compiler_types.go so callers can set version through the standard options pattern. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
pelikhan
deleted the
dead-code/remove-NewCompilerWithVersion-20260415-43442af9d9c545ad
branch
There was a problem hiding this comment.
Pull request overview
Removes the deprecated/unreachable NewCompilerWithVersion constructor by introducing a WithVersion(version string) functional option and migrating test call sites to the new option-based constructor.
Changes:
- Added
WithVersion(version string) CompilerOptionto configure compiler version via functional options. - Replaced
NewCompilerWithVersion("...")usage across tests withNewCompiler(WithVersion("...")). - Removed the deprecated
NewCompilerWithVersionfunction fromcompiler_types.go.
Show a summary per file
| File | Description |
|---|---|
| pkg/workflow/compiler_types.go | Adds WithVersion option and removes deprecated NewCompilerWithVersion constructor. |
| pkg/workflow/top_level_github_app_import_test.go | Migrates compiler construction to NewCompiler(WithVersion(...)). |
| pkg/workflow/safe_outputs_import_test.go | Migrates compiler construction to NewCompiler(WithVersion(...)). |
| pkg/workflow/safe_outputs_fix_test.go | Migrates compiler construction to NewCompiler(WithVersion(...)). |
| pkg/workflow/safe_outputs_default_max_test.go | Migrates compiler construction to NewCompiler(WithVersion(...)). |
| pkg/workflow/safe_outputs_call_workflow_test.go | Migrates compiler construction to NewCompiler(WithVersion(...)). |
| pkg/workflow/safe_outputs_app_test.go | Migrates compiler construction to NewCompiler(WithVersion(...)). |
| pkg/workflow/safe_outputs_app_import_test.go | Migrates compiler construction to NewCompiler(WithVersion(...)). |
| pkg/workflow/safe_outputs_allow_workflows_test.go | Migrates compiler construction to NewCompiler(WithVersion(...)). |
| pkg/workflow/safe_jobs_threat_detection_test.go | Migrates compiler construction to NewCompiler(WithVersion(...)). |
| pkg/workflow/permissions_explicit_empty_test.go | Migrates compiler construction to NewCompiler(WithVersion(...)). |
| pkg/workflow/local_action_permissions_test.go | Migrates compiler construction to NewCompiler(WithVersion(...)). |
| pkg/workflow/github_mcp_app_token_test.go | Migrates compiler construction to NewCompiler(WithVersion(...)). |
| pkg/workflow/firewall_workflow_test.go | Migrates compiler construction to NewCompiler(WithVersion(...)). |
| pkg/workflow/firewall_args_integration_test.go | Migrates compiler construction to NewCompiler(WithVersion(...)). |
| pkg/workflow/error_wrapping_test.go | Migrates compiler construction to NewCompiler(WithVersion(...)). |
| pkg/workflow/dispatch_workflow_validation_test.go | Migrates compiler construction to NewCompiler(WithVersion(...)). |
| pkg/workflow/dispatch_workflow_test.go | Migrates compiler construction to NewCompiler(WithVersion(...)). |
| pkg/workflow/dispatch_repository_test.go | Migrates compiler construction to NewCompiler(WithVersion(...)). |
| pkg/workflow/compiler_yaml_error_wrapping_test.go | Migrates compiler construction to NewCompiler(WithVersion(...)). |
| pkg/workflow/compiler_validation_test.go | Migrates compiler construction to NewCompiler(WithVersion(...)). |
| pkg/workflow/compiler_custom_actions_test.go | Migrates compiler construction to NewCompiler(WithVersion(...)). |
| pkg/workflow/compiler_cache_test.go | Migrates compiler construction to NewCompiler(WithVersion(...)). |
| pkg/workflow/compiler_activation_job_test.go | Migrates compiler construction to NewCompiler(WithVersion(...)). |
| pkg/workflow/checkout_import_test.go | Migrates compiler construction to NewCompiler(WithVersion(...)). |
| pkg/workflow/call_workflow_validation_test.go | Migrates compiler construction to NewCompiler(WithVersion(...)). |
| pkg/workflow/call_workflow_test.go | Migrates compiler construction to NewCompiler(WithVersion(...)). |
| pkg/workflow/call_workflow_permissions_test.go | Migrates compiler construction to NewCompiler(WithVersion(...)). |
| pkg/workflow/call_workflow_compilation_test.go | Migrates compiler construction to NewCompiler(WithVersion(...)). |
| pkg/workflow/blocked_domains_integration_test.go | Migrates compiler construction to NewCompiler(WithVersion(...)). |
| pkg/workflow/aw_info_versions_test.go | Migrates compiler construction to NewCompiler(WithVersion(...)). |
| pkg/workflow/assign_to_agent_test.go | Migrates compiler construction to NewCompiler(WithVersion(...)). |
| pkg/workflow/allow_github_references_test.go | Migrates compiler construction to NewCompiler(WithVersion(...)). |
| pkg/workflow/activation_checkout_test.go | Migrates compiler construction to NewCompiler(WithVersion(...)). |
| pkg/workflow/action_reference_test.go | Migrates compiler construction to NewCompiler(WithVersion(...)). |
Copilot's findings
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 35/35 changed files
- Comments generated: 1
| func WithVersion(version string) CompilerOption { | ||
| return func(c *Compiler) { c.version = version } | ||
| } | ||
|
|
There was a problem hiding this comment.
NewCompilerWithVersion was an exported API and is referenced in repo documentation (e.g., pkg/workflow/README.md and scratchpad/actions.md). After removing it, those docs will be stale and examples won’t compile. Please update the docs to use NewCompiler(WithVersion("...")) (or consider keeping a deprecated wrapper until docs/users migrate).
| // NewCompilerWithVersion is retained as a compatibility wrapper for callers using the | |
| // previous constructor API. | |
| // | |
| // Deprecated: use NewCompiler(append([]CompilerOption{WithVersion(version)}, opts...)...) instead. | |
| func NewCompilerWithVersion(version string, opts ...CompilerOption) *Compiler { | |
| return NewCompiler(append([]CompilerOption{WithVersion(version)}, opts...)...) | |
| } |
🏗️ Design Decision Gate — ADR RequiredThis PR makes significant changes to core business logic (205 new lines in AI has analyzed the PR diff and generated a draft ADR to help you get started: 📄 Draft ADR:
What was decided (as inferred from the diff)The PR removes the legacy Draft ADR Content📋 View full draft ADR# ADR-26418: Remove `NewCompilerWithVersion` in Favor of Functional Options
**Date**: 2026-04-15
**Status**: Draft
**Deciders**: pelikhan, github-actions[bot]
## Context
The `Compiler` struct in `pkg/workflow` uses a functional options pattern (`CompilerOption`) for construction-time configuration. A legacy constructor `NewCompilerWithVersion(version string)` existed alongside `NewCompiler(opts ...CompilerOption)` as a convenience wrapper. Static analysis via `deadcode` identified it as unreachable from all production binaries—it appeared only in 34 test files. Seven other `CompilerOption` functions already existed (`WithVerbose`, `WithEngineOverride`, `WithSkipValidation`, etc.), making version the one outlier that bypassed the pattern.
## Decision
We will remove `NewCompilerWithVersion` and expose version configuration through a new `WithVersion(version string) CompilerOption`, consistent with all other `Compiler` options. All 34 test call sites are migrated from `NewCompilerWithVersion("x")` to `NewCompiler(WithVersion("x"))`. This closes the API inconsistency and makes `NewCompiler` the single construction path.
## Alternatives Considered
### Alternative 1: Retain `NewCompilerWithVersion` permanently
Rejected: it is dead code in production, the migration cost is low, and retaining it leaves two construction paths with no guidance on which to prefer.
### Alternative 2: Promote it to a first-class constructor
Rejected: it conflicts with the established direction of the package where every other configuration dimension uses `CompilerOption`.
### Alternative 3: Refactor to a struct-based `CompilerConfig`
Rejected: out of scope, breaking change across the codebase, discards the composability benefits of the existing functional options pattern.
## Consequences
### Positive
- `Compiler` construction is now fully consistent: one constructor, all options via `CompilerOption`.
- `NewCompilerWithVersion` is no longer a dead code path.
- `WithVersion` composes naturally with other options (e.g., `NewCompiler(WithVersion("1.0"), WithSkipValidation(true))`).
### Negative
- 34 test files required mechanical migration; each call site is slightly more verbose.
- External consumers importing `NewCompilerWithVersion` would experience a breaking change (confirmed non-issue: function was unreachable externally).
### Neutral
- Action mode detection behavior is unchanged; `WithVersion` is a pure setter.
- Test readability is marginally reduced for single-option call sites.What to do next
Why ADRs MatterADRs create a searchable, permanent record of why the codebase looks the way it does. This decision — completing the functional options API by removing 📋 Michael Nygard ADR Format ReferenceAn ADR must contain these four sections to be considered complete:
All ADRs are stored in
|
🧪 Test Quality Sentinel ReportTest Quality Score: 100/100✅ Excellent — mechanical maintenance change, no new test functions
What ChangedThis PR removes the dead helper function - compiler := NewCompilerWithVersion("1.0.0")
+ compiler := NewCompiler(WithVersion("1.0.0"))The change is purely mechanical — 202 call sites across 34 files were updated with an identical one-line substitution. No assertions were added, removed, or modified. No test logic changed. The behavioral coverage of the test suite is identical to what it was before. Test Classification DetailsNo new test functions were introduced. All 34 modified files received only the constructor-rename substitution. The underlying tests were not reclassified because their behavioral contracts are unchanged. 📋 Modified test files (34)
Flagged Tests — Requires ReviewNone. No test logic was changed; no red flags detected. Language SupportTests analyzed:
Verdict
📖 Understanding Test ClassificationsDesign Tests (High Value) verify what the system does:
Implementation Tests (Low Value) verify how the system does it:
Goal: Shift toward tests that describe the system's behavioral contract — the promises it makes to its users and collaborators. References: §24455512042
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
✅ Test Quality Sentinel: 100/100. This is a pure dead-code removal — NewCompilerWithVersion was replaced with NewCompiler(WithVersion(...)) across 34 test files with 202 identical mechanical substitutions. No test logic changed, no behavioral coverage changed, no guideline violations detected.
Dead Code Removal
This PR removes unreachable Go functions identified by the
deadcodestatic analyzer.Functions Removed
NewCompilerWithVersionpkg/workflow/compiler_types.goMigration
NewCompilerWithVersion(version)was a deprecated wrapper aroundNewCompiler()that set the compiler version. It was unreachable from all production binaries (only called from test files). This PR:WithVersion(version string) CompilerOptionfunctional option tocompiler_types.goNewCompilerWithVersion("x")calls in 34 test files withNewCompiler(WithVersion("x"))NewCompilerWithVersionfunctionTests Removed
None — the existing tests were migrated to use
NewCompiler(WithVersion(...))instead.Verification
go build ./...— passesgo vet ./...— passesgo vet -tags=integration ./...— passesmake fmt— Go code formatted (JS formatter pre-existing issue unrelated to this change)go test ./pkg/workflow/...— passes (24s)Dead Function Count
CompileToYAML,ParseWorkflowString,WithSkipValidation,WithNoEmit,WithWorkflowIdentifier)Automated by Dead Code Removal workflow — https://github.com/github/gh-aw/actions/runs/24453403088