fix: make YAML-relative cwd resolution opt-in - #43
Conversation
Session-Id: 01a037bb-4e8c-7c20-8962-621515f5e335
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
3 issues found across 5 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="packages/core/src/__tests__/step-cwd.test.ts">
<violation number="1" location="packages/core/src/__tests__/step-cwd.test.ts:174">
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.</violation>
</file>
<file name="packages/core/src/runner.ts">
<violation number="1" location="packages/core/src/runner.ts:3052">
P2: After a runner parses a YAML file, parsing a string config leaves the previous `workflowFileDir` in place, so `cwdResolution: workflow-file` resolves against the wrong file. Clear `workflowFileDir` when the source is not an absolute YAML path before configuring the mode.</violation>
</file>
<file name="packages/core/src/schema.json">
<violation number="1" location="packages/core/src/schema.json:845">
P3: WorktreeWorkflowStep, IntegrationWorkflowStep and WaitForWorkflowStep do not declare `cwd` and set additionalProperties: false, yet the runtime (resolveEffectiveCwd) and the WorkflowStep type (schema.ts) support `cwd` on those step types too. A worktree/integration/waitFor step with `cwd` is valid and honored at runtime but flagged invalid by JSON-Schema editor validation. Add `cwd` to those three step definitions to keep the schema aligned with the resolver and the shared WorkflowStep type.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
| if (source !== '<string>' && path.isAbsolute(source)) { | ||
| this.workflowFileDir = path.dirname(source); | ||
| } |
There was a problem hiding this comment.
P2: After a runner parses a YAML file, parsing a string config leaves the previous workflowFileDir in place, so cwdResolution: workflow-file resolves against the wrong file. Clear workflowFileDir when the source is not an absolute YAML path before configuring the mode.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/core/src/runner.ts, line 3052:
<comment>After a runner parses a YAML file, parsing a string config leaves the previous `workflowFileDir` in place, so `cwdResolution: workflow-file` resolves against the wrong file. Clear `workflowFileDir` when the source is not an absolute YAML path before configuring the mode.</comment>
<file context>
@@ -2997,11 +3043,15 @@ export class WorkflowRunner {
/** Parse a relay.yaml string. */
parseYamlString(raw: string, source = '<string>'): RelayYamlConfig {
+ if (source !== '<string>' && path.isAbsolute(source)) {
+ this.workflowFileDir = path.dirname(source);
+ }
</file context>
| if (source !== '<string>' && path.isAbsolute(source)) { | |
| this.workflowFileDir = path.dirname(source); | |
| } | |
| this.workflowFileDir = source !== '<string>' && path.isAbsolute(source) ? path.dirname(source) : undefined; |
| const workflowDir = '/workflow/config'; | ||
| const runner = new WorkflowRunner({ cwd: runnerRoot }); | ||
| (runner as any).workflowFileDir = workflowDir; | ||
| const warn = vi.spyOn(console, 'warn').mockImplementation(() => {}); |
There was a problem hiding this comment.
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
Check if this issue is valid — if so, understand the root cause and fix it. At packages/core/src/__tests__/step-cwd.test.ts, line 174:
<comment>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.</comment>
<file context>
@@ -83,4 +106,82 @@ describe('WorkflowRunner step cwd resolution', () => {
+ const workflowDir = '/workflow/config';
+ const runner = new WorkflowRunner({ cwd: runnerRoot });
+ (runner as any).workflowFileDir = workflowDir;
+ const warn = vi.spyOn(console, 'warn').mockImplementation(() => {});
+
+ (runner as any).configureCwdResolution(config());
</file context>
| "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.
P3: WorktreeWorkflowStep, IntegrationWorkflowStep and WaitForWorkflowStep do not declare cwd and set additionalProperties: false, yet the runtime (resolveEffectiveCwd) and the WorkflowStep type (schema.ts) support cwd on those step types too. A worktree/integration/waitFor step with cwd is valid and honored at runtime but flagged invalid by JSON-Schema editor validation. Add cwd to those three step definitions to keep the schema aligned with the resolver and the shared WorkflowStep type.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/core/src/schema.json, line 845:
<comment>WorktreeWorkflowStep, IntegrationWorkflowStep and WaitForWorkflowStep do not declare `cwd` and set additionalProperties: false, yet the runtime (resolveEffectiveCwd) and the WorkflowStep type (schema.ts) support `cwd` on those step types too. A worktree/integration/waitFor step with `cwd` is valid and honored at runtime but flagged invalid by JSON-Schema editor validation. Add `cwd` to those three step definitions to keep the schema aligned with the resolver and the shared WorkflowStep type.</comment>
<file context>
@@ -836,6 +842,10 @@
"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."
</file context>
2 participants
Summary
cwdResolution: "workflow-file" | "process"cwdfrom the YAML directory only when explicitly opted inThis PR is stacked on
ci/pr-test-gate-0825so pull-request CI covers it. It should be retargeted tomainafter PR #42 merges.Release note
Workflows can now opt into YAML-file-relative agent and step working directories with
cwdResolution: workflow-file; usecwdResolution: processto pin the legacy behavior. The default remains process-relative in this release, but ambiguous omitted configurations emit a deprecation warning and will flip in a future major. This prevents a silent path mismatch seen by the Native design partner, wherecwd: ..selected an unrelated 5.9GB directory and increased a tick from 115k tokens to 326k tokens.Which execution paths do these tests cover?
cwdResolution: workflow-file: both agentcwdand stepcwdresolve from the YAML parent directory.cwdResolution: process: both agent and step paths retain runner/process-relative behavior.cwdResolution: the compatibility warning includes the concrete current and future paths plus the future-major default flip.A green suite proves the paths that have tests, and nothing else.
Verification
step-cwd.test.ts: exit 0, 6/6 testsnpm test: exit 0, 951/951 testsnpm run typecheck: exit 0