Add GH_AW_RUNTIME_FEATURES support to built-in workflow jobs#40824
Conversation
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
|
✅ PR Code Quality Reviewer completed the code quality review. |
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. |
|
✅ Test Quality Sentinel completed test quality analysis. |
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅ |
There was a problem hiding this comment.
Pull request overview
This PR adds propagation of a runtime feature flag bundle via GH_AW_RUNTIME_FEATURES into framework-managed (built-in) jobs, plus shared JS helpers for parsing/querying the runtime feature set and a step-summary log in the activation job. It updates the workflow golden fixtures to reflect the new generated YAML.
Changes:
- Inject
GH_AW_RUNTIME_FEATURES: ${{ vars.GH_AW_RUNTIME_FEATURES }}into job-levelenvfor built-in jobs during render. - Add an activation job step that logs the runtime features value into
$GITHUB_STEP_SUMMARY. - Add JS parsing helpers (
parseRuntimeFeatures,hasRuntimeFeature,getRuntimeFeatureValue) and expose them viasetupGlobals, with Vitest coverage.
Show a summary per file
| File | Description |
|---|---|
| pkg/workflow/jobs.go | Injects GH_AW_RUNTIME_FEATURES into rendered env for a defined set of built-in jobs. |
| pkg/workflow/runtime_features_env_test.go | Adds Go unit tests for the built-in job env injection and activation step presence. |
| pkg/workflow/compiler_activation_job.go | Wires the runtime-features summary step into activation job construction. |
| pkg/workflow/compiler_activation_job_builder.go | Implements the activation “Log runtime features” step YAML. |
| actions/setup/js/runtime_features.cjs | Adds runtime feature parsing and lookup helpers for newline-delimited inputs. |
| actions/setup/js/runtime_features.test.cjs | Adds Vitest coverage for the runtime feature parser/helpers. |
| actions/setup/js/setup_globals.cjs | Exposes parsed runtime features + helper accessors on global.* for runtime scripts. |
| pkg/workflow/testdata/TestWasmGolden_CompileFixtures/with-imports.golden | Golden update to reflect built-in env injection + activation summary step. |
| pkg/workflow/testdata/TestWasmGolden_CompileFixtures/smoke-copilot.golden | Golden update to reflect built-in env injection + activation summary step. |
| pkg/workflow/testdata/TestWasmGolden_CompileFixtures/playwright-cli-mode.golden | Golden update to reflect built-in env injection + activation summary step. |
| pkg/workflow/testdata/TestWasmGolden_CompileFixtures/basic-copilot.golden | Golden update to reflect built-in env injection + activation summary step. |
| pkg/workflow/testdata/TestWasmGolden_AllEngines/pi.golden | Golden update to reflect built-in env injection + activation summary step. |
| pkg/workflow/testdata/TestWasmGolden_AllEngines/gemini.golden | Golden update to reflect built-in env injection + activation summary step. |
| pkg/workflow/testdata/TestWasmGolden_AllEngines/copilot.golden | Golden update to reflect built-in env injection + activation summary step. |
| pkg/workflow/testdata/TestWasmGolden_AllEngines/codex.golden | Golden update to reflect built-in env injection + activation summary step. |
| pkg/workflow/testdata/TestWasmGolden_AllEngines/claude.golden | Golden update to reflect built-in env injection + activation summary step. |
Copilot's findings
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 16/16 changed files
- Comments generated: 2
| func buildRuntimeFeaturesSummaryStep() []string { | ||
| return []string{ | ||
| " - name: Log runtime features\n", | ||
| " run: |\n", | ||
| " {\n", | ||
| " echo \"## Runtime features\"\n", | ||
| " echo\n", | ||
| " if [[ -n \"${GH_AW_RUNTIME_FEATURES:-}\" ]]; then\n", | ||
| " echo '```text'\n", | ||
| " printf '%s\\n' \"$GH_AW_RUNTIME_FEATURES\"\n", | ||
| " echo '```'\n", | ||
| " else\n", | ||
| " echo \"_Not set_\"\n", | ||
| " fi\n", | ||
| " } >> \"$GITHUB_STEP_SUMMARY\"\n", | ||
| } | ||
| } |
There was a problem hiding this comment.
Fixed in 470f5bd. The activation summary step now uses a separate GH_AW_RUNTIME_FEATURES_IS_SET flag so it can render _Not set_ for an absent repo variable and _Empty string_ when the variable exists but is empty.
| import { describe, expect, it } from "vitest"; | ||
|
|
||
| const { parseRuntimeFeatures, hasRuntimeFeature, getRuntimeFeatureValue } = require("./runtime_features.cjs"); | ||
|
|
There was a problem hiding this comment.
Fixed in 470f5bd. runtime_features.test.cjs now uses createRequire(import.meta.url) and keeps the .cjs import path aligned with the repo’s Vitest ESM pattern.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
🏗️ Design Decision Gate — ADR RequiredThis PR makes significant changes to core business logic (>100 new lines in 📄 Draft ADR committed: 📋 What to do next
Once an ADR is linked in the PR body, this gate will re-run and verify the implementation matches the decision. ❓ Why ADRs Matter
ADRs create a searchable, permanent record of why the codebase looks the way it does. Future contributors (and your future self) will thank you. 📋 Michael Nygard ADR Format ReferenceAn ADR must contain these four sections to be considered complete:
All ADRs are stored in
|
🧪 Test Quality Sentinel Report✅ Test Quality Score: 91/100 — Excellent
📊 Metrics & Test Classification (7 tests analyzed)
Go: 4 ( Score breakdown: Behavioral Coverage 40/40 + Edge/Error Coverage 21/30 + Low Duplication 20/20 + Proportional Growth 10/10 = 91/100 Inflation ratios: JS 35/66 = 0.53 ✅ · Go 71/66 = 1.08 ✅ 💡 Minor Coverage Gaps (non-blocking)These do not affect the score or verdict but are worth noting:
Verdict
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /tdd, /grill-with-docs, and /zoom-out — commenting with test coverage gaps and a naming-consistency observation. No blocking issues found.
📋 Key Themes & Highlights
Key Themes
- Allowlist maintenance: Three job names in
runtimeFeaturesBuiltInJobNamesuse raw string literals rather thanconstants.*JobNamevalues, creating a silent-drift risk on future rename. - Test coverage gaps: The
Uses-job exclusion guard,null/undefinedparser input, and multi-=value parsing are all implemented correctly but lack explicit tests. - Global re-parse on repeat calls:
setupGlobals()overwritesglobal.runtimeFeatureson every invocation — harmless in production but worth documenting for test scaffolding.
Positive Highlights
- ✅
buildRenderedJobEnvusesmaps.Clone()— no aliasing bugs possible on the returned env map. - ✅ The
!existsguard inbuildRenderedJobEnvcorrectly preserves caller-suppliedGH_AW_RUNTIME_FEATURESoverrides, and that behaviour is tested. - ✅ Parser handles CRLF (
\r?\n), whitespace-trimmed keys and values, and malformed entries — robust for real-world GitHub vars input. - ✅ Nine golden fixture files updated identically — a strong regression guard across all engine combinations.
- ✅ Go and JS tests together cover the primary happy paths and key edge cases well.
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · 88.5 AIC · ⌖ 10.4 AIC · ⊞ 6.5K
| string(constants.UnlockJobName): {}, | ||
| "push_experiments_state": {}, | ||
| "push_repo_memory": {}, | ||
| "update_cache_memory": {}, |
There was a problem hiding this comment.
[/grill-with-docs] Three entries in runtimeFeaturesBuiltInJobNames use raw string literals instead of constants.*JobName values, so a job rename will silently stop injecting GH_AW_RUNTIME_FEATURES for that job with no compile-time signal.
💡 Suggestion
"push_experiments_state", "push_repo_memory", and "update_cache_memory" are the only allowlist members not backed by a typed constant. Consider either adding constants.PushExperimentsStateJobName etc. (consistent with the nine entries above them), or adding a comment noting why they lack constants so a future refactor knows to revisit the allowlist.
There was a problem hiding this comment.
Fixed in 470f5bd. I introduced shared job-name constants and now use them in both the runtime-features allowlist and the corresponding job builders.
| if len(env) != 0 { | ||
| t.Fatalf("expected no env vars for custom job, got %v", env) | ||
| } | ||
| } |
There was a problem hiding this comment.
[/tdd] The shouldInjectRuntimeFeaturesEnv guard that skips uses:-based reusable workflow jobs has no test — it is the only branch of that function not covered here.
💡 Suggested test
func TestBuildRenderedJobEnv_DoesNotAddRuntimeFeaturesForUsesJobs(t *testing.T) {
job := &Job{
Name: string(constants.AgentJobName),
Uses: "some/repo/.github/workflows/action.yml@main",
}
env := buildRenderedJobEnv(job)
if _, ok := env[runtimeFeaturesEnvVarName]; ok {
t.Fatal("expected no runtime features env for uses: job")
}
}This pins the job.Uses != "" check against accidental removal.
There was a problem hiding this comment.
Fixed in 470f5bd. Added a focused test covering the Uses != "" reusable-workflow skip path in buildRenderedJobEnv.
| expect(getRuntimeFeatureValue(features, "mode")).toBe("fast"); | ||
| expect(getRuntimeFeatureValue(features, "missing")).toBeUndefined(); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
[/tdd] The JSDoc declares @param {string | undefined | null} but there are no tests for null or undefined input — the if (!raw) return {} branch is unreachable from the three existing tests.
💡 Suggested tests
it("returns empty map for null", () => {
expect(parseRuntimeFeatures(null)).toEqual({});
});
it("returns empty map for undefined", () => {
expect(parseRuntimeFeatures(undefined)).toEqual({});
});These lock in the contract described by the annotation and prevent a future refactor from accidentally dropping the guard.
There was a problem hiding this comment.
Fixed in 470f5bd. Added nullish-input coverage so parseRuntimeFeatures(null) and parseRuntimeFeatures(undefined) are both pinned to {}.
|
|
||
| describe("runtime_features", () => { | ||
| it("parses newline-delimited flags and key value pairs", () => { | ||
| const features = parseRuntimeFeatures("key\nkey2=value\nkey3 = spaced value"); |
There was a problem hiding this comment.
[/tdd] indexOf("=") correctly splits on the first =, so "key=a=b=c" yields { key: "a=b=c" } — but this is not tested. Feature values containing = (base64 payloads, URLs, etc.) are a realistic input.
💡 Suggested test
it("treats only the first = as the key/value separator", () => {
expect(parseRuntimeFeatures("key=a=b=c")).toEqual({ key: "a=b=c" });
});There was a problem hiding this comment.
Fixed in 470f5bd. Added coverage for values containing additional = characters so only the first separator is treated as structural.
| */ | ||
| function setupGlobals(coreModule, githubModule, contextModule, execModule, ioModule, getOctokitFn) { | ||
| global.core = coreModule; | ||
| const runtimeFeatures = parseRuntimeFeatures(process.env.GH_AW_RUNTIME_FEATURES); |
There was a problem hiding this comment.
[/zoom-out] setupGlobals() unconditionally overwrites global.runtimeFeatures on every call — in test scaffolding that calls it multiple times to swap core/github mocks, features are re-derived from the process environment rather than from any test-controlled value.
💡 Options
In production this is called once, so there is no live bug. But for test isolation you could either:
- Parse only on first call:
if (!global.runtimeFeatures) {
const rf = parseRuntimeFeatures(process.env.GH_AW_RUNTIME_FEATURES);
global.runtimeFeatures = rf;
global.hasRuntimeFeature = key => hasRuntimeFeature(rf, key);
global.getRuntimeFeatureValue = key => getRuntimeFeatureValue(rf, key);
}- Or add a JSDoc/comment noting that
setupGlobalsis call-once so test helpers know to resetprocess.env.GH_AW_RUNTIME_FEATURESbefore each call.
There was a problem hiding this comment.
No code change for the re-parse behavior in 470f5bd. I kept setupGlobals() re-evaluating process.env.GH_AW_RUNTIME_FEATURES on each call so repeated test setup can intentionally pick up the current environment, and paired that with an immutable Object.freeze(...) runtime-features map.
There was a problem hiding this comment.
Overall: Non-blocking, 3 findings worth addressing
The core wiring is sound — buildRenderedJobEnv correctly clones before mutating, the inject-only-once guard prevents clobbering explicit overrides, and the activation-job summary step is a clean observability addition. Three concerns worth addressing before this gets heavy usage:
📋 Finding summary
-
pkg/workflow/jobs.golines 29–31 — Three job names (push_experiments_state,push_repo_memory,update_cache_memory) are raw string literals with no compile-time link to their builderName:assignments. A silent rename in any builder file would drop the env injection for that job with no failing test or compiler error. -
actions/setup/js/runtime_features.cjsline 38 — Akey=entry (empty value) produces""(falsy), buthasRuntimeFeaturestill returnstrue. Callers who checkif (getRuntimeFeatureValue(...))for presence will get a silent false negative. Consider normalising empty values totrue, or documenting the contract explicitly in JSDoc. -
pkg/workflow/runtime_features_env_test.go— TheUses != ""guard inshouldInjectRuntimeFeaturesEnvhas no test. A regression there would silently inject env into reusable workflow calls.
Note: grumpy-coder sub-agent timed out; this review is based on direct analysis.
🔎 Code quality review by PR Code Quality Reviewer · 167 AIC · ⌖ 7.79 AIC · ⊞ 5.1K
| string(constants.UnlockJobName): {}, | ||
| "push_experiments_state": {}, | ||
| "push_repo_memory": {}, | ||
| "update_cache_memory": {}, |
There was a problem hiding this comment.
Three job names are raw string literals with no compile-time safety: if any of these names change in their builder files, the runtime-features injection silently breaks.
💡 Details and suggested fix
"push_experiments_state", "push_repo_memory", and "update_cache_memory" are hardcoded here as raw string literals, while the same names are independently set as raw strings in their builder files:
compiler_experiments.go:664—Name: "push_experiments_state"repo_memory.go:817—Name: "push_repo_memory"cache.go:1067—Name: "update_cache_memory"
There is no compile-time link between this allowlist entry and the job's actual Name field. A silent rename in any builder file would leave a stale entry here and silently drop the env injection for that job — no error, no warning, no test failure (since the golden fixtures don't cover those optional jobs).
Suggested fix: introduce package-level constants mirroring the constants.XxxJobName pattern:
// In an appropriate constants or jobs file:
const (
pushExperimentsStateJobName = "push_experiments_state"
pushRepoMemoryJobName = "push_repo_memory"
updateCacheMemoryJobName = "update_cache_memory"
)Then use those constants in both the allowlist here and the Name: assignments in the builder files.
There was a problem hiding this comment.
Fixed in 470f5bd by introducing shared constants for those three built-in job names and using them from both the allowlist and the builder Name: assignments.
| if (!key) { | ||
| continue; | ||
| } | ||
| features[key] = trimmed.slice(equalsIndex + 1).trim(); |
There was a problem hiding this comment.
key= entries produce a falsy empty string "", but hasRuntimeFeature still returns true for them — the two helpers give contradictory truthiness signals for the same feature.
💡 Impact and suggested fix
A line like myfeature= parses to features["myfeature"] = "". This means:
hasRuntimeFeature(features, "myfeature") // → true (feature IS present)
getRuntimeFeatureValue(features, "myfeature") // → "" (falsy!)Any consumer script that guards on truthiness of the value:
if (getRuntimeFeatureValue(runtimeFeatures, "myfeature")) {
// never reached for "myfeature=" — silent false negative
}will silently skip the feature even though it was explicitly set. This is especially easy to hit since hasRuntimeFeature is the advertised "is it set?" API, but callers may reasonably collapse both checks into one.
Option A — Normalize empty values to true (treat key= same as key):
const raw_value = trimmed.slice(equalsIndex + 1).trim();
features[key] = raw_value === "" ? true : raw_value;Option B — Keep the distinction but document it explicitly in the JSDoc and the hasRuntimeFeature JSDoc so consumers know to use hasRuntimeFeature for presence checks and never rely on truthiness of getRuntimeFeatureValue.
There was a problem hiding this comment.
Addressed in 470f5bd by documenting and testing the empty-string contract instead of changing parsing semantics. key= still preserves "", and callers should use hasRuntimeFeature(...) for presence checks when they need to distinguish empty from missing.
| job := &Job{ | ||
| Name: string(constants.AgentJobName), | ||
| Env: map[string]string{ | ||
| runtimeFeaturesEnvVarName: `"explicit"`, |
There was a problem hiding this comment.
No test covers the reusable-workflow skip path in shouldInjectRuntimeFeaturesEnv: a job with Uses != "" must not receive the env injection, but this is currently untested.
💡 Suggested test
func TestBuildRenderedJobEnv_SkipsReusableWorkflowJobs(t *testing.T) {
// Even a built-in job name should be skipped when Uses is set
job := &Job{
Name: string(constants.AgentJobName),
Uses: "./.github/workflows/reusable.yml",
}
env := buildRenderedJobEnv(job)
if _, ok := env[runtimeFeaturesEnvVarName]; ok {
t.Fatalf("expected reusable workflow job to not receive runtime features env, got %v", env)
}
}Without this test, the guard job.Uses != "" in shouldInjectRuntimeFeaturesEnv could be removed or broken with no immediate test failure.
There was a problem hiding this comment.
Fixed in 470f5bd. Added a reusable-workflow skip test so a built-in job name with Uses set does not receive the runtime-features env injection.
| function setupGlobals(coreModule, githubModule, contextModule, execModule, ioModule, getOctokitFn) { | ||
| global.core = coreModule; | ||
| const runtimeFeatures = parseRuntimeFeatures(process.env.GH_AW_RUNTIME_FEATURES); | ||
| global.runtimeFeatures = runtimeFeatures; |
There was a problem hiding this comment.
global.runtimeFeatures is a mutable object: any action script in the same Node.js process can modify it and corrupt feature state for all subsequent callers.
💡 Suggested fix
All runtime scripts run in the same long-lived Node.js process. Because global.runtimeFeatures is a plain object, any script can do:
global.runtimeFeatures['someFlag'] = true; // silently corrupts feature stateThis would affect every subsequent call to hasRuntimeFeature / getRuntimeFeatureValue in that process, including calls made by unrelated scripts.
Freeze the object after assignment:
global.runtimeFeatures = Object.freeze(parseRuntimeFeatures(process.env.GH_AW_RUNTIME_FEATURES));Note: Object.freeze is shallow, but since all values here are primitives (boolean | string), it fully protects the feature map.
There was a problem hiding this comment.
Fixed in 470f5bd. setupGlobals() now freezes the parsed runtime-features object before exposing it on global.runtimeFeatures.
|
@copilot run pr-finisher skill |
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
This adds runtime feature flag propagation via
GH_AW_RUNTIME_FEATURESusing GitHub Actions vars, with support for newline-delimited flags andkey=valueentries. It also makes the activation job surface the effective value in the step summary for quick runtime inspection.Built-in job env wiring
GH_AW_RUNTIME_FEATURES: ${{ vars.GH_AW_RUNTIME_FEATURES }}into built-in job-levelenvblocks.activation,agent,detection,safe_outputs,conclusion, and related built-in follow-on jobs.Activation visibility
$GITHUB_STEP_SUMMARY.Runtime parsing helpers
keykey2=valuesetup_globalsso runtime scripts can query feature presence and values without duplicating parsing logic.Golden output updates
Example shape of the propagated variable:
Example runtime value: