-
Notifications
You must be signed in to change notification settings - Fork 0
fix: make YAML-relative cwd resolution opt-in #43
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||
|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -860,6 +860,8 @@ export class WorkflowRunner { | |||||||||
| private readonly workspaceId: string; | ||||||||||
| private readonly relayOptions: RuntimeSpawnOptions; | ||||||||||
| private readonly cwd: string; | ||||||||||
| private workflowFileDir?: string; | ||||||||||
| private cwdResolution: NonNullable<RelayYamlConfig['cwdResolution']> = 'process'; | ||||||||||
| private readonly summaryDir: string; | ||||||||||
| private executor?: RunnerStepExecutor; | ||||||||||
| private readonly envSecrets?: Record<string, string>; | ||||||||||
|
|
@@ -1004,6 +1006,50 @@ export class WorkflowRunner { | |||||||||
| }); | ||||||||||
| } | ||||||||||
|
|
||||||||||
| private resolveConfiguredCwd(configuredCwd: string): string { | ||||||||||
| const base = this.cwdResolution === 'workflow-file' ? this.workflowFileDir : this.cwd; | ||||||||||
| return path.resolve(base ?? this.cwd, configuredCwd); | ||||||||||
| } | ||||||||||
|
|
||||||||||
| private configureCwdResolution(config: RelayYamlConfig): void { | ||||||||||
| const configuredMode = config.cwdResolution; | ||||||||||
| this.cwdResolution = configuredMode ?? 'process'; | ||||||||||
|
|
||||||||||
| if (this.cwdResolution === 'workflow-file' && !this.workflowFileDir) { | ||||||||||
| throw new Error( | ||||||||||
| 'cwdResolution: "workflow-file" requires a workflow loaded from a YAML file so its directory is known' | ||||||||||
| ); | ||||||||||
| } | ||||||||||
| if (configuredMode || !this.workflowFileDir) return; | ||||||||||
|
|
||||||||||
| const candidates: Array<{ label: string; cwd: string }> = []; | ||||||||||
| for (const agent of config.agents ?? []) { | ||||||||||
| if (agent.cwd) candidates.push({ label: `Agent "${agent.name}"`, cwd: agent.cwd }); | ||||||||||
| } | ||||||||||
| for (const workflow of config.workflows ?? []) { | ||||||||||
| for (const step of workflow.steps) { | ||||||||||
| if (step.cwd) { | ||||||||||
| candidates.push({ | ||||||||||
| label: `Step "${workflow.name}.${step.name}"`, | ||||||||||
| cwd: step.cwd, | ||||||||||
| }); | ||||||||||
| } | ||||||||||
| } | ||||||||||
| } | ||||||||||
|
|
||||||||||
| for (const candidate of candidates) { | ||||||||||
| const processPath = path.resolve(this.cwd, candidate.cwd); | ||||||||||
| const workflowPath = path.resolve(this.workflowFileDir, candidate.cwd); | ||||||||||
| if (processPath === workflowPath) continue; | ||||||||||
| console.warn( | ||||||||||
| `[WorkflowRunner] DEPRECATION WARNING: ${candidate.label} cwd "${candidate.cwd}" ` + | ||||||||||
| `currently resolves from the process cwd to "${processPath}"; workflow-file resolution ` + | ||||||||||
| `would use "${workflowPath}". Set top-level cwdResolution to "process" or ` + | ||||||||||
| `"workflow-file" explicitly. The default will flip to "workflow-file" in a future major release.` | ||||||||||
| ); | ||||||||||
| } | ||||||||||
| } | ||||||||||
|
|
||||||||||
| /** | ||||||||||
| * Resolve and validate the top-level `paths` definitions from the config. | ||||||||||
| * Returns a map of name → absolute directory path. | ||||||||||
|
|
@@ -1466,7 +1512,7 @@ export class WorkflowRunner { | |||||||||
| return resolved; | ||||||||||
| } | ||||||||||
| if (agent.cwd) { | ||||||||||
| return path.resolve(this.cwd, agent.cwd); | ||||||||||
| return this.resolveConfiguredCwd(agent.cwd); | ||||||||||
| } | ||||||||||
| return this.cwd; | ||||||||||
| } | ||||||||||
|
|
@@ -1488,7 +1534,7 @@ export class WorkflowRunner { | |||||||||
|
|
||||||||||
| private resolveEffectiveCwd(step: WorkflowStep, agentDef?: AgentDefinition): string { | ||||||||||
| if (step.cwd) { | ||||||||||
| return path.resolve(this.cwd, step.cwd); | ||||||||||
| return this.resolveConfiguredCwd(step.cwd); | ||||||||||
| } | ||||||||||
| return this.resolveStepWorkdir(step) ?? (agentDef ? this.resolveAgentCwd(agentDef) : this.cwd); | ||||||||||
| } | ||||||||||
|
|
@@ -2997,11 +3043,15 @@ export class WorkflowRunner { | |||||||||
| async parseYamlFile(filePath: string): Promise<RelayYamlConfig> { | ||||||||||
| const absPath = path.resolve(this.cwd, filePath); | ||||||||||
| const raw = await readFile(absPath, 'utf-8'); | ||||||||||
| this.workflowFileDir = path.dirname(absPath); | ||||||||||
| return this.parseYamlString(raw, absPath); | ||||||||||
| } | ||||||||||
|
|
||||||||||
| /** Parse a relay.yaml string. */ | ||||||||||
| parseYamlString(raw: string, source = '<string>'): RelayYamlConfig { | ||||||||||
| if (source !== '<string>' && path.isAbsolute(source)) { | ||||||||||
| this.workflowFileDir = path.dirname(source); | ||||||||||
| } | ||||||||||
|
Comment on lines
+3052
to
+3054
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. P2: After a runner parses a YAML file, parsing a string config leaves the previous Prompt for AI agents
Suggested change
|
||||||||||
| const parsed = parseYaml(raw); | ||||||||||
| this.validateConfig(parsed, source); | ||||||||||
| const config = this.normalizeLegacyPermissionConfig(parsed as RelayYamlConfig); | ||||||||||
|
|
@@ -3085,6 +3135,15 @@ export class WorkflowRunner { | |||||||||
| if (c.agents !== undefined && !Array.isArray(c.agents)) { | ||||||||||
| throw new Error(`${source}: "agents" must be an array when provided`); | ||||||||||
| } | ||||||||||
| if ( | ||||||||||
| c.cwdResolution !== undefined && | ||||||||||
| c.cwdResolution !== 'process' && | ||||||||||
| c.cwdResolution !== 'workflow-file' | ||||||||||
| ) { | ||||||||||
| throw new Error( | ||||||||||
| `${source}: "cwdResolution" must be either "process" or "workflow-file"` | ||||||||||
| ); | ||||||||||
| } | ||||||||||
|
|
||||||||||
| // Approval gates that cannot reach a human fail OPEN, so they must be refused | ||||||||||
| // on every path — not only when someone happens to run `--validate` first. | ||||||||||
|
|
@@ -3221,6 +3280,7 @@ export class WorkflowRunner { | |||||||||
| this.validateConfig(config); | ||||||||||
| resolved = vars ? this.resolveVariables(config, vars) : config; | ||||||||||
| resolved = this.applyPermissionProfiles(resolved); | ||||||||||
| this.configureCwdResolution(resolved); | ||||||||||
| } catch (err) { | ||||||||||
| errors.push(err instanceof Error ? err.message : String(err)); | ||||||||||
| return { | ||||||||||
|
|
@@ -3335,7 +3395,7 @@ export class WorkflowRunner { | |||||||||
| // Validate cwd paths | ||||||||||
| for (const agent of resolved.agents) { | ||||||||||
| if (agent.cwd) { | ||||||||||
| const resolvedCwd = path.resolve(this.cwd, agent.cwd); | ||||||||||
| const resolvedCwd = this.resolveConfiguredCwd(agent.cwd); | ||||||||||
| if (!existsSync(resolvedCwd)) { | ||||||||||
| warnings.push( | ||||||||||
| `Agent "${agent.name}" cwd "${agent.cwd}" resolves to "${resolvedCwd}" which does not exist` | ||||||||||
|
|
@@ -3845,6 +3905,7 @@ export class WorkflowRunner { | |||||||||
|
|
||||||||||
| // Validate config (catches cycles, missing deps, invalid steps, etc.) | ||||||||||
| this.validateConfig(resolved); | ||||||||||
| this.configureCwdResolution(resolved); | ||||||||||
| const runtimeConfig = this.applyReliabilityDefaults(resolved); | ||||||||||
|
|
||||||||||
| const permissionResult = this.validatePermissions( | ||||||||||
|
|
@@ -4015,6 +4076,7 @@ export class WorkflowRunner { | |||||||||
| const resolvedConfig = this.applyReliabilityDefaults( | ||||||||||
| vars ? this.resolveVariables(run.config, vars) : run.config | ||||||||||
| ); | ||||||||||
| this.configureCwdResolution(resolvedConfig); | ||||||||||
|
|
||||||||||
| // Resolve path definitions (same as execute()) so workdir lookups work on resume | ||||||||||
| const pathResult = this.resolvePathDefinitions(resolvedConfig.paths, this.cwd); | ||||||||||
|
|
||||||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -19,6 +19,12 @@ | |
| "type": "string", | ||
| "description": "Optional description of the configuration" | ||
| }, | ||
| "cwdResolution": { | ||
| "type": "string", | ||
| "enum": ["workflow-file", "process"], | ||
| "default": "process", | ||
| "description": "Base directory for relative agent and step cwd values. process preserves the current behavior; workflow-file resolves from the directory containing this YAML file." | ||
| }, | ||
| "permission_profiles": { | ||
| "type": "object", | ||
| "description": "Reusable permission profiles that agents can reference via permissions.profile.", | ||
|
|
@@ -410,7 +416,7 @@ | |
| }, | ||
| "cwd": { | ||
| "type": "string", | ||
| "description": "Working directory for this agent, resolved relative to the runner's configured working directory (the `cwd` option, defaulting to process.cwd()) — not the YAML file's location." | ||
| "description": "Working directory for this agent, resolved according to the top-level cwdResolution setting." | ||
| }, | ||
| "workdir": { | ||
| "type": "string", | ||
|
|
@@ -836,6 +842,10 @@ | |
| "type": "string", | ||
| "description": "Sets this step's working directory to a named entry from the top-level paths array." | ||
| }, | ||
| "cwd": { | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. P3: WorktreeWorkflowStep, IntegrationWorkflowStep and WaitForWorkflowStep do not declare Prompt for AI agents |
||
| "type": "string", | ||
| "description": "Working directory for this step, resolved according to the top-level cwdResolution setting." | ||
| }, | ||
| "humanAssistance": { | ||
| "anyOf": [ | ||
| { | ||
|
|
@@ -892,6 +902,10 @@ | |
| "workdir": { | ||
| "type": "string", | ||
| "description": "Sets this step's working directory to a named entry from the top-level paths array." | ||
| }, | ||
| "cwd": { | ||
| "type": "string", | ||
| "description": "Working directory for this step, resolved according to the top-level cwdResolution setting." | ||
| } | ||
| } | ||
| }, | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
P3: warn.mockRestore() runs only on the success path. If an assertion fails, the console.warn spy leaks into the rest of the suite and silently swallows warnings from later tests. Wrap the spy registration and assertions in try/finally, or restore the spy after each test.
Prompt for AI agents