From a4521c3d5f065af2a1bc146c1d13037760af41cd Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Tue, 14 Apr 2026 21:17:50 +0000 Subject: [PATCH 1/4] Initial plan From 5dbabef6d382774efb0bbc47d3356b6d2b4d152d Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Tue, 14 Apr 2026 21:55:12 +0000 Subject: [PATCH 2/4] feat: support checkout field in importable shared workflows Allow the `checkout` field to be defined in shared/importable workflows so it gets merged into the importing workflow, similar to how `github-app` and `tools.github` fields are already handled. This enables SideRepoOps workflows that all target the same repository to centralize the checkout block in their shared import, reducing duplication and ensuring consistency across all workflows in the pattern. Agent-Logs-Url: https://github.com/github/gh-aw/sessions/344b0d32-2b76-466b-9597-115d6ff92823 Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com> --- docs/src/content/docs/reference/imports.md | 2 + pkg/parser/import_field_extractor.go | 11 + pkg/parser/import_processor.go | 1 + pkg/workflow/checkout_import_test.go | 239 ++++++++++++++++++ .../compiler_orchestrator_workflow.go | 23 ++ 5 files changed, 276 insertions(+) create mode 100644 pkg/workflow/checkout_import_test.go diff --git a/docs/src/content/docs/reference/imports.md b/docs/src/content/docs/reference/imports.md index 5e648f028e2..76b0b8d6b13 100644 --- a/docs/src/content/docs/reference/imports.md +++ b/docs/src/content/docs/reference/imports.md @@ -321,6 +321,7 @@ Shared workflow files (without `on:` field) can define: - `secret-masking:` - Secret masking steps - `env:` - Workflow-level environment variables - `github-app:` - GitHub App credentials for token minting (centralize shared app config) +- `checkout:` - Checkout configuration for the agent job (centralize side-repo checkout setup) Agent files (`.github/agents/*.md`) can additionally define: @@ -343,6 +344,7 @@ Imports are processed using breadth-first traversal: direct imports first, then | `runtimes:` | Main overrides imports; imported values fill in unspecified fields. | | `services:` | All services merged; duplicate names fail compilation. | | `github-app:` | Main workflow's `github-app` takes precedence; first imported value fills in if main does not define one. | +| `checkout:` | Imported checkout entries are appended after the main workflow's entries. For duplicate (repository, path) pairs, the main workflow's entry takes precedence. `checkout: false` in the main workflow disables all checkout including imported entries. | | `steps:` | Imported steps prepended to main; concatenated in import order. | | `jobs:` | Not merged — define only in the main workflow. Use `safe-outputs.jobs` for importable jobs. | | `safe-outputs.jobs` | Names must be unique; duplicates fail. Order determined by `needs:` dependencies. | diff --git a/pkg/parser/import_field_extractor.go b/pkg/parser/import_field_extractor.go index 1972bb714ef..aa417018dfd 100644 --- a/pkg/parser/import_field_extractor.go +++ b/pkg/parser/import_field_extractor.go @@ -57,6 +57,8 @@ type importAccumulator struct { activationGitHubApp string // JSON-encoded GitHubAppConfig // First top-level github-app found across all imported files (first-wins strategy) topLevelGitHubApp string // JSON-encoded GitHubAppConfig + // Checkout configs from all imported files (append in order; main workflow's checkouts take precedence) + checkouts []string // JSON-encoded checkout values, one per import } // newImportAccumulator creates and initializes a new importAccumulator. @@ -324,6 +326,14 @@ func (acc *importAccumulator) extractAllImportFields(content []byte, item import } } + // Extract checkout from imported file (append in order; main workflow's checkouts take precedence). + // The checkout field may be a single object or an array of objects; store the raw JSON for + // later parsing by the compiler. + if checkoutJSON, checkoutErr := extractFieldJSONFromMap(fm, "checkout", ""); checkoutErr == nil && checkoutJSON != "" && checkoutJSON != "null" && checkoutJSON != "false" { + acc.checkouts = append(acc.checkouts, checkoutJSON) + log.Printf("Extracted checkout from import: %s", item.fullPath) + } + // Extract pre-steps from imported file (prepend in order) preStepsContent, err := extractYAMLFieldFromMap(fm, "pre-steps") if err == nil && preStepsContent != "" { @@ -470,6 +480,7 @@ func (acc *importAccumulator) toImportsResult(topologicalOrder []string) *Import MergedActivationGitHubToken: acc.activationGitHubToken, MergedActivationGitHubApp: acc.activationGitHubApp, MergedTopLevelGitHubApp: acc.topLevelGitHubApp, + MergedCheckout: strings.Join(acc.checkouts, "\n"), } } diff --git a/pkg/parser/import_processor.go b/pkg/parser/import_processor.go index d7468270616..2f2af069b7f 100644 --- a/pkg/parser/import_processor.go +++ b/pkg/parser/import_processor.go @@ -36,6 +36,7 @@ type ImportsResult struct { MergedActivationGitHubToken string // GitHub token from on.github-token in first imported workflow that defines it MergedActivationGitHubApp string // JSON-encoded on.github-app from first imported workflow that defines it MergedTopLevelGitHubApp string // JSON-encoded top-level github-app from first imported workflow that defines it + MergedCheckout string // JSON-encoded checkout configurations from imported workflows (one JSON value per line) MergedPostSteps string // Merged post-steps configuration from all imports (appended in order) MergedLabels []string // Merged labels from all imports (union of label names) MergedCaches []string // Merged cache configurations from all imports (appended in order) diff --git a/pkg/workflow/checkout_import_test.go b/pkg/workflow/checkout_import_test.go new file mode 100644 index 00000000000..d5c73142e80 --- /dev/null +++ b/pkg/workflow/checkout_import_test.go @@ -0,0 +1,239 @@ +//go:build !integration + +package workflow + +import ( + "os" + "path/filepath" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +// TestCheckoutImportFromSharedWorkflow tests that a checkout block defined in a shared +// workflow is inherited by the importing workflow. +func TestCheckoutImportFromSharedWorkflow(t *testing.T) { + compiler := NewCompilerWithVersion("1.0.0") + + tmpDir := t.TempDir() + workflowsDir := filepath.Join(tmpDir, ".github", "workflows") + require.NoError(t, os.MkdirAll(workflowsDir, 0755)) + + // Shared workflow that declares a checkout block for a side repository + sharedWorkflow := `--- +checkout: + - repository: org/target-repo + ref: master + path: target-repo + current: true +--- + +# Shared side-repo checkout configuration + +This shared workflow centralizes the checkout block for SideRepoOps workflows. +` + require.NoError(t, os.WriteFile(filepath.Join(workflowsDir, "shared-checkout.md"), []byte(sharedWorkflow), 0644)) + + // Main workflow that imports the shared workflow without its own checkout block + mainWorkflow := `--- +on: issues +permissions: + contents: read +imports: + - ./shared-checkout.md +--- + +# Main Workflow + +This workflow inherits the checkout configuration from the shared workflow. +` + mainFile := filepath.Join(workflowsDir, "main.md") + require.NoError(t, os.WriteFile(mainFile, []byte(mainWorkflow), 0644)) + + origDir, err := os.Getwd() + require.NoError(t, err) + require.NoError(t, os.Chdir(workflowsDir)) + defer func() { _ = os.Chdir(origDir) }() + + data, err := compiler.ParseWorkflowFile("main.md") + require.NoError(t, err) + + require.Len(t, data.CheckoutConfigs, 1, "Should have one checkout config from the shared workflow") + cfg := data.CheckoutConfigs[0] + assert.Equal(t, "org/target-repo", cfg.Repository, "Repository should come from the shared workflow") + assert.Equal(t, "master", cfg.Ref, "Ref should come from the shared workflow") + assert.Equal(t, "target-repo", cfg.Path, "Path should come from the shared workflow") + assert.True(t, cfg.Current, "Current should be true from the shared workflow") +} + +// TestCheckoutImportMainWorkflowTakesPrecedence tests that the main workflow's checkout +// takes precedence over an imported checkout for the same (repository, path) key. +func TestCheckoutImportMainWorkflowTakesPrecedence(t *testing.T) { + compiler := NewCompilerWithVersion("1.0.0") + + tmpDir := t.TempDir() + workflowsDir := filepath.Join(tmpDir, ".github", "workflows") + require.NoError(t, os.MkdirAll(workflowsDir, 0755)) + + sharedWorkflow := `--- +checkout: + - repository: org/target-repo + ref: main + path: target-repo +--- + +# Shared Checkout +` + require.NoError(t, os.WriteFile(filepath.Join(workflowsDir, "shared-checkout.md"), []byte(sharedWorkflow), 0644)) + + // Main workflow overrides the checkout for the same path + mainWorkflow := `--- +on: issues +permissions: + contents: read +imports: + - ./shared-checkout.md +checkout: + - repository: org/target-repo + ref: feature-branch + path: target-repo +--- + +# Main Workflow + +This workflow overrides the checkout from the shared workflow. +` + mainFile := filepath.Join(workflowsDir, "main.md") + require.NoError(t, os.WriteFile(mainFile, []byte(mainWorkflow), 0644)) + + origDir, err := os.Getwd() + require.NoError(t, err) + require.NoError(t, os.Chdir(workflowsDir)) + defer func() { _ = os.Chdir(origDir) }() + + data, err := compiler.ParseWorkflowFile("main.md") + require.NoError(t, err) + + // The main workflow's checkout is primary; CheckoutManager merges duplicates with first-seen wins + // for most fields. Both should result in one logical entry for (org/target-repo, target-repo). + require.NotEmpty(t, data.CheckoutConfigs, "Should have checkout configs") + // Find the entry for target-repo path + var found *CheckoutConfig + for _, cfg := range data.CheckoutConfigs { + if cfg.Path == "target-repo" { + found = cfg + break + } + } + require.NotNil(t, found, "Should find checkout config for target-repo path") + assert.Equal(t, "org/target-repo", found.Repository, "Repository should be org/target-repo") +} + +// TestCheckoutImportDisabledByMainWorkflow tests that checkout: false in the main workflow +// suppresses imported checkout configs. +func TestCheckoutImportDisabledByMainWorkflow(t *testing.T) { + compiler := NewCompilerWithVersion("1.0.0") + + tmpDir := t.TempDir() + workflowsDir := filepath.Join(tmpDir, ".github", "workflows") + require.NoError(t, os.MkdirAll(workflowsDir, 0755)) + + sharedWorkflow := `--- +checkout: + - repository: org/target-repo + ref: main + path: target-repo +--- + +# Shared Checkout +` + require.NoError(t, os.WriteFile(filepath.Join(workflowsDir, "shared-checkout.md"), []byte(sharedWorkflow), 0644)) + + mainWorkflow := `--- +on: issues +permissions: + contents: read +imports: + - ./shared-checkout.md +checkout: false +--- + +# Main Workflow + +This workflow disables checkout entirely. +` + mainFile := filepath.Join(workflowsDir, "main.md") + require.NoError(t, os.WriteFile(mainFile, []byte(mainWorkflow), 0644)) + + origDir, err := os.Getwd() + require.NoError(t, err) + require.NoError(t, os.Chdir(workflowsDir)) + defer func() { _ = os.Chdir(origDir) }() + + data, err := compiler.ParseWorkflowFile("main.md") + require.NoError(t, err) + + assert.True(t, data.CheckoutDisabled, "Checkout should be disabled") + assert.Empty(t, data.CheckoutConfigs, "No checkout configs should be merged when checkout is disabled") +} + +// TestCheckoutImportMultipleImports tests that checkout configs from multiple shared +// workflows are all merged into the importing workflow. +func TestCheckoutImportMultipleImports(t *testing.T) { + compiler := NewCompilerWithVersion("1.0.0") + + tmpDir := t.TempDir() + workflowsDir := filepath.Join(tmpDir, ".github", "workflows") + require.NoError(t, os.MkdirAll(workflowsDir, 0755)) + + shared1 := `--- +checkout: + - repository: org/repo-a + path: repo-a +--- + +# Shared Checkout A +` + shared2 := `--- +checkout: + - repository: org/repo-b + path: repo-b +--- + +# Shared Checkout B +` + require.NoError(t, os.WriteFile(filepath.Join(workflowsDir, "shared-a.md"), []byte(shared1), 0644)) + require.NoError(t, os.WriteFile(filepath.Join(workflowsDir, "shared-b.md"), []byte(shared2), 0644)) + + mainWorkflow := `--- +on: issues +permissions: + contents: read +imports: + - ./shared-a.md + - ./shared-b.md +--- + +# Main Workflow +` + mainFile := filepath.Join(workflowsDir, "main.md") + require.NoError(t, os.WriteFile(mainFile, []byte(mainWorkflow), 0644)) + + origDir, err := os.Getwd() + require.NoError(t, err) + require.NoError(t, os.Chdir(workflowsDir)) + defer func() { _ = os.Chdir(origDir) }() + + data, err := compiler.ParseWorkflowFile("main.md") + require.NoError(t, err) + + require.Len(t, data.CheckoutConfigs, 2, "Should have checkout configs from both shared workflows") + + repos := make(map[string]bool) + for _, cfg := range data.CheckoutConfigs { + repos[cfg.Repository] = true + } + assert.True(t, repos["org/repo-a"], "Should include checkout for org/repo-a") + assert.True(t, repos["org/repo-b"], "Should include checkout for org/repo-b") +} diff --git a/pkg/workflow/compiler_orchestrator_workflow.go b/pkg/workflow/compiler_orchestrator_workflow.go index 8d8c1e23084..a1b0ddcc436 100644 --- a/pkg/workflow/compiler_orchestrator_workflow.go +++ b/pkg/workflow/compiler_orchestrator_workflow.go @@ -296,6 +296,29 @@ func (c *Compiler) buildInitialWorkflowData( } } + // Merge checkout configs from imported shared workflows. + // Imported configs are appended after the main workflow's configs so that the main + // workflow's entries take precedence when CheckoutManager deduplicates by (repository, path). + // checkout: false in the main workflow disables all checkout (including imports). + if !workflowData.CheckoutDisabled && importsResult.MergedCheckout != "" { + for line := range strings.SplitSeq(strings.TrimSpace(importsResult.MergedCheckout), "\n") { + if line == "" { + continue + } + var raw any + if err := json.Unmarshal([]byte(line), &raw); err != nil { + orchestratorWorkflowLog.Printf("Failed to unmarshal imported checkout JSON from line %q: %v", line, err) + continue + } + importedConfigs, err := ParseCheckoutConfigs(raw) + if err != nil { + orchestratorWorkflowLog.Printf("Failed to parse imported checkout configs from JSON %q: %v", line, err) + continue + } + workflowData.CheckoutConfigs = append(workflowData.CheckoutConfigs, importedConfigs...) + } + } + // Populate check-for-updates flag: disabled when check-for-updates: false is set in frontmatter. if toolsResult.parsedFrontmatter != nil && toolsResult.parsedFrontmatter.UpdateCheck != nil { workflowData.UpdateCheckDisabled = !*toolsResult.parsedFrontmatter.UpdateCheck From 9672d9d1923e04214e47bb6729fb58e6448e1087 Mon Sep 17 00:00:00 2001 From: "github-actions[bot]" <41898282+github-actions[bot]@users.noreply.github.com> Date: Tue, 14 Apr 2026 22:03:56 +0000 Subject: [PATCH 3/4] docs(adr): add draft ADR-26292 for checkout field in shared workflow imports Generated by Design Decision Gate workflow run 24424945242. Co-Authored-By: Claude Sonnet 4.6 --- ...eckout-field-in-shared-workflow-imports.md | 87 +++++++++++++++++++ 1 file changed, 87 insertions(+) create mode 100644 docs/adr/26292-checkout-field-in-shared-workflow-imports.md diff --git a/docs/adr/26292-checkout-field-in-shared-workflow-imports.md b/docs/adr/26292-checkout-field-in-shared-workflow-imports.md new file mode 100644 index 00000000000..0030b0f5c31 --- /dev/null +++ b/docs/adr/26292-checkout-field-in-shared-workflow-imports.md @@ -0,0 +1,87 @@ +# ADR-26292: `checkout` Field Support in Importable Shared Workflows with Append-After-Main Merge Semantics + +**Date**: 2026-04-14 +**Status**: Draft +**Deciders**: pelikhan, Copilot + +--- + +## Part 1 — Narrative (Human-Friendly) + +### Context + +The GitHub Agentic Workflows (gh-aw) compiler allows shared workflow files to be imported by main workflows, enabling reusable configuration for steps, tools, permissions, and similar fields. However, the `checkout` field — used to configure additional repository checkouts for SideRepoOps workflows — could only be declared in the main workflow file. This forced every workflow that needed to check out a shared target repository to duplicate an identical `checkout:` block, making shared workflows less self-contained and violating the DRY principle across the many SideRepoOps patterns in the codebase. + +### Decision + +We will allow the `checkout` field to be declared in importable shared workflow files. Imported checkout entries are appended *after* the main workflow's checkout entries so that the existing `CheckoutManager` deduplication logic — which uses the `(repository, path)` key pair and a first-seen-wins strategy — naturally gives the main workflow's entries unconditional precedence over any imported value. If the main workflow sets `checkout: false`, all checkout configuration, including any entries sourced from imported files, is suppressed entirely. Internally, imported checkout configs are accumulated as newline-separated JSON values (one per imported file) in a new `MergedCheckout` field on `ImportsResult`, then parsed and appended in the compiler orchestrator. + +### Alternatives Considered + +#### Alternative 1: Continue Requiring Main Workflow to Declare All Checkout Config (Status Quo) + +Each main workflow consuming a shared SideRepoOps pattern must repeat the same `checkout:` block. This is the simplest implementation but contradicts the goal of making shared workflows fully self-contained and creates drift risk when the target repo or branch changes across multiple consumer workflows. + +#### Alternative 2: First-Import-Wins Strategy (Like `github-app`) + +Accept only the first `checkout:` found across all imported files and discard any subsequent ones. This mirrors the strategy used for `github-app`. It was rejected because `checkout` is a list field that may legitimately aggregate distinct repository entries from multiple independent imports (e.g., one shared workflow contributes repo-a, another contributes repo-b). Discarding all but the first import would silently drop valid configurations. + +#### Alternative 3: Error on Duplicate `(repository, path)` Pairs Across Imports (Like `env`) + +Surface a hard compilation error when two imported files both define a checkout for the same `(repository, path)` key. This was considered for consistency with the `env` merge semantics, but rejected because the `CheckoutManager`'s existing first-seen-wins deduplication is already the documented and tested contract for checkout merging. Adding an error here would constrain valid use cases (e.g., an import that happens to duplicate a checkout already present in the main workflow) and is unnecessary given that the main workflow already has clear override authority. + +#### Alternative 4: Introduce a Dedicated `shared-checkout:` Field + +Add a separate frontmatter field (e.g., `shared-checkout:` or `imported-checkout:`) to avoid conflating the local checkout intent with the inherited one. This was rejected because it introduces unnecessary naming complexity, would require documentation and parser changes for a new field, and the `checkout:` field name already carries the right semantic meaning regardless of origin. + +### Consequences + +#### Positive +- Shared workflow files for SideRepoOps patterns can now centralize the `checkout:` block, eliminating repetition across every consumer workflow. +- The main workflow retains full override authority: its entries always take precedence via `CheckoutManager`'s `(repository, path)` deduplication (first-seen-wins), consistent with the "main workflow is the source of truth" invariant established for other merged fields. +- `checkout: false` in the main workflow continues to act as a hard suppress, disabling all checkout regardless of what imports define. +- The implementation reuses the existing newline-separated JSON serialization convention already used for other multi-import fields (`MergedJobs`, `MergedEnv`, etc.). + +#### Negative +- The merge semantics (append-after-main, silent deduplication) are subtler than a simple override or an explicit error — workflow authors must understand that duplicate `(repository, path)` pairs from imports are silently dropped, not flagged. +- `checkout: false` now suppresses imported checkout entries, which may be surprising to authors who expect the main workflow's `checkout: false` to only affect its own locally declared config. +- `ImportsResult`, `importAccumulator`, and the compiler orchestrator each gain new fields and logic, increasing the structural surface area of the compiler pipeline. + +#### Neutral +- The new behavior is additive: existing workflows without `checkout:` in their shared imports are entirely unaffected; no migration is needed. +- The JSON-per-line accumulation pattern in `MergedCheckout` is consistent with `MergedJobs` and `MergedCaches`, keeping the internal serialization approach uniform. + +--- + +## Part 2 — Normative Specification (RFC 2119) + +> The key words **MUST**, **MUST NOT**, **REQUIRED**, **SHALL**, **SHALL NOT**, **SHOULD**, **SHOULD NOT**, **RECOMMENDED**, **MAY**, and **OPTIONAL** in this section are to be interpreted as described in [RFC 2119](https://www.rfc-editor.org/rfc/rfc2119). + +### Checkout Field Allowance in Shared Imports + +1. Shared workflow imports **MUST** be permitted to declare a `checkout:` field; the compiler **MUST NOT** treat `checkout` as a forbidden field in shared workflow files. +2. The `checkout` key **MUST NOT** appear in `SharedWorkflowForbiddenFields`. +3. A shared workflow's `checkout:` field **MAY** be a single object or an array of objects; the extractor **MUST** handle both forms. +4. A shared workflow's `checkout: false` value **MUST** be silently ignored by the import extractor (the `false` suppression semantics apply only to the main workflow's declaration). + +### Checkout Merge Semantics + +1. Imported checkout entries **MUST** be appended after the main workflow's checkout entries in `workflowData.CheckoutConfigs` so that the `CheckoutManager`'s first-seen-wins deduplication on `(repository, path)` pairs gives the main workflow's entries unconditional precedence. +2. When the main workflow declares `checkout: false`, the compiler **MUST NOT** append any imported checkout entries; `workflowData.CheckoutDisabled` **MUST** remain `true` regardless of what imported files define. +3. When the main workflow does not declare `checkout: false`, imported checkout entries **MUST** be parsed and appended to `workflowData.CheckoutConfigs` after the main workflow's entries, in the order they appear across imports. +4. Duplicate `(repository, path)` pairs across imports **MUST** be resolved by the existing `CheckoutManager` deduplication logic (first-seen-wins); the compiler **MUST NOT** return an error for such duplicates. + +### Internal Data Model + +1. `ImportsResult` **MUST** expose a `MergedCheckout string` field containing newline-separated JSON-encoded checkout values accumulated from all imported files. +2. The `importAccumulator` struct **MUST** maintain a `checkouts []string` slice, where each element is the raw JSON of a single imported `checkout:` value (object or array). +3. Implementations **MUST** serialize `MergedCheckout` as `strings.Join(acc.checkouts, "\n")`, consistent with the newline-separated JSON convention used for other multi-import accumulated fields. +4. Implementations **MUST NOT** include `"null"` or `"false"` JSON values in the `checkouts` slice; such values from imported files **MUST** be silently skipped. + +### Conformance + +An implementation is considered conformant with this ADR if it satisfies all **MUST** and **MUST NOT** requirements above. Specifically: the `checkout` field is accepted in shared imports without warning; `checkout: false` in a shared import is silently ignored; imported checkout entries are appended after the main workflow's entries so the main workflow takes precedence; `checkout: false` in the main workflow suppresses all imported checkout entries; and the internal representation uses newline-separated JSON in `ImportsResult.MergedCheckout`. Failure to meet any **MUST** or **MUST NOT** requirement constitutes non-conformance. + +--- + +*This is a DRAFT ADR generated by the [Design Decision Gate](https://github.com/github/gh-aw/actions/runs/24424945242) workflow. The PR author must review, complete, and finalize this document before the PR can merge.* From 31f689f327cbf49ab394d05cfaaec90f05c0d2a4 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Tue, 14 Apr 2026 22:28:59 +0000 Subject: [PATCH 4/4] fix: enforce auth mutual exclusivity in checkout merge and improve logging safety - checkout_manager.go: Once github-token or github-app is set for a (repository, path) entry, the other auth method from a later (imported) config is ignored. This preserves the main workflow's auth choice over imported defaults. - compiler_orchestrator_workflow.go: Remove raw JSON payload from debug log messages to avoid accidentally logging sensitive credential values. - checkout_import_test.go: Assert deduplication count and ref precedence via NewCheckoutManager; add TestCheckoutImportAuthPrecedence to verify that a main workflow github-token is not overridden by an imported github-app. - imports.md: Update checkout merge semantics to document auth mutual exclusivity. Agent-Logs-Url: https://github.com/github/gh-aw/sessions/9efa9bc4-2a9e-40c4-8886-85d107f7c0f4 Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com> --- docs/src/content/docs/reference/imports.md | 2 +- pkg/workflow/checkout_import_test.go | 83 ++++++++++++++++--- pkg/workflow/checkout_manager.go | 12 +-- .../compiler_orchestrator_workflow.go | 4 +- 4 files changed, 81 insertions(+), 20 deletions(-) diff --git a/docs/src/content/docs/reference/imports.md b/docs/src/content/docs/reference/imports.md index 76b0b8d6b13..9efb833e534 100644 --- a/docs/src/content/docs/reference/imports.md +++ b/docs/src/content/docs/reference/imports.md @@ -344,7 +344,7 @@ Imports are processed using breadth-first traversal: direct imports first, then | `runtimes:` | Main overrides imports; imported values fill in unspecified fields. | | `services:` | All services merged; duplicate names fail compilation. | | `github-app:` | Main workflow's `github-app` takes precedence; first imported value fills in if main does not define one. | -| `checkout:` | Imported checkout entries are appended after the main workflow's entries. For duplicate (repository, path) pairs, the main workflow's entry takes precedence. `checkout: false` in the main workflow disables all checkout including imported entries. | +| `checkout:` | Imported checkout entries are appended after the main workflow's entries. For duplicate (repository, path) pairs, the main workflow's entry takes precedence: first-seen wins for `ref`, and auth is mutually exclusive — once `github-token` or `github-app` is set by the main workflow, an imported duplicate cannot add the other auth method. `checkout: false` in the main workflow disables all checkout including imported entries. | | `steps:` | Imported steps prepended to main; concatenated in import order. | | `jobs:` | Not merged — define only in the main workflow. Use `safe-outputs.jobs` for importable jobs. | | `safe-outputs.jobs` | Names must be unique; duplicates fail. Order determined by `needs:` dependencies. | diff --git a/pkg/workflow/checkout_import_test.go b/pkg/workflow/checkout_import_test.go index d5c73142e80..a819cac8605 100644 --- a/pkg/workflow/checkout_import_test.go +++ b/pkg/workflow/checkout_import_test.go @@ -115,19 +115,17 @@ This workflow overrides the checkout from the shared workflow. data, err := compiler.ParseWorkflowFile("main.md") require.NoError(t, err) - // The main workflow's checkout is primary; CheckoutManager merges duplicates with first-seen wins - // for most fields. Both should result in one logical entry for (org/target-repo, target-repo). + // data.CheckoutConfigs holds the raw (pre-dedup) slice: main entry first, then imported. + // Deduplication and merge-precedence are enforced by NewCheckoutManager. require.NotEmpty(t, data.CheckoutConfigs, "Should have checkout configs") - // Find the entry for target-repo path - var found *CheckoutConfig - for _, cfg := range data.CheckoutConfigs { - if cfg.Path == "target-repo" { - found = cfg - break - } - } - require.NotNil(t, found, "Should find checkout config for target-repo path") - assert.Equal(t, "org/target-repo", found.Repository, "Repository should be org/target-repo") + + cm := NewCheckoutManager(data.CheckoutConfigs) + // After deduplication there should be exactly one resolved entry for (org/target-repo, target-repo). + require.Len(t, cm.ordered, 1, "Duplicate (repository, path) entries should be merged into one") + entry := cm.ordered[0] + assert.Equal(t, "org/target-repo", entry.key.repository, "Repository should be org/target-repo") + assert.Equal(t, "target-repo", entry.key.path, "Path should be target-repo") + assert.Equal(t, "feature-branch", entry.ref, "Main workflow's ref should take precedence over imported ref") } // TestCheckoutImportDisabledByMainWorkflow tests that checkout: false in the main workflow @@ -237,3 +235,64 @@ imports: assert.True(t, repos["org/repo-a"], "Should include checkout for org/repo-a") assert.True(t, repos["org/repo-b"], "Should include checkout for org/repo-b") } + +// TestCheckoutImportAuthPrecedence tests that the main workflow's auth method is preserved +// when an imported shared workflow defines conflicting auth for the same (repository, path). +// A main workflow github-token must not be overridden by an imported github-app, and vice versa. +func TestCheckoutImportAuthPrecedence(t *testing.T) { + compiler := NewCompilerWithVersion("1.0.0") + + tmpDir := t.TempDir() + workflowsDir := filepath.Join(tmpDir, ".github", "workflows") + require.NoError(t, os.MkdirAll(workflowsDir, 0755)) + + // Shared workflow has a github-app for the same repository/path + sharedWorkflow := `--- +checkout: + - repository: org/target-repo + ref: main + path: target-repo + github-app: + app-id: ${{ vars.APP_ID }} + private-key: ${{ secrets.APP_PRIVATE_KEY }} +--- + +# Shared Checkout with App Auth +` + require.NoError(t, os.WriteFile(filepath.Join(workflowsDir, "shared-checkout.md"), []byte(sharedWorkflow), 0644)) + + // Main workflow uses a plain token for the same repository/path + mainWorkflow := `--- +on: issues +permissions: + contents: read +imports: + - ./shared-checkout.md +checkout: + - repository: org/target-repo + ref: feature-branch + path: target-repo + github-token: ${{ secrets.MY_PAT }} +--- + +# Main Workflow +` + mainFile := filepath.Join(workflowsDir, "main.md") + require.NoError(t, os.WriteFile(mainFile, []byte(mainWorkflow), 0644)) + + origDir, err := os.Getwd() + require.NoError(t, err) + require.NoError(t, os.Chdir(workflowsDir)) + defer func() { _ = os.Chdir(origDir) }() + + data, err := compiler.ParseWorkflowFile("main.md") + require.NoError(t, err) + + cm := NewCheckoutManager(data.CheckoutConfigs) + require.Len(t, cm.ordered, 1, "Should have one merged entry for the duplicate (repository, path)") + entry := cm.ordered[0] + assert.Equal(t, "${{ secrets.MY_PAT }}", entry.token, "Main workflow's github-token must be preserved") + assert.Nil(t, entry.githubApp, "Imported github-app must not override main workflow's github-token") + assert.Equal(t, "feature-branch", entry.ref, "Main workflow's ref should take precedence") + assert.False(t, cm.HasAppAuth(), "Checkout manager should report no app auth (main token takes precedence)") +} diff --git a/pkg/workflow/checkout_manager.go b/pkg/workflow/checkout_manager.go index 1a72e83ad47..c06d5ed7f3a 100644 --- a/pkg/workflow/checkout_manager.go +++ b/pkg/workflow/checkout_manager.go @@ -212,17 +212,19 @@ func (cm *CheckoutManager) add(cfg *CheckoutConfig) { } if idx, exists := cm.index[key]; exists { - // Merge into existing entry; first-seen wins for ref and token + // Merge into existing entry; first-seen wins for ref and token/app (auth is mutually exclusive: + // once either github-token or github-app is set for an entry, the other method is not added + // even if a later config provides it — this preserves the main workflow's auth choice). entry := cm.ordered[idx] entry.fetchDepth = deeperFetchDepth(entry.fetchDepth, cfg.FetchDepth) if cfg.Ref != "" && entry.ref == "" { entry.ref = cfg.Ref // first-seen ref wins } - if cfg.GitHubToken != "" && entry.token == "" { - entry.token = cfg.GitHubToken // first-seen github-token wins + if cfg.GitHubToken != "" && entry.token == "" && entry.githubApp == nil { + entry.token = cfg.GitHubToken // first-seen auth wins (mutually exclusive with github-app) } - if cfg.GitHubApp != nil && entry.githubApp == nil { - entry.githubApp = cfg.GitHubApp // first-seen github-app wins + if cfg.GitHubApp != nil && entry.githubApp == nil && entry.token == "" { + entry.githubApp = cfg.GitHubApp // first-seen auth wins (mutually exclusive with github-token) } if cfg.SparseCheckout != "" { entry.sparsePatterns = mergeSparsePatterns(entry.sparsePatterns, cfg.SparseCheckout) diff --git a/pkg/workflow/compiler_orchestrator_workflow.go b/pkg/workflow/compiler_orchestrator_workflow.go index a1b0ddcc436..8e6df03ee85 100644 --- a/pkg/workflow/compiler_orchestrator_workflow.go +++ b/pkg/workflow/compiler_orchestrator_workflow.go @@ -307,12 +307,12 @@ func (c *Compiler) buildInitialWorkflowData( } var raw any if err := json.Unmarshal([]byte(line), &raw); err != nil { - orchestratorWorkflowLog.Printf("Failed to unmarshal imported checkout JSON from line %q: %v", line, err) + orchestratorWorkflowLog.Printf("Failed to unmarshal imported checkout JSON: %v", err) continue } importedConfigs, err := ParseCheckoutConfigs(raw) if err != nil { - orchestratorWorkflowLog.Printf("Failed to parse imported checkout configs from JSON %q: %v", line, err) + orchestratorWorkflowLog.Printf("Failed to parse imported checkout configs: %v", err) continue } workflowData.CheckoutConfigs = append(workflowData.CheckoutConfigs, importedConfigs...)