Skip to content

fix(workflows): pass ARTIFACTS_DIR/LOG_DIR/BASE_BRANCH to script node subprocesses#1640

Merged
coleam00 merged 1 commit into
devfrom
fix/script-node-artifacts-env
May 11, 2026
Merged

fix(workflows): pass ARTIFACTS_DIR/LOG_DIR/BASE_BRANCH to script node subprocesses#1640
coleam00 merged 1 commit into
devfrom
fix/script-node-artifacts-env

Conversation

@coleam00

@coleam00 coleam00 commented May 11, 2026 •
edited by coderabbitai Bot
Loading

Copy link
Copy Markdown
Owner

Summary

Script nodes weren't getting ARTIFACTS_DIR, LOG_DIR, or BASE_BRANCH in their subprocess environment, while bash nodes were. This caused the marketplace-pr-review-and-merge workflow's parse-entry script node to fail with ENOENT in CI when trying to read .scope-result from artifacts.

Root cause

dag-executor.ts:1320-1326 (bash node) sets all three vars on subprocessEnv. dag-executor.ts:1487-1488 (script node) only conditionally forwards caller-supplied envVars and never sets these three. Locally the issue was masked because some local shells happened to have ARTIFACTS_DIR inherited from prior runs.

Fix

Mirror the bash node env construction in the script node executor.

Verification

  • Type-check passes (all 10 packages)
  • Pre-existing 32 test failures in the workflows package suite are unchanged — they are state-pollution issues in the test runner (script-node-deps tests pass in isolation, same failure count on baseline dev)
  • Will be validated end-to-end by the marketplace auto-review re-running on PR feat(marketplace): add video-generic workflow #1639 after merge

Test plan

Summary by CodeRabbit

  • Bug Fixes
    • Improved environment variable handling in workflow script execution to ensure consistent and explicit setup for all subprocess calls.

Review Change Stack

@coleam00
fix(workflows): pass ARTIFACTS_DIR/LOG_DIR/BASE_BRANCH to script node…
0776b88 
… subprocesses

The bash node executor sets ARTIFACTS_DIR LOG_DIR and BASE_BRANCH on the subprocess (dag-executor.ts:1320-1326), but the script node executor only conditionally forwards caller-supplied variables and never sets these three. Scripts that read ARTIFACTS_DIR from the process environment (e.g. the marketplace-pr-review-and-merge workflow parse-entry node) hit ENOENT in CI because the variable is undefined and resolve with empty string falls back to cwd. Mirror the bash-node executor pattern so script nodes receive the same runtime variables unconditionally.
@coleam00 coleam00 merged commit 78ec70d into dev May 11, 2026
3 of 4 checks passed
@coleam00 coleam00 deleted the fix/script-node-artifacts-env branch May 11, 2026 15:36
@coderabbitai

coderabbitai Bot commented May 11, 2026 •
edited
Loading

Copy link
Copy Markdown

Caution

Review failed

The pull request is closed.

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: e71f6c57-62e2-4817-a2da-780ab61a2479

📥 Commits

Reviewing files that changed from the base of the PR and between 6be6dc8 and 0776b88.

📒 Files selected for processing (1)
  • packages/workflows/src/dag-executor.ts
📝 Walkthrough

Walkthrough

The change modifies executeScriptNode to always provide an explicit environment object to subprocess execution. Previously, the subprocess environment was conditionally set only when caller-provided envVars were present. Now it always merges the current process environment with required workflow variables: ARTIFACTS_DIR, LOG_DIR, and BASE_BRANCH, plus any provided envVars.

Changes

Subprocess Environment Variable Handling

Layer / File(s) Summary
Environment Variable Merging
packages/workflows/src/dag-executor.ts		 executeScriptNode now always constructs and passes subprocessEnv to execFileAsync, merging process.env with required workflow variables and optional envVars (defaulting to {}), rather than conditionally setting env only when envVars is defined.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~8 minutes

Possibly related PRs

  • coleam00/Archon#1387: Similar environment variable merging pattern applied to bash node subprocess execution in the same file.

Poem

🐰 A script runs forth with clearer skies,
Env vars merged—no more surprise!
ARTIFACTS and LOGS now always near,
The subprocess path is crystal clear. ✨

✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch fix/script-node-artifacts-env

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@coderabbitai coderabbitai Bot mentioned this pull request May 12, 2026
Merged
@Wirasm Wirasm mentioned this pull request May 12, 2026
Merged
@coderabbitai coderabbitai Bot mentioned this pull request May 13, 2026
Open
cropse pushed a commit to cropse/Archon that referenced this pull request May 19, 2026
@coleam00 @cropse0219
fix(workflows): pass ARTIFACTS_DIR/LOG_DIR/BASE_BRANCH to script node…
248e89e 
… subprocesses (coleam00#1640)

The bash node executor sets ARTIFACTS_DIR LOG_DIR and BASE_BRANCH on the subprocess (dag-executor.ts:1320-1326), but the script node executor only conditionally forwards caller-supplied variables and never sets these three. Scripts that read ARTIFACTS_DIR from the process environment (e.g. the marketplace-pr-review-and-merge workflow parse-entry node) hit ENOENT in CI because the variable is undefined and resolve with empty string falls back to cwd. Mirror the bash-node executor pattern so script nodes receive the same runtime variables unconditionally.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

1 participant

@coleam00