Add checkout field and manager for agent job - #18223
Conversation
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
|
@copilot add checkout entry in the smoke-codex with fetch depth 2 |
…ack parsing Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Done in 62e65ce. Added
|
There was a problem hiding this comment.
Pull request overview
Adds a top-level checkout frontmatter field to control how actions/checkout steps are emitted for the agent job, including support for multiple checkouts and merged/deduped configs.
Changes:
- Introduces
CheckoutManager+CheckoutConfigparsing/merging and uses it to generate checkout steps in the main job. - Extends frontmatter/workflow data types to carry raw + parsed checkout configs, including a fallback raw parse path when typed frontmatter parsing fails.
- Updates the JSON schema and smoke-test workflow example/lock output to include
checkout.fetch-depth.
Reviewed changes
Copilot reviewed 10 out of 10 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| pkg/workflow/checkout_manager.go | Implements checkout config parsing, merging, and YAML step generation |
| pkg/workflow/checkout_manager_test.go | Adds unit tests for parsing/merge/generation behavior |
| pkg/workflow/frontmatter_types.go | Adds checkout fields to frontmatter config and parses them into typed configs |
| pkg/workflow/compiler_types.go | Plumbs checkout configs through WorkflowData |
| pkg/workflow/compiler_orchestrator_workflow.go | Populates WorkflowData.CheckoutConfigs from parsed or raw frontmatter |
| pkg/workflow/compiler_yaml_main_job.go | Switches checkout step generation to CheckoutManager |
| pkg/parser/schemas/main_workflow_schema.json | Adds schema support for checkout and checkoutConfig |
| .github/workflows/smoke-codex.md | Adds checkout.fetch-depth as a smoke-test example |
| .github/workflows/smoke-codex.lock.yml | Reflects generated workflow output including fetch-depth: 2 |
| .github/agents/agentic-workflows.agent.md | Updates agent routing documentation (report prompt) |
Comments suppressed due to low confidence (2)
pkg/workflow/checkout_manager.go:88
- The comment says merged checkouts are keyed by
(repository, ref, path, token), but the actual grouping key is only(repository, path)(and ref/token are merged settings). Please update the doc comment to match the implemented behavior to avoid misleading future changes/debugging.
// Merging rules:
// - Checkouts with the same (repository, ref, path, token) are merged into one.
// - The deepest fetch-depth wins: 0 (full history) overrides any shallower value.
// - Sparse-checkout patterns are unioned across merged configs.
// - LFS and submodules are OR-ed (if any request enables them, the result enables them).
pkg/workflow/checkout_manager.go:138
- Submodules merging is not actually OR-ed/unioned: if the first config sets
submodules(including"false"), subsequent configs for the same repo/path can never enable submodules/recursive mode becauseentry.submodulesis only set when empty. If the intended behavior is “enable if any config enables it”, implement a precedence/merge function (e.g.,recursive>true>false> empty) instead of “first non-empty wins”.
entry.lfs = true
}
if cfg.Submodules != "" && entry.submodules == "" {
entry.submodules = cfg.Submodules
}
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
|
||
| // ParseCheckoutConfigs converts a raw frontmatter value (single map or array of maps) | ||
| // into a slice of CheckoutConfig entries. | ||
| // Returns nil if the value is nil or cannot be parsed. |
There was a problem hiding this comment.
The docstring for ParseCheckoutConfigs says it returns nil when the value “cannot be parsed”, but the function actually returns a non-nil error for invalid types/shapes. Update the comment to reflect the real contract (nil only when raw is nil; otherwise errors are returned on invalid input).
This issue also appears on line 84 of the same file.
| // Returns nil if the value is nil or cannot be parsed. | |
| // Returns (nil, nil) if the value is nil; for non-nil values, invalid types or shapes | |
| // result in a non-nil error. |
There was a problem hiding this comment.
Fixed in 4ad61d6 — updated to: "Returns (nil, nil) if the value is nil; for non-nil values, invalid types or shapes result in a non-nil error."
| //go:build !integration | ||
|
|
There was a problem hiding this comment.
This file is guarded by //go:build !integration, but NewCheckoutManager, CheckoutConfig, and ParseCheckoutConfigs are referenced from non-tagged production code (e.g., compiler YAML generation and frontmatter parsing). Building/tests with the integration tag will exclude this file and fail to compile. Remove the build tag (or provide an integration-compatible implementation) so the workflow package builds under -tags=integration.
| //go:build !integration |
There was a problem hiding this comment.
The //go:build !integration tag was only on checkout_manager_test.go (the test file), not on checkout_manager.go itself — the production code has no build tag and compiles under all build configurations.
| if cfg.Ref != "" { | ||
| entry.ref = cfg.Ref // last non-empty ref wins | ||
| } | ||
| if cfg.Token != "" { | ||
| entry.token = cfg.Token // last non-empty token wins | ||
| } |
There was a problem hiding this comment.
Token merging currently uses “last non-empty token wins”, but the PR description states “token is excluded from the grouping key (first-seen wins when merging duplicate targets)”. Please align the implementation with the intended behavior (e.g., only set entry.token if it is empty) or update the PR description/spec so users get predictable results when multiple configs target the same repo/path.
This issue also appears on line 134 of the same file.
There was a problem hiding this comment.
Fixed in 4ad61d6. Both ref and token now use first-seen-wins: if cfg.Ref != "" && entry.ref == "" / if cfg.GitHubToken != "" && entry.token == "". Updated the test that previously asserted last-ref-wins.
| case uint64: | ||
| depth := int(n) | ||
| cfg.FetchDepth = &depth | ||
| case float64: |
There was a problem hiding this comment.
For fetch-depth values parsed as float64, the code does depth := int(n) without validating that n is a whole number. This will silently truncate fractional values (e.g. 1.9 → 1) even though the error message says it must be an integer. Add a check that the float is integral before converting (similar to other parsers in this repo).
| case float64: | |
| case float64: | |
| if n != float64(int64(n)) { | |
| return nil, errors.New("checkout.fetch-depth must be an integer") | |
| } |
There was a problem hiding this comment.
Fixed in 4ad61d6. Added if n != float64(int64(n)) { return nil, errors.New("checkout.fetch-depth must be an integer") } before the conversion.
|
@copilot rename checkout.token to checkout.github-token |
…string, float validation Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Adds a top-level
checkoutfrontmatter field that controls howactions/checkoutis emitted in the agent job.Changes Made
checkout_manager.go: NewCheckoutManagerthat collects and deduplicates checkout configs. Merging rules: deepest fetch-depth wins, sparse-checkout patterns are unioned, LFS/submodules are OR-ed.persist-credentials: falseis always enforced. Token and ref use first-seen-wins when merging duplicate targets.frontmatter_types.go: AddedCheckout any(JSON round-trip) andCheckoutConfigs []*CheckoutConfig(parsed) fields toFrontmatterConfig.compiler_types.go: AddedCheckoutConfigs []*CheckoutConfigtoWorkflowData.compiler_orchestrator_workflow.go: PopulateCheckoutConfigsfrom parsed frontmatter, with a fallback to extractcheckoutdirectly from the raw frontmatter map when full frontmatter parsing fails (e.g. for non-standard tool configs likebash: ["*"]).compiler_yaml_main_job.go: UseCheckoutManagerfor checkout step generation instead of inline YAML.main_workflow_schema.json: Addedcheckoutproperty andcheckoutConfigdefinition.checkout_manager_test.go: Unit tests covering all merging and generation behaviors.smoke-codex.md: Addedcheckout: fetch-depth: 2as a usage example and smoke test.Usage
Key Design Decisions
persist-credentials: falseis always enforced regardless of user inputgithub-token(mapped toactions/checkout'stokeninput)repository+path)path+ emptyrepository) applies user overrides on top of the automatic workspace checkout; trial-mode overrides take precedencefetch-depthinteger values are handled for all YAML numeric types (int,int64,uint64,float64); fractional float values are rejected💡 You can make Copilot smarter by setting up custom instructions, customizing its development environment and configuring Model Context Protocol (MCP) servers. Learn more Copilot coding agent tips in the docs.