Prevent compile panic when workflow directory contains only shared components#33029
Conversation
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Fixes a panic in gh aw compile --strict when compiling a workflow directory that contains only shared Markdown components (i.e., no compiled top-level workflows), by preventing nil workflow entries from being dereferenced during maintenance post-processing.
Changes:
- Filter out nil
WorkflowDataresults during compilation so downstream steps don’t receive nil entries. - Add defensive nil checks in maintenance workflow scanning and resolver selection.
- Add a regression test ensuring shared-only workflow directories compile (strict + non-strict) without panicking and do not emit a maintenance workflow.
Show a summary per file
| File | Description |
|---|---|
| pkg/workflow/maintenance_workflow.go | Adds nil-safe iteration for resolver selection and expires scanning/disabled handling. |
| pkg/cli/compile_pipeline.go | Avoids appending nil workflow results and gates directory post-processing generation. |
| pkg/cli/compile_maintenance_test.go | Adds regression test for shared-only workflow directory compilation (strict/non-strict). |
Copilot's findings
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 3/3 changed files
- Comments generated: 3
| // Also skip when no compiled workflow data is available (for example shared-only directories). | ||
| if !config.NoEmit && config.WorkflowDir == "" && len(workflowDataList) > 0 { |
| minExpires := 0 // Track minimum expires value in hours | ||
|
|
||
| for _, workflowData := range workflowDataList { | ||
| if workflowData.SafeOutputs == nil { | ||
| if workflowData == nil || workflowData.SafeOutputs == nil { | ||
| continue | ||
| } |
| func TestCompileOnlySharedWorkflow_DoesNotPanic(t *testing.T) { | ||
| // Create temporary directory structure | ||
| tempDir := testutil.TempDir(t, "test-*") | ||
| workflowsDir := filepath.Join(tempDir, ".github/workflows") | ||
| if err := os.MkdirAll(workflowsDir, 0755); err != nil { | ||
| t.Fatalf("Failed to create workflows directory: %v", err) | ||
| } | ||
|
|
||
| // Change to temp directory | ||
| originalDir, _ := os.Getwd() | ||
| defer os.Chdir(originalDir) | ||
| if err := os.Chdir(tempDir); err != nil { | ||
| t.Fatalf("Failed to change directory: %v", err) | ||
| } | ||
|
|
||
| // Initialize git repo | ||
| initCmd := exec.Command("git", "init") | ||
| initCmd.Dir = tempDir | ||
| if err := initCmd.Run(); err != nil { | ||
| t.Fatalf("Failed to initialize git repo: %v", err) | ||
| } | ||
|
|
||
| // Create a shared workflow component (missing top-level "on") | ||
| sharedWorkflowContent := `--- | ||
| description: "Shared Component" | ||
| engine: copilot | ||
| --- | ||
|
|
||
| Shared workflow component. | ||
| ` | ||
| sharedWorkflowPath := filepath.Join(workflowsDir, "shared-component.md") | ||
| if err := os.WriteFile(sharedWorkflowPath, []byte(sharedWorkflowContent), 0644); err != nil { | ||
| t.Fatalf("Failed to write shared workflow file: %v", err) | ||
| } | ||
|
|
||
| for _, strict := range []bool{false, true} { | ||
| t.Run("strict="+strconv.FormatBool(strict), func(t *testing.T) { | ||
| config := CompileConfig{ | ||
| MarkdownFiles: []string{}, | ||
| Verbose: false, | ||
| EngineOverride: "", | ||
| Validate: false, | ||
| Watch: false, | ||
| WorkflowDir: "", | ||
| SkipInstructions: false, | ||
| NoEmit: false, | ||
| Purge: false, | ||
| TrialMode: false, | ||
| TrialLogicalRepoSlug: "", | ||
| Strict: strict, | ||
| } | ||
|
|
||
| if _, err := CompileWorkflows(context.Background(), config); err != nil { | ||
| t.Fatalf("CompileWorkflows should succeed for shared-only workflow directory: %v", err) | ||
| } | ||
| }) | ||
| } | ||
|
|
||
| maintenancePath := filepath.Join(workflowsDir, "agentics-maintenance.yml") | ||
| if _, err := os.Stat(maintenancePath); !os.IsNotExist(err) { | ||
| t.Error("Maintenance workflow should not be generated for shared-only workflow directory") | ||
| } | ||
| } |
🧪 Test Quality Sentinel ReportTest Quality Score: 80/100✅ Excellent
Test Classification Details
Test Inflation Note
This is acceptable here: the test requires substantial scaffolding (creating a temp dir, initialising a git repo, writing a fixture file, and changing directory) that dwarfs the single assertion being made. The scaffolding is necessary for a realistic end-to-end exercise of the compile pipeline. No action is required, but it is noted per scoring rules. Analysis
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: §26036151113
|
![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: 80/100. Test quality is excellent — 0% of new tests are implementation tests (threshold: 30%). The single added test directly guards the shared-only-directory panic regression with both strict and non-strict execution paths covered.
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /diagnose and /tdd — this is a targeted bug fix with regression coverage, which is exactly the right combination to evaluate.
Key Themes
- Test naming (
/tdd): The test name references the internal symptom (panic) rather than the behavioral contract being enforced. Small rename makes the intent clearer. - Missing coverage of
compileSpecificFiles(/diagnose): Both code paths received the same nil guard fix, but the test only exercises the directory-scan path. The explicit-files path goes untested. - Comment accuracy (
/diagnose): The new comment inGenerateMaintenanceWorkflowimplies the pipeline can pass nil entries, but the upstream fix now prevents that. The comment should clarify it's defensive-only.
Positive Highlights
- ✅ The root cause is properly addressed at the right level — nil entries are filtered out upstream in the pipeline, rather than relying solely on downstream guards
- ✅ The
len(workflowDataList) > 0guard is clean and obvious — a reader immediately understands why maintenance generation is skipped - ✅ Regression test exercises
CompileWorkflowsthrough the real public interface (not a mock), which is exactly the right seam per/diagnosePhase 5 - ✅ Both
strict=falseandstrict=truevariants are tested in a single test function
Verdict
No blocking issues — the fix is correct and the test coverage is good. The suggestions above are improvements worth considering, particularly the missing compileSpecificFiles test case.
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · ● 5.6M
| } | ||
| } | ||
|
|
||
| func TestCompileOnlySharedWorkflow_DoesNotPanic(t *testing.T) { |
There was a problem hiding this comment.
[/tdd] The test name TestCompileOnlySharedWorkflow_DoesNotPanic describes an implementation artifact (panic) rather than the observable behavior being specified. Per TDD principles, test names should read as specifications.
Consider renaming to something like TestSharedOnlyDirectory_CompilesSuccessfullyWithNoMaintenanceWorkflow — this tells the next developer exactly what contract is being enforced without referencing internal crash behavior.
| Verbose: false, | ||
| EngineOverride: "", | ||
| Validate: false, | ||
| Watch: false, |
There was a problem hiding this comment.
[/diagnose] The compileSpecificFiles code path (triggered when MarkdownFiles is non-empty) received the same nil guard fix as compileAllFilesInDirectory, but this test only exercises the directory-scan path (MarkdownFiles: []string{}).
Consider adding a second sub-test that passes the shared component path explicitly:
config := CompileConfig{
MarkdownFiles: []string{sharedWorkflowPath},
// ...
}Without this, the compileSpecificFiles fix is untested and could regress silently.
| // Get the setup action reference (local or remote based on mode). | ||
| // Use the first available WorkflowData's ActionResolver to enable SHA pinning. | ||
| // Computed early so it is available in the !hasExpires path for side-repo workflows. | ||
| // Iterate to find the first non-nil entry because shared-only compilation paths |
There was a problem hiding this comment.
[/diagnose] The comment says "may provide nil placeholders" but the upstream fix in compile_pipeline.go now actively filters out nil entries before calling this function, and the len(workflowDataList) > 0 guard prevents the call entirely for shared-only directories.
The nil loop here is therefore purely defensive (guarding against direct callers bypassing the pipeline). Consider wording the comment to reflect that:
// Defensive nil check: callers outside the standard pipeline may pass nil entries.This avoids implying the pipeline itself passes nils, which is no longer accurate.
|
@copilot review all comments, lint go |
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Addressed actionable review feedback in commit
I also ran Go lint ( |
gh aw compile/--strictcould panic in maintenance post-processing when every.mdfile was a shared component (no top-levelon:), because nil workflow entries were dereferenced. This change makes shared-only directories a clean, non-panicking compile path.Post-processing guard (compile pipeline)
WorkflowDatafrom per-file compilation results.len(workflowDataList) > 0) so shared-only directories do not enter maintenance scanning.Defensive nil handling (maintenance workflow)
scanWorkflowsForExpiresandhandleMaintenanceDisabled.Regression coverage