Conversation
…unner Agent-Logs-Url: https://github.com/github/gh-aw/sessions/f7a1d9ec-effe-4298-9833-3ac09ce84ecd Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Agent-Logs-Url: https://github.com/github/gh-aw/sessions/f7a1d9ec-effe-4298-9833-3ac09ce84ecd Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
|
@copilot if safe_outputs job needs @actions/github on a custom runner, emit the @actions/setup-node step before action/setup |
There was a problem hiding this comment.
Pull request overview
Updates the workflow compiler runtime detection to automatically inject a Node.js setup requirement when compiling workflows that target non-standard/custom runners where Node may not be preinstalled.
Changes:
- Add
isCustomImageRunnerand use it inDetectRuntimeRequirementsto ensure anoderuntime requirement is added for custom runners. - Add unit + integration tests validating runner detection and Node setup injection behavior.
- Recompile affected lock workflows so agent jobs include
actions/setup-node@v6with Node 24 on custom runners.
Show a summary per file
| File | Description |
|---|---|
pkg/workflow/runtime_detection.go |
Adds custom-runner detection and injects a Node runtime requirement when detected. |
pkg/workflow/runtime_setup_test.go |
Adds unit tests for isCustomImageRunner and runtime requirement injection/deduping. |
pkg/workflow/runtime_setup_integration_test.go |
Adds integration coverage ensuring compiled lock output includes/excludes Node setup appropriately. |
.github/workflows/daily-news.lock.yml |
Recompiled lockfile now includes Node setup step for the agent job on the custom GPU runner. |
.github/workflows/daily-fact.lock.yml |
Recompiled lockfile now includes Node setup step for the agent job on the custom GPU runner. |
Copilot's findings
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 5/5 changed files
- Comments generated: 4
| const keyPrefix = "runs-on: " | ||
| if value, ok := strings.CutPrefix(runsOn, keyPrefix); ok { | ||
| // Single-line value: extract the runner label and check against known standard prefixes. | ||
| value = strings.TrimSpace(value) | ||
| lower := strings.ToLower(value) | ||
| // Standard GitHub-hosted runners that ship with Node.js pre-installed. | ||
| return !strings.HasPrefix(lower, "ubuntu-") && !strings.HasPrefix(lower, "windows-") | ||
| } |
There was a problem hiding this comment.
isCustomImageRunner treats any label starting with ubuntu- as a standard GitHub-hosted runner. This misclassifies repo-specific/custom runner labels like ubuntu-slim (used as constants.DefaultActivationJobRunnerImage) and any self-hosted runner label that happens to start with ubuntu-, causing Node setup to be skipped when it may be required. Consider using an allowlist of known GitHub-hosted labels (e.g., ubuntu-latest, ubuntu-24.04, ubuntu-22.04, ubuntu-20.04, plus any supported *-arm variants) and treat everything else (including ubuntu-slim) as custom.
| name: "ubuntu-slim is standard", | ||
| runsOn: "runs-on: ubuntu-slim", | ||
| expected: false, |
There was a problem hiding this comment.
This test encodes ubuntu-slim as a “standard” runner (expected false for custom). In this repo, ubuntu-slim is not a GitHub-hosted label and is used as the default framework runner (constants.DefaultActivationJobRunnerImage), so it should be treated as a custom label by isCustomImageRunner if the goal is to detect non-GitHub-hosted runners.
| name: "ubuntu-slim is standard", | |
| runsOn: "runs-on: ubuntu-slim", | |
| expected: false, | |
| name: "ubuntu-slim is custom", | |
| runsOn: "runs-on: ubuntu-slim", | |
| expected: true, |
pkg/workflow/runtime_detection.go
Outdated
| // When using a custom image runner, ensure Node.js v24 is set up. | ||
| // Standard GitHub-hosted runners (ubuntu-*, windows-*) have Node.js pre-installed, | ||
| // but custom image runners (self-hosted, enterprise runners, non-standard labels) may not. | ||
| // Node.js is required for gh-aw scripts such as start_safe_outputs_server.sh and | ||
| // start_mcp_scripts_server.sh that invoke `node` directly. | ||
| if isCustomImageRunner(workflowData.RunsOn) { | ||
| runtimeSetupLog.Printf("Custom image runner detected (%q), ensuring Node.js v24 is set up", workflowData.RunsOn) |
There was a problem hiding this comment.
The log message hardcodes “Node.js v24”, but this code adds the node runtime with an empty version (default) and the version may later be overridden by frontmatter (runtimes.node.version). Consider logging a version-agnostic message (or log the resolved version after overrides) to avoid misleading diagnostics.
| // When using a custom image runner, ensure Node.js v24 is set up. | |
| // Standard GitHub-hosted runners (ubuntu-*, windows-*) have Node.js pre-installed, | |
| // but custom image runners (self-hosted, enterprise runners, non-standard labels) may not. | |
| // Node.js is required for gh-aw scripts such as start_safe_outputs_server.sh and | |
| // start_mcp_scripts_server.sh that invoke `node` directly. | |
| if isCustomImageRunner(workflowData.RunsOn) { | |
| runtimeSetupLog.Printf("Custom image runner detected (%q), ensuring Node.js v24 is set up", workflowData.RunsOn) | |
| // When using a custom image runner, ensure Node.js is set up. | |
| // Standard GitHub-hosted runners (ubuntu-*, windows-*) have Node.js pre-installed, | |
| // but custom image runners (self-hosted, enterprise runners, non-standard labels) may not. | |
| // Node.js is required for gh-aw scripts such as start_safe_outputs_server.sh and | |
| // start_mcp_scripts_server.sh that invoke `node` directly. | |
| if isCustomImageRunner(workflowData.RunsOn) { | |
| runtimeSetupLog.Printf("Custom image runner detected (%q), ensuring Node.js is set up", workflowData.RunsOn) |
| { | ||
| name: "custom runner with node version override uses overridden version", | ||
| workflowMarkdown: `--- | ||
| on: push | ||
| engine: copilot | ||
| runs-on: self-hosted | ||
| runtimes: | ||
| node: | ||
| version: '22' | ||
| --- | ||
|
|
||
| # Test workflow`, | ||
| expectNodeSetup: true, | ||
| }, |
There was a problem hiding this comment.
The test case named “custom runner with node version override uses overridden version” only asserts that a setup-node step exists, not that it uses the overridden node-version: '22'. Adding an assertion for the version in the agent job section would cover the override interaction introduced by this change.
…nner when custom tokens needed Agent-Logs-Url: https://github.com/github/gh-aw/sessions/ce07a588-4938-4c89-b085-78d7a224a99a Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Done in commit Warning Firewall rules blocked me from connecting to one or more addresses (expand for details)I tried to connect to the following addresses, but was blocked by firewall rules:
If you need me to access, download, or install something from one of these locations, you can either:
|
|
@copilot Review comments |
…im is custom, version-agnostic log Agent-Logs-Url: https://github.com/github/gh-aw/sessions/f5ab582e-5ab4-429c-b062-78243b07802a Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Addressed all 4 review points in commit
The Warning Firewall rules blocked me from connecting to one or more addresses (expand for details)I tried to connect to the following addresses, but was blocked by firewall rules:
If you need me to access, download, or install something from one of these locations, you can either:
|
|
@copilot ubuntu-slim is a supported runner with builtin node. |
…(has built-in Node.js) Agent-Logs-Url: https://github.com/github/gh-aw/sessions/7e7deaba-2564-443b-9f51-831b9215b378 Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Fixed in commit Warning Firewall rules blocked me from connecting to one or more addresses (expand for details)I tried to connect to the following addresses, but was blocked by firewall rules:
If you need me to access, download, or install something from one of these locations, you can either:
|
Summary
When using a non-standard GitHub-hosted runner (self-hosted, custom enterprise runners, GPU runners like
aw-gpu-runner-T4), Node.js may not be pre-installed. This PR updates the compiler to automatically add a Node.js setup step in two places:actions/setupwhen custom tokens are needed (@actions/githubis installed bysetup.shvianpm install, so Node.js must be on PATH)Changes
pkg/workflow/runtime_detection.goisCustomImageRunner(runsOn string) boolhelper function that identifies non-standard runners using an explicit allowlist of known runner labels that have Node.js pre-installed. The allowlist includes official GitHub-hosted labels (e.g.ubuntu-latest,ubuntu-22.04,ubuntu-24.04, ARM variants, Windows variants) as well asubuntu-slim(gh-aw's default framework runner, which also has Node.js pre-installed). Everything else (self-hosted, enterprise labels, GPU runner labels) is treated as custom:false— Node.js is pre-installedtrue— Node.js may not be presentDetectRuntimeRequirementsto inject anoderuntime requirement when a custom image runner is detected, ensuringactions/setup-node@v6is emitted in the agent job steps (version respects anyruntimes.node.versionfrontmatter override)pkg/workflow/compiler_safe_outputs_job.goenableCustomTokensistrue(setup action installs@actions/githubvianpm install) and the safe_outputs job uses a custom image runner, aSetup Node.jsstep is now emitted before theSetup Scripts(actions/setup) steppkg/workflow/runtime_setup_test.goisCustomImageRunnercovering all runner forms, includingubuntu-slimas standardpkg/workflow/runtime_setup_integration_test.gonode-versionvalue appears in the compiled outputpkg/workflow/safe_outputs_runs_on_test.goactions/setupin the safe_outputs job when custom tokens are needed and a custom image runner is configured, and is absent when either condition is not metRecompiled lock files
.github/workflows/daily-fact.lock.yml/daily-news.lock.yml— useruns-on: aw-gpu-runner-T4(custom GPU runner), now include a Node.js setup step in their agent jobsWhy this approach
The runtime manager (
DetectRuntimeRequirements→GenerateRuntimeSetupSteps) is the correct place for the agent job logic because it:runtimes.node.versionin frontmatteractions/setup-nodesteps in custom stepsFor the safe_outputs job, the Node.js setup step is emitted directly before
actions/setupsince the runtime manager does not control the safe_outputs job step order.