Rewrite experiments.<name> in engine.model to valid job-scoped expressions - #61599
Conversation
…sions Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
…ed experiment names Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
…prefix collisions Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
experiments.<name> in engine.model to valid job-scoped expressions
|
✅ Ponytail Reviewer completed successfully! Warning Firewall blocked 1 domainThe following domain was blocked by the firewall during workflow execution:
To allow these domains, add them to the network:
allowed:
- defaults
- "ab.chatgpt.com"See Network Configuration for more information.
|
|
Warning Firewall blocked 2 domainsThe following domains were blocked by the firewall during workflow execution:
To allow these domains, add them to the network:
allowed:
- defaults
- "github.com"
- "proxy.golang.org"See Network Configuration for more information.
|
|
Warning Threat Detection Engine Failure — The analysis engine could not complete. This is a tooling failure, not a security finding. What happenedThe threat detection engine failed to produce results. Review the workflow run logs for details.
|
|
🧠 Matt Pocock Skills Reviewer failed during the skills-based review.
|
|
✅ Test Quality Sentinel completed test quality analysis. Test Quality Sentinel skipped because pre-fetch PR data was unavailable: unable to fetch test file diff
|
There was a problem hiding this comment.
Ponytail pass focused only on simplification opportunities.
net: -6 lines possible.
Warning
Firewall blocked 1 domain
The following domain was blocked by the firewall during workflow execution:
ab.chatgpt.com
To allow these domains, add them to the network.allowed list in your workflow frontmatter:
network:
allowed:
- defaults
- "ab.chatgpt.com"See Network Configuration for more information.
Generated by ✂️ Ponytail Reviewer for #61599 · codex · gpt53codex · 5.44 AIC · ⌖ 3.28 AIC · ⊞ 12.5K
Comment /ponytail to run again
Comments that could not be inline-anchored
pkg/workflow/compiler_experiments.go:69
L69: yagni: separate activation-specific rewrite helper adds API surface for one call site. Inline rewriteDeclaredExperimentNames(..., pickExperimentOutputsPrefix) at use site.
pkg/workflow/compiler_yaml_step_lifecycle.go:179
L179: shrink: temp infoModel variable only feeds one Fprintf. Call RewriteActivationOutputsToLocalStepOutputs(data.Model, data.Experiments) inline in the formatter call.
There was a problem hiding this comment.
🟡 Changes recommended
The activation info step reads experiment outputs before selection occurs, and the regex can corrupt unrelated expression text.
Get a fresh assessment by requesting another Copilot review.
Pull request overview
Fixes experiment-based model references when compiling GitHub Actions workflows.
Changes:
- Adds scoped experiment-reference rewrite helpers.
- Applies rewrites to model environment variables and activation metadata.
- Adds unit and compilation regression tests.
File summaries
| File | Description |
|---|---|
pkg/workflow/compiler_experiments.go |
Adds reference-rewriting helpers. |
pkg/workflow/compiler_orchestrator_workflow.go |
Rewrites configured models after experiment extraction. |
pkg/workflow/compiler_yaml_step_lifecycle.go |
Adapts model references for activation metadata. |
pkg/workflow/compiler_experiments_test.go |
Tests rewrite behavior and name collisions. |
pkg/workflow/engine_config_test.go |
Tests compiled Claude model expressions. |
Review details
- Files reviewed: 5/5 changed files
- Comments generated: 2
- Review effort level: Balanced
💡 Add a code-review agent skill for context-aware, tailored reviews. Learn more in the docs.
| // data.Model may reference an experiment variant as needs.activation.outputs.<name> | ||
| // (rewritten from engine.model: ${{ experiments.<name> }} for jobs downstream of | ||
| // activation). This step runs inside the activation job itself, so it must read the | ||
| // pick-experiment step's output directly rather than via the needs context. | ||
| infoModel := RewriteActivationOutputsToLocalStepOutputs(data.Model, data.Experiments) |
There was a problem hiding this comment.
Fixed in b6eda7a: experiment selection steps are now emitted right after the setup steps (addActivationExperimentSteps in newActivationJobBuildContext), so pick-experiment runs before generate_aw_info and GH_AW_INFO_MODEL / the activation model output resolve to the selected variant. Job outputs for experiment names are still registered after the engine outputs so a declared name keeps precedence. Added an ordering assertion in TestCompileClaudeWithExperimentsModel.
| // experimentsFieldReferenceRegex matches `experiments.<name>` tokens (simple identifier) | ||
| // appearing anywhere inside a raw configuration string, such as an `engine.model` value. | ||
| // Unlike experimentNameRegex/experimentComparisonRegex in expression_extraction.go, this | ||
| // pattern is not anchored, so it can rewrite the reference wherever it appears inside a | ||
| // larger ${{ ... }} expression (e.g. "${{ experiments.model }}"). | ||
| var experimentsFieldReferenceRegex = regexp.MustCompile(`\bexperiments\.([a-zA-Z_][a-zA-Z0-9_]*)\b`) |
There was a problem hiding this comment.
Fixed in b6eda7a: rewriteDeclaredExperimentNames now only scans the bodies of ${{ ... }} expressions, skips single-quoted string literals (including doubled-quote escapes), and ignores matches adjacent to a .. So ${{ format('experiments.model-{0}', inputs.suffix) }} and ${{ fromJSON(inputs.config).experiments.model }} are left untouched, while ${{ experiments.model || 'sonnet' }} still rewrites. Covered by TestRewriteExperimentsReferenceOnlyRewritesExpressionTokens.
🏗️ ADR Required — draft added for PR #61599I enforced the design-decision gate for this PR because it adds 228 new lines in business-logic directories ( Evidence used
Gate resultNo existing ADR covering this decision was found in the PR body or on the branch, so I added a draft ADR:
Next actionPlease review and refine the draft ADR so the architectural decision and trade-offs are explicit before merge. Warning Firewall blocked 2 domainsThe following domains were blocked by the firewall during workflow execution:
To allow these domains, add them to the network:
allowed:
- defaults
- "github.com"
- "proxy.golang.org"See Network Configuration for more information.
|
There was a problem hiding this comment.
Reviewed the experiments-model rewrite change (refactor/cleanup on gh-aw compiler internals). The logic looks correct and well-tested: RewriteExperimentsReferenceForDownstreamJobs is applied to workflowData.Model right after experiments are extracted, and RewriteActivationOutputsToLocalStepOutputs correctly converts it back for the activation job's own info step, since a job can't reference its own outputs via needs. Word-boundary anchoring in the regexes correctly avoids the model vs model_variant prefix-collision case, which is covered by a dedicated test. Compile-time model validation (validateModelAliasMap, warnCodexCopilotModelCompatibility) runs before the rewrite and already skips values containing ${{, so no regression there.
One non-blocking cleanup item left as an inline comment: RewriteExperimentsReferenceForActivationJob is new, exported, and tested, but has no production caller — the activation job's info step actually goes through the downstream-rewrite-then-local-conversion path instead. Worth removing or wiring in to avoid dead public API surface.
🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · copilot · sonnet50 · 91.6 AIC · ⌖ 13.8 AIC · ⊞ 8.6K
| // info" step), where the experiment variant was just selected by the earlier pick-experiment | ||
| // step in the same job. A job cannot reference its own outputs via the `needs` context, so | ||
| // `needs.activation.outputs.*` is not valid here. | ||
| func RewriteExperimentsReferenceForActivationJob(s string, experiments map[string][]string) string { |
There was a problem hiding this comment.
RewriteExperimentsReferenceForActivationJob is exported and covered by TestRewriteExperimentsReferenceForActivationJob, but it has no production caller anywhere in the codebase — the activation job's own info step actually goes through RewriteExperimentsReferenceForDownstreamJobs (in extractAdditionalConfigurations) followed by RewriteActivationOutputsToLocalStepOutputs (in generateCreateAwInfo). If this helper isn't meant to be used directly elsewhere, consider removing it (and its test) to avoid dead public API surface, or wire it in where it was originally intended so the two forward/backward-conversion paths aren't duplicated logic. @copilot please address this.
There was a problem hiding this comment.
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · copilot · sonnet50 · 220.2 AIC · ⌖ 16.4 AIC · ⊞ 10.6K
Comment /matt to run again
| // info" step), where the experiment variant was just selected by the earlier pick-experiment | ||
| // step in the same job. A job cannot reference its own outputs via the `needs` context, so | ||
| // `needs.activation.outputs.*` is not valid here. | ||
| func RewriteExperimentsReferenceForActivationJob(s string, experiments map[string][]string) string { |
There was a problem hiding this comment.
[/codebase-design] RewriteExperimentsReferenceForActivationJob is exported but never called from production code — only RewriteExperimentsReferenceForDownstreamJobs (in compiler_orchestrator_workflow.go) and RewriteActivationOutputsToLocalStepOutputs (in compiler_yaml_step_lifecycle.go) are wired up.
💡 Why this matters
The activation job path is instead handled by rewriting downstream-form (needs.activation.outputs.*) back to step-local form via RewriteActivationOutputsToLocalStepOutputs, which works because workflowData.Model is already rewritten to downstream form earlier in extractAdditionalConfigurations. That leaves RewriteExperimentsReferenceForActivationJob as dead code with no callers outside its own unit test — it adds public API surface and a maintenance burden without being exercised by the actual compile path.
Consider either removing this function (and its dedicated test) if RewriteActivationOutputsToLocalStepOutputs fully covers the activation-job case, or documenting why both entry points need to exist (e.g. a future direct-experiments-to-activation-job path that does not go through the downstream rewrite first).
@copilot please address this.
There was a problem hiding this comment.
Removed RewriteExperimentsReferenceForActivationJob (and its test) in b6eda7a — it had no production caller; the activation-job path goes through RewriteActivationOutputsToLocalStepOutputs.
|
@copilot PR sous-chef triage: there are unresolved review threads to address before a maintainer can investigate quickly.
Warning Firewall blocked 1 domainThe following domain was blocked by the firewall during workflow execution:
To allow these domains, add them to the network:
allowed:
- defaults
- "github.com"See Network Configuration for more information.
|
…okens Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
|
@copilot PR sous-chef triage: maintainer follow-through is still blocked by one unresolved review thread.
Generated by PR Sous Chef run: https://github.com/github/gh-aw/actions/runs/35239680233
|
|
@copilot Please move this PR forward. Open review threads (newest first):
Please address the feedback, refresh the branch if needed, rerun any affected checks, and then continue with the Run: https://github.com/github/gh-aw/actions/runs/35240793612 Warning Firewall blocked 1 domainThe following domain was blocked by the firewall during workflow execution:
To allow these domains, add them to the network:
allowed:
- defaults
- "github.com"See Network Configuration for more information.
|
|
🎉 This pull request is included in a new release. Release: |
${{ experiments.model }}used inengine.model(or the split top-levelmodel:field) was rewritten for prompt-body text but not forANTHROPIC_MODEL,GH_AW_INFO_MODEL, orGH_AW_ENGINE_MODEL. The compiled lock file kept the literalexperiments.modelreference, which GitHub Actions rejects at run time sinceexperimentsis not a valid context:Fix
pkg/workflow/compiler_experiments.go) that convertexperiments.<name>into a valid, job-scoped reference:RewriteExperimentsReferenceForDownstreamJobs→needs.activation.outputs.<name>, for jobs downstream of activation (agent, detection, conclusion, safe-outputs).RewriteExperimentsReferenceForActivationJob/RewriteActivationOutputsToLocalStepOutputs→steps.pick-experiment.outputs.<name>, for the activation job's own info step (a job cannot reference its own outputs vianeeds).RewriteExperimentsReferenceForDownstreamJobstoworkflowData.Modelright after experiments are extracted from frontmatter, so every consumer ofdata.Model(Claude/Codex/Copilot/Gemini/Pi engines' native model env vars,GH_AW_ENGINE_MODEL, etc.) picks up the corrected value automatically.RewriteActivationOutputsToLocalStepOutputsingenerateCreateAwInfosoGH_AW_INFO_MODEL(emitted inside the activation job itself) uses the step-local form instead.modelvsmodel_variant) can't corrupt an unrelated occurrence, and unrelated text is never rewritten.Result
Tests
experiments.modelnever contains a rawexperiments.modeltoken, for both the nestedengine.modeland split top-levelmodel:forms.Run: https://github.com/github/gh-aw/actions/runs/35240793612
Warning
Firewall blocked 1 domain
The following domain was blocked by the firewall during workflow execution:
github.laiyagushi.comTo allow these domains, add them to the
network.allowedlist in your workflow frontmatter:See Network Configuration for more information.