Adding in the ability to add a git worktree into the created workspace for a test#154
Adding in the ability to add a git worktree into the created workspace for a test#154richardpark-msft wants to merge 9 commits intomicrosoft:mainfrom
Conversation
|
(just a note, the coding is done, we have tests in place, but I want to see if I can add some more E2E tests before moving this into final review) |
…for a copilot run. This is most useful for people that are developing skills within a repo they're also using the skills - this lets them get a clean copy of the repo for each test run. See 'internal/models/testdata/git-resources-task-example.yaml' for an idea of what the YAML looks like to use it. Future plans are to add in more git clone methods, but this is a nice cheap way of boostrapping a clean copy of a repo, when working locally. We also now validate that the task configuration is valid before starting - prior to this we were apparently just throwing out log messages but no errors, so a user could start with a partially invalid configuration and not realize that critical checks, etc... weren't even being loaded.
bf022a3 to
99ec1b5
Compare
There was a problem hiding this comment.
Pull request overview
This PR adds first-class “git resources” to task inputs so a test run can materialize a clean copy of a local repository (via git worktree) inside the per-run workspace, and tightens up task input validation so invalid file/resource config fails fast instead of only warning.
Changes:
- Extend the task schema and models to support
inputs.repos(git worktree resources) and stricterinputs.filesvalidation. - Introduce
ExecutableTestCaseto pre-validate/materialize file and git resources before execution, and plumb git resources through to the execution engine(s). - Add execution-layer git worktree support plus tests and docs updates.
Reviewed changes
Copilot reviewed 22 out of 22 changed files in this pull request and generated 12 comments.
Show a summary per file
| File | Description |
|---|---|
schemas/task.schema.json |
Adds inputs.repos and tightens resourceRef validation (needs workdir added + stricter gitResource def). |
internal/orchestration/runner.go |
Adds ExecutableTestCase, preloads resources, and passes git resources/workdir into execution requests. |
internal/orchestration/runner_test.go |
Updates runner unit tests to use ExecutableTestCase. |
internal/orchestration/runner_orchestration_test.go |
Updates resource-loading tests for new (resources, gitResources, err) signature. |
internal/orchestration/filter.go |
Updates filtering helpers to operate on ExecutableTestCase. |
internal/orchestration/filter_test.go |
Updates filter tests for ExecutableTestCase. |
internal/models/testcase.go |
Adds git resource types and ResourceRef.Validate (missing WorkDir field currently blocks compilation). |
internal/models/testcase_test.go |
Adds tests for git resources, workdir, and ResourceRef validation; refactors trigger YAMLs into files. |
internal/models/testdata/* |
Adds example task YAMLs for git resources, file resources, triggers, and workdir. |
internal/execution/engine.go |
Adds GitResources + WorkDir to ExecutionRequest and introduces ResolveWorkDir. |
internal/execution/copilot.go |
Plumbs git resources into workspace setup and adds cleanup tracking (but still doesn’t apply WorkDir). |
internal/execution/mock.go |
Adds git resource materialization/cleanup in mock engine (but changes WorkspaceDir semantics incorrectly). |
internal/execution/git.go |
Implements git worktree materialization and cleanup helpers. |
internal/execution/git_test.go |
Adds tests for git worktree behavior and workdir resolution (needs git author config for reliability). |
internal/execution/generate.go |
Adds mock generation for GitResource. |
internal/execution/execution_mocks.go |
Generated GoMock for GitResource. |
internal/execution/engine_shutdown_test.go |
Extends shutdown tests to ensure git resources are cleaned up. |
README.md |
Documents git resources + workdir usage (cleanup timing is currently misstated). |
Comments suppressed due to low confidence (2)
schemas/task.schema.json:97
- Task YAML/examples now use
inputs.workdir, but the task schema doesn’t define aworkdirproperty underinputs. Add it (as a string) so schema validation and editor tooling match the documented behavior.
"properties": {
"prompt": {
"type": "string",
"description": "The prompt message sent to the agent."
},
"context": {
"type": "object",
"description": "Arbitrary key-value metadata passed to the agent."
},
"files": {
"type": "array",
"items": {
"$ref": "#/$defs/resourceRef"
},
"description": "File references or inline content provided to the agent."
},
"repos": {
"type": "array",
"items": {
"$ref": "#/$defs/gitResource"
},
"description": "Git repositories provided to the agent workspace."
},
"environment": {
"type": "object",
"additionalProperties": {
"type": "string"
},
"description": "Environment variables set during task execution."
}
internal/models/testcase.go:33
TestStimulusis missing aWorkDirfield, but the new YAML examples/tests (andrunner.goviatc.Stimulus.WorkDir) rely on it. This will not compile andworkdir:in task YAML will be ignored until aWorkDir stringyaml:"workdir,omitempty"`` field is added toTestStimulus.
// TestStimulus defines the input for a test
type TestStimulus struct {
Message string `yaml:"prompt" json:"message"`
Metadata map[string]any `yaml:"context,omitempty" json:"metadata,omitempty"`
Resources []ResourceRef `yaml:"files,omitempty" json:"resources,omitempty"`
Repos []GitResource `yaml:"repos,omitempty" json:"repos,omitempty"`
Environment map[string]string `yaml:"environment,omitempty" json:"environment,omitempty"`
}
| "gitResource": { | ||
| "type": "object", | ||
| "description": "A git repository checked out at a specific commit.", | ||
| "required": [ | ||
| "type" | ||
| ], | ||
| "properties": { | ||
| "commit": { | ||
| "type": "string", | ||
| "description": "Git commit SHA, branch, or tag to check out. Defaults to HEAD." | ||
| }, | ||
| "type": { | ||
| "type": "string", | ||
| "enum": [ | ||
| "worktree" | ||
| ], | ||
| "description": "Git resource type. Currently only 'worktree' is supported." | ||
| }, | ||
| "source": { | ||
| "type": "string", | ||
| "description": "Type-specific source location. For 'worktree', this is the folder where the git repository resides." | ||
| }, | ||
| "dest": { | ||
| "type": "string", | ||
| "description": "Subdirectory name within the workspace. If omitted, the git repo becomes the workspace root." | ||
| } | ||
| }, | ||
| "allOf": [ | ||
| { | ||
| "if": { | ||
| "properties": { | ||
| "type": { | ||
| "const": "worktree" | ||
| } | ||
| }, | ||
| "required": [ | ||
| "type" | ||
| ] | ||
| }, | ||
| "then": { | ||
| "required": [ | ||
| "source" | ||
| ] | ||
| } | ||
| } | ||
| ] | ||
| } |
There was a problem hiding this comment.
$defs.gitResource currently allows unknown keys because it lacks "additionalProperties": false (many other object defs in this schema are strict). Consider adding additionalProperties:false and (optionally) minLength: 1 for required strings like source to catch typos early.
| // Clean up worktrees before removing workspaces (worktrees may be inside workspace dirs) | ||
| for _, wt := range gitResources { | ||
| if err := wt.Cleanup(ctx); err != nil { | ||
| slog.Warn("failed to cleanup git resource", "error", err) | ||
| } | ||
| } |
There was a problem hiding this comment.
doShutdown cleans up git resources using the caller-provided ctx. If Shutdown is invoked with a canceled/deadline-exceeded context, git cleanup will be skipped/aborted, leaving stale worktrees behind. Consider using a background/best-effort context (or a fresh timeout) for cleanup operations.
| wts, err := CloneGitResources(ctx, gitResources, workspaceDir) | ||
| if err != nil { | ||
| return "", err | ||
| } | ||
| if len(wts) > 0 { | ||
| e.resourcesMu.Lock() | ||
| e.gitResources = append(e.gitResources, wts...) | ||
| e.resourcesMu.Unlock() | ||
| } |
There was a problem hiding this comment.
If CloneGitResources fails, setupWorkspace returns without cleaning up the newly created temp workspace (already appended to e.workspaces) and without cleaning any partially-created git resources. Consider cleaning up immediately on error (and/or have CloneGitResources rollback created resources) to avoid leaking temp dirs/worktrees when Execute fails early.
| // Materialize git resources | ||
| wts, err := CloneGitResources(ctx, req.GitResources, m.workspace) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("failed to materialize git resource in mock workspace: %w", err) | ||
| } | ||
| m.gitResources = append(m.gitResources, wts...) | ||
|
|
There was a problem hiding this comment.
m.gitResources is appended to on each Execute, but prior worktrees are never cleaned up when a new workspace replaces the old one (the old workspace dir is deleted earlier, but the source repo will still track the worktree). Consider cleaning up any existing m.gitResources before creating/materializing new git resources, and reset the slice per-execute.
| ToolCalls: []models.ToolCall{}, | ||
| Success: true, | ||
| WorkspaceDir: m.workspace, | ||
| } | ||
|
|
||
| return resp, nil | ||
| } | ||
|
|
||
| func (m *MockEngine) Shutdown(ctx context.Context) error { |
There was a problem hiding this comment.
MockEngine sets ExecutionResponse.WorkspaceDir to the resolved workdir (agentCwd). WorkspaceDir is used by graders/tests as the workspace root; changing it breaks existing expectations (e.g., tests that read resp.WorkspaceDir/fixtures/...). Keep WorkspaceDir as the workspace root and, if needed, introduce a separate field for the agent CWD.
internal/execution/git.go
Outdated
| targetDir := workspaceDir | ||
|
|
||
| if gitRes.Dest != "" { | ||
| targetDir = filepath.Join(workspaceDir, gitRes.Dest) |
There was a problem hiding this comment.
CloneGitResource joins workspaceDir with gitRes.Dest without validating that Dest stays within the workspace (e.g., dest: "../../.." would write outside the sandbox). Please validate Dest is relative/non-traversing (similar to ResolveWorkDir) before constructing targetDir.
| targetDir = filepath.Join(workspaceDir, gitRes.Dest) | |
| cleanDest := filepath.Clean(gitRes.Dest) | |
| // Ensure destination is a relative, non-traversing path within the workspace. | |
| if filepath.IsAbs(cleanDest) || | |
| cleanDest == ".." || | |
| strings.HasPrefix(cleanDest, ".."+string(os.PathSeparator)) { | |
| return nil, fmt.Errorf("invalid git destination path %q", gitRes.Dest) | |
| } | |
| targetDir = filepath.Join(workspaceDir, cleanDest) |
…t do the final validation at this point.
…folder with GitResource.
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #154 +/- ##
=======================================
Coverage ? 73.42%
=======================================
Files ? 143
Lines ? 15999
Branches ? 0
=======================================
Hits ? 11747
Misses ? 3394
Partials ? 858
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 23 out of 23 changed files in this pull request and generated 11 comments.
Comments suppressed due to low confidence (1)
internal/execution/git.go:78
git worktree add <path> <commit>will check out a branch ifcommitis a branch name (e.g.main). That commonly fails because the branch is already checked out in the source repo (“already checked out at ...”). Since the schema/docs allow branch/tag names, consider always using--detachwhen acommitis provided (i.e.,git worktree add --detach <path> <commit-ish>) so branch names work reliably without conflicts.
// gitWorkTreeAdd runs 'git worktree add', creating a git worktree (an incredibly cheap copy) of a local repo to
// another local path on disk. Note, this requires a local clone of a git repo to work.
func gitWorkTreeAdd(ctx context.Context, commit string, repoDir string, targetDir string) (*GitWorkTree, error) {
args := []string{"worktree", "add"}
if commit != "" {
// git worktree add <path> <commit>
args = append(args, targetDir, commit)
} else {
// git worktree add --detach <path> (uses HEAD)
args = append(args, "--detach", targetDir)
}
if _, err := runGitCommand(ctx, repoDir, args...); err != nil {
return nil, fmt.Errorf("git worktree add failed: %w", err)
}
| case models.GitTypeWorktree: | ||
| targetDir := workspaceDir | ||
|
|
||
| if gitRes.Dest != "" { | ||
| targetDir = filepath.Join(workspaceDir, gitRes.Dest) | ||
| } | ||
|
|
||
| // Only worktree is supported; Validate() already enforces this. | ||
| return gitWorkTreeAdd(ctx, gitRes.Commit, gitRes.Source, targetDir) | ||
| default: |
There was a problem hiding this comment.
When gitRes.Dest is empty, targetDir is set to workspaceDir, but setupWorkspace() creates that directory ahead of time. git worktree add <path> fails if the target directory already exists, so the documented “omit dest to use workspace root” behavior won’t work. Consider creating the workspace as a parent dir and adding the worktree into a non-existent child dir, or removing/renaming the pre-created workspace dir before running git worktree add (and updating the returned workspace root accordingly).
This issue also appears on line 63 of the same file.
|
|
||
| if err := validateGitSourceDir(context.Background(), sourceDir); err != nil { | ||
| errs = append(errs, fmt.Errorf("%q source dir is invalid: %w", repo.Source, err)) | ||
| } else { |
There was a problem hiding this comment.
sourceDir is computed/validated, but the original repo (with the original repo.Source) is what gets appended to gitResources. If repo.Source is relative, it will later be interpreted relative to the process working directory in execution.CloneGitResource, not relative to the task file path used for validation. Either normalize repo.Source (e.g., assign repo.Source = sourceDir) before appending, or ensure CloneGitResource resolves relative sources consistently.
| } else { | |
| } else { | |
| // Normalize the repo source to the validated, task-relative directory | |
| repo.Source = sourceDir |
| // Write resource files to workspace | ||
| if err := setupWorkspaceResources(workspaceDir, resources); err != nil { | ||
| return "", fmt.Errorf("failed to setup resources at workspace %s: %w", workspaceDir, err) | ||
| } | ||
|
|
||
| wts, err := CloneGitResources(ctx, gitResources, workspaceDir) | ||
| if err != nil { | ||
| return "", err | ||
| } |
There was a problem hiding this comment.
On CloneGitResources error, this returns immediately without cleaning up the temp workspace directory and without cleaning up any worktrees created before the failure. This can leak both the workspace dir and git worktrees until engine shutdown (and in the worktree case, may persist in the source repo if the process exits). Consider doing best-effort cleanup (worktrees + workspace) before returning the error, and only appending the workspace to e.workspaces after setup succeeds.
See below for a potential fix:
func (e *CopilotEngine) setupWorkspace(ctx context.Context, resources []ResourceFile, gitResources []models.GitResource) (workspaceDir string, err error) {
workspaceDir, err = os.MkdirTemp("", "waza-*")
if err != nil {
return "", fmt.Errorf("failed to create temp workspace: %w", err)
}
// Track any git resources created during setup so we can best-effort
// clean them up if a subsequent step fails.
var wts []GitResource
// Best-effort cleanup on failure: remove any git resources created so far
// and delete the temporary workspace directory.
defer func() {
if err != nil {
for _, wt := range wts {
if cerr := wt.Cleanup(ctx); cerr != nil {
slog.Warn("failed to cleanup git resource after setup failure", "error", cerr)
}
}
if rmErr := os.RemoveAll(workspaceDir); rmErr != nil {
slog.Warn("failed to remove workspace after setup failure", "dir", workspaceDir, "error", rmErr)
}
}
}()
// Write resource files to workspace
if err = setupWorkspaceResources(workspaceDir, resources); err != nil {
err = fmt.Errorf("failed to setup resources at workspace %s: %w", workspaceDir, err)
return "", err
}
wts, err = CloneGitResources(ctx, gitResources, workspaceDir)
if err != nil {
return "", err
}
if len(wts) > 0 {
e.resourcesMu.Lock()
e.gitResources = append(e.gitResources, wts...)
e.resourcesMu.Unlock()
}
e.resourcesMu.Lock()
e.workspaces = append(e.workspaces, workspaceDir)
e.resourcesMu.Unlock()
Rename 'Dest' to 'RelativeDest', just to make it obvious.
| args := []string{"worktree", "add"} | ||
|
|
||
| if commit != "" { | ||
| // git worktree add <path> <commit> | ||
| args = append(args, targetDir, commit) | ||
| } else { | ||
| // git worktree add --detach <path> (uses HEAD) | ||
| args = append(args, "--detach", targetDir) | ||
| } |
There was a problem hiding this comment.
When commit is non-empty, gitWorkTreeAdd runs git worktree add <path> <commit>, which will fail if <commit> is a branch currently checked out in the main worktree (common case when users specify commit: main). To support branch/tag names as documented, consider using --detach even when a commit-ish is provided (e.g., git worktree add --detach <path> <commit>), or otherwise ensure the branch-checkout conflict is handled.
| func TestResolveWorkDir(t *testing.T) { | ||
| tests := []struct { | ||
| name string | ||
| workDir string | ||
| want string | ||
| wantErr bool | ||
| }{ | ||
| { | ||
| name: "empty returns workspace root", | ||
| workDir: "", | ||
| want: "/workspace", | ||
| }, | ||
| { | ||
| name: "subdirectory", | ||
| workDir: "my-repo", | ||
| want: "/workspace/my-repo", | ||
| }, | ||
| { | ||
| name: "nested subdirectory", | ||
| workDir: "repos/my-repo", | ||
| want: "/workspace/repos/my-repo", | ||
| }, |
There was a problem hiding this comment.
TestResolveWorkDir hardcodes POSIX-style expected paths (e.g. /workspace/my-repo). Since CI runs tests on Windows too, filepath.Join will produce Windows separators and these assertions will fail. Build want values using filepath.Join (or normalize path separators before comparing) to keep the test cross-platform.
| inputs: | ||
| prompt: "Fix the bug in server.go" | ||
| workdir: my-repo # agent starts inside this subdirectory | ||
| files: | ||
| # Existing resource types still work: | ||
| - path: helpers/utils.js # file from context_dir | ||
| - content: "package main\n..." # inline content | ||
|
|
||
| repos: | ||
| # Git resource — checkout a commit from a local repo | ||
| - type: worktree # required (currently only worktree is supported) | ||
| source: /path/to/local/repo # required for worktree strategy | ||
| commit: abc123def | ||
| dest: my-repo # optional: subdirectory in workspace | ||
| ``` | ||
|
|
||
| **`workdir`** (optional): A relative path within the workspace to use as the agent's working directory. When a git resource is checked out into a subdirectory via `dest`, set `workdir` to that subdirectory so the agent starts inside the repo. Must not escape the workspace root. | ||
|
|
There was a problem hiding this comment.
README documents an inputs.workdir field, but there is no corresponding field in the task schema (schemas/task.schema.json) or model (internal/models/testcase.go), and it isn’t wired through ExecutionRequest. As written, users can’t actually configure workdir despite the docs. Either remove/adjust this section, or implement and document the end-to-end support for workdir consistently (schema + models + runner/engine).
| // Validate checks that at least one file reference field is specified. | ||
| func (r *ResourceRef) Validate() error { | ||
| if r.Location == "" && r.Body == "" { | ||
| return fmt.Errorf("resource must specify one of: path or content") | ||
| } |
There was a problem hiding this comment.
ResourceRef.Validate allows content without a path, but setupWorkspaceResources ignores resource files with an empty Path, so this input will be silently dropped. If inline content is meant to create a file in the workspace, validation (and the JSON schema) should require path when content is provided, or alternatively treat a missing path as an error during resource loading.
| // Validate checks that at least one file reference field is specified. | |
| func (r *ResourceRef) Validate() error { | |
| if r.Location == "" && r.Body == "" { | |
| return fmt.Errorf("resource must specify one of: path or content") | |
| } | |
| // Validate checks that a valid file reference is specified. | |
| func (r *ResourceRef) Validate() error { | |
| if r.Location == "" && r.Body == "" { | |
| return fmt.Errorf("resource must specify one of: path or content") | |
| } | |
| if r.Location == "" && r.Body != "" { | |
| return fmt.Errorf("resource must specify path when content is provided") | |
| } |
| Type GitType `yaml:"type" json:"type"` | ||
|
|
||
| // Source varies, depending on the type. | ||
| // - For 'worktree', Source is the folder where the git repository resides. If empty, uses the current directory. |
There was a problem hiding this comment.
The comment on GitResource.Source says that if it’s empty it "uses the current directory", but the task schema requires source for worktree repos and the orchestration code resolves/validates it differently. Please align the comment with actual supported behavior (either make source truly optional, or update the comment/docs/schema to reflect it’s required).
| // - For 'worktree', Source is the folder where the git repository resides. If empty, uses the current directory. | |
| // - For 'worktree', Source is the folder where the git repository resides. This is required and must not be empty. |
| // mustCreateRepo creates a repo with a single commit, with 'test.txt' in the root (contents: "hello world") | ||
| func mustCreateRepo(t *testing.T) (repoDir string, headCommitSHA string) { | ||
| repoDir = t.TempDir() | ||
|
|
||
| _, err := runGitCommand(context.Background(), repoDir, "init") | ||
| require.NoError(t, err) | ||
|
|
||
| err = os.WriteFile(filepath.Join(repoDir, "hello.txt"), []byte("hello world"), 0644) | ||
| require.NoError(t, err) |
There was a problem hiding this comment.
The comment on mustCreateRepo says it creates test.txt, but the helper actually writes hello.txt. This can confuse future maintenance/debugging of these tests; please update the comment to match the file that’s created.
|
Going to take another look at something, I think this PR might be too general and it could be more targeted. |
Adding in the ability to add a git worktree into a workspace created for a copilot run. This is most useful for people that are developing skills within a repo they're also using the skills - this lets them get a clean copy of the repo for each test run. See 'internal/models/testdata/git-resources-task-example.yaml' for an idea of what the YAML looks like to use it.
Future plans are to add in more git clone methods, but this is a nice cheap way of boostrapping a clean copy of a repo, when working locally.
We also now validate that the task configuration is valid before starting - prior to this we were apparently just throwing out log messages but no errors, so a user could start with a partially invalid configuration and not realize that critical checks, etc... weren't even being loaded.
Fixes #121