path and workdir for workflows#488
Conversation
xkonjin
left a comment
There was a problem hiding this comment.
Quick review pass:
- Main risk area here is connection lifecycle, retry behavior, and state re-initialization.
- I didn’t see targeted regression coverage in the diff; please add or point CI at a focused test for the changed path in init.py, types.py, schema.json (+1 more).
- Before merge, I’d smoke-test the behavior touched by init.py, types.py, schema.json (+1 more) with malformed input / retry / rollback cases, since that’s where this class of change usually breaks.
The types and schema for `paths` (top-level) and `workdir` (agents/steps) were added in 75337dd but had no runtime implementation. This adds: - Path resolution with env var expansion ($HOME, $VAR) - Preflight validation in dryRun() (required/optional, duplicate names) - workdir→cwd mapping for agent steps (both PTY and non-interactive) - workdir→cwd mapping for deterministic steps - Step-level workdir override for agent steps - Includes real E2E test workflow for cross-repo validation Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…resume Fixes two Devin review findings: 1. workdir missing from AgentWorkflowStep, DeterministicWorkflowStep, WorktreeWorkflowStep in schema.json (additionalProperties: false would reject workdir on steps) 2. resume() never called resolvePathDefinitions(), so workdir lookups would fail on resumed workflow runs
# Conflicts: # .gitignore
executeWorktreeStep now uses resolveStepWorkdir() like deterministic and agent steps, so worktree operations happen in the correct repo when workdir references a named path.
| // Build the git worktree command | ||
| // If createBranch is true and branch doesn't exist, use -b flag | ||
| const absoluteWorktreePath = path.resolve(this.cwd, worktreePath); | ||
| const absoluteWorktreePath = path.resolve(stepCwd, worktreePath); |
Check warning
Code scanning / CodeQL
Unsafe shell command constructed from library input Medium
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI about 2 months ago
General approach: avoid passing dynamic, potentially untrusted strings to a shell interpreter. Instead of building full command lines and executing them via sh -c, call git directly with an argument array using cpSpawn('git', ['worktree', 'add', ...]). This ensures Node passes the arguments directly to git without an intermediate shell, eliminating shell metacharacter interpretation and fixing all CodeQL variants in one place.
Concrete fix in this code:
-
Replace the
checkBranchCmdstring and its execution viacpSpawn('sh', ['-c', checkBranchCmd], ...)with a directgitinvocation:- Use
cpSpawn('git', ['rev-parse', '--verify', '--quiet', branch], { cwd: stepCwd, env: ... }). - Keep the same
branchExistsboolean logic based on exit code.
- Use
-
Replace construction of
worktreeCmdas a string and the subsequentcpSpawn('sh', ['-c', worktreeCmd], ...)with array-formgitcalls:- Build a
const worktreeArgs: string[]:- If
branchExists:['worktree', 'add', absoluteWorktreePath, branch]. - Else if
createBranch:['worktree', 'add', '-b', branch, absoluteWorktreePath, baseBranch].
- If
- Spawn via
cpSpawn('git', worktreeArgs, { cwd: stepCwd, env: ... }). - Preserve stdout/stderr collection, abort handling, and exit-code checking logic unchanged.
- Build a
-
Because we already import
spawn as cpSpawnfromnode:child_process, no new imports are required. We are only changing how we callcpSpawn, not its signature.
No changes are required in builder.ts or run.ts; hardening the sink (executeWorktreeStep in runner.ts) resolves the entire taint path.
| @@ -2048,12 +2048,11 @@ | ||
| // If createBranch is true and branch doesn't exist, use -b flag | ||
| const absoluteWorktreePath = path.resolve(stepCwd, worktreePath); | ||
|
|
||
| // First, check if the branch already exists | ||
| const checkBranchCmd = `git rev-parse --verify --quiet ${branch} 2>/dev/null`; | ||
| // First, check if the branch already exists using a direct git invocation | ||
| let branchExists = false; | ||
|
|
||
| await new Promise<void>((resolve) => { | ||
| const checkChild = cpSpawn('sh', ['-c', checkBranchCmd], { | ||
| const checkChild = cpSpawn('git', ['rev-parse', '--verify', '--quiet', branch], { | ||
| stdio: 'pipe', | ||
| cwd: stepCwd, | ||
| env: { ...process.env }, | ||
| @@ -2065,14 +2061,14 @@ | ||
| checkChild.on('error', () => resolve()); | ||
| }); | ||
|
|
||
| // Build appropriate worktree add command | ||
| let worktreeCmd: string; | ||
| // Build appropriate worktree add command arguments | ||
| let worktreeArgs: string[]; | ||
| if (branchExists) { | ||
| // Branch exists, just checkout into worktree | ||
| worktreeCmd = `git worktree add "${absoluteWorktreePath}" ${branch}`; | ||
| worktreeArgs = ['worktree', 'add', absoluteWorktreePath, branch]; | ||
| } else if (createBranch) { | ||
| // Create new branch from baseBranch | ||
| worktreeCmd = `git worktree add -b ${branch} "${absoluteWorktreePath}" ${baseBranch}`; | ||
| worktreeArgs = ['worktree', 'add', '-b', branch, absoluteWorktreePath, baseBranch]; | ||
| } else { | ||
| // Branch doesn't exist and we're not creating it | ||
| const errorMsg = `Branch "${branch}" does not exist and createBranch is false`; | ||
| @@ -2081,7 +2073,7 @@ | ||
| } | ||
|
|
||
| const output = await new Promise<string>((resolve, reject) => { | ||
| const child = cpSpawn('sh', ['-c', worktreeCmd], { | ||
| const child = cpSpawn('git', worktreeArgs, { | ||
| stdio: 'pipe', | ||
| cwd: stepCwd, | ||
| env: { ...process.env }, |
| // Build the git worktree command | ||
| // If createBranch is true and branch doesn't exist, use -b flag | ||
| const absoluteWorktreePath = path.resolve(this.cwd, worktreePath); | ||
| const absoluteWorktreePath = path.resolve(stepCwd, worktreePath); |
Check warning
Code scanning / CodeQL
Unsafe shell command constructed from library input Medium
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI about 2 months ago
In general, unsafe shell command construction should be fixed by avoiding sh -c with string interpolation and instead passing arguments as an array to spawn/execFile, or—if a shell is necessary—by rigorously escaping any interpolated data using a trustworthy quoting library such as shell-quote. Here, the git worktree operation does not require shell features like pipes or redirections, so the best fix is to call git directly and pass branch and path as separate arguments.
Concretely, in executeWorktreeStep we should:
- Stop building a
worktreeCmdstring that embedsabsoluteWorktreePath. - Replace
cpSpawn('sh', ['-c', worktreeCmd], …)with a directcpSpawn('git', ['worktree', 'add', …], …)call, whereabsoluteWorktreePathandbranchare passed as separate array elements. - Keep the surrounding promise/stream handling unchanged so behavior and logging remain the same.
- We do not need new imports because
cpSpawnis already imported and we don’t introduce new libraries.
The change is localized to the region where worktreeCmd is used: lines around 2046–2089, ensuring we don’t alter other behavior like workdir selection or environment handling.
| @@ -2081,7 +2081,16 @@ | ||
| } | ||
|
|
||
| const output = await new Promise<string>((resolve, reject) => { | ||
| const child = cpSpawn('sh', ['-c', worktreeCmd], { | ||
| const args = ['worktree', 'add']; | ||
| if (createBranch && !branchExists) { | ||
| args.push('-b', branch); | ||
| } | ||
| args.push(absoluteWorktreePath); | ||
| if (!createBranch || branchExists) { | ||
| args.push(branch); | ||
| } | ||
|
|
||
| const child = cpSpawn('git', args, { | ||
| stdio: 'pipe', | ||
| cwd: stepCwd, | ||
| env: { ...process.env }, |
| { | ||
| "$schema": "http://json-schema.org/draft-07/schema#", | ||
| "$id": "RelayYamlConfig", |
There was a problem hiding this comment.
🔴 schema.json is invalid JSON due to plain-text lines prepended before the JSON object
Lines 1-3 of packages/sdk/src/workflows/schema.json contain plain-text comments (Added workdir to AgentWorkflowStep, etc.) before the JSON body starts on line 4. This makes the entire file unparseable as JSON, which breaks any consumer that tries to load this schema — including editor autocompletion, JSON Schema validators, and the example at packages/sdk/src/examples/workflow-superiority.ts:41 that references this file.
| { | |
| "$schema": "http://json-schema.org/draft-07/schema#", | |
| "$id": "RelayYamlConfig", | |
| { |
Was this helpful? React with 👍 or 👎 to provide feedback.
Summary
Test Plan
Screenshots