feat: allow GitHub Actions expressions for list-valued safe-output constraints#29212
feat: allow GitHub Actions expressions for list-valued safe-output constraints#29212
Conversation
Add GitHub Actions expression support for list-valued safe-output fields: - safe-outputs.push-to-pull-request-branch.labels - safe-outputs.push-to-pull-request-branch.allowed-repos - safe-outputs.create-pull-request.labels - safe-outputs.create-pull-request.allowed-repos - safe-outputs.create-pull-request.allowed-base-branches - safe-outputs.add-comment.allowed-repos Implementation: - preprocessStringArrayFieldAsTemplatable(): detects expression strings and wraps them in single-element []string for YAML unmarshal compat - ParseStringArrayOrExprFromConfig(): expression-aware array helper for manually-parsed configs (push-to-pull-request-branch) - AddTemplatableStringSlice(): builder method that emits expression single-element slices as JSON strings for runtime GH Actions evaluation - Schema updated to oneOf[array, string] for the 6 affected fields - 37 new unit tests covering all new paths Agent-Logs-Url: https://github.com/github/gh-aw/sessions/9c90c727-b7b8-4ad6-86cd-a09b109ab3f8 Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
- Add wantErrContains assertions to verify error messages are actionable - Remove unused fieldMap variable from TestExpressionFieldsRejectedForInvalidStrings - Import strings package for error message substring checks Agent-Logs-Url: https://github.com/github/gh-aw/sessions/9c90c727-b7b8-4ad6-86cd-a09b109ab3f8 Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
🧪 Test Quality Sentinel ReportTest Quality Score: 79/100
Test Classification DetailsView All Test Classifications (8 tests)
Flagged Tests — Suggestions
|
| Component | Score | Notes |
|---|---|---|
| Behavioral Coverage (40 pts) | 40 | All 8 tests verify observable outputs |
| Error/Edge Case Coverage (30 pts) | 19 | 5/8 tests include error or edge cases |
| Low Duplication (20 pts) | 20 | No duplicate clusters detected |
| Proportional Growth (10 pts) | 0 | Test inflation flag triggered (>2:1) |
| Total | 79/100 |
Language Support
Tests analyzed:
- 🟨 JavaScript: No JS test files changed
Verdict
✅ Check passed. 0% of new tests are implementation tests (threshold: 30%). All 8 tests enforce behavioral contracts. Build tags are present on both changed test files. No mock libraries detected.
📖 Understanding Test Classifications
Design Tests (High Value) verify what the system does:
- Assert on observable outputs, return values, or state changes
- Cover error paths and boundary conditions
- Would catch a behavioral regression if deleted
- Remain valid even after internal refactoring
Implementation Tests (Low Value) verify how the system does it:
- Assert on internal function calls (mocking internals)
- Only test the happy path with typical inputs
- Break during legitimate refactoring even when behavior is correct
- Give false assurance: they pass even when the system is wrong
Goal: Shift toward tests that describe the system's behavioral contract — the promises it makes to its users and collaborators.
References: §25140707612
🧪 Test quality analysis by Test Quality Sentinel · ● 412.7K · ◷
…afe-output constraints Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
|
Commit pushed:
|
🏗️ Design Decision Gate — ADR RequiredThis PR makes significant changes to core business logic (860 new lines in AI has analyzed the PR diff and generated a draft ADR to help you get started: 📄 Draft ADR: 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. Draft ADR SummaryThe generated draft covers:
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
References: §25140707660 Note 🔒 Integrity filter blocked 1 itemThe following item was blocked because it doesn't meet the GitHub integrity level.
To allow these resources, lower tools:
github:
min-integrity: approved # merged | approved | unapproved | none
|
There was a problem hiding this comment.
Pull request overview
Enables list-valued safe-output constraints (e.g., labels, allowed-repos, allowed-base-branches) to be supplied as GitHub Actions expression strings (primarily for workflow_call inputs), while keeping existing YAML []string unmarshal paths working.
Changes:
- Added Go preprocessing/parsing helpers to treat expression strings as single-element
[]stringfor YAML unmarshal, then re-emit as JSON strings for runtime evaluation. - Updated safe-output parsers/builders to accept and propagate these templatable list fields through to the handler config.
- Relaxed the JSON schema for the affected fields from strict arrays to
oneOf: [array, string], with accompanying schema test updates.
Show a summary per file
| File | Description |
|---|---|
| pkg/workflow/templatables.go | Adds preprocessing helper to wrap expression strings into []string for list fields. |
| pkg/workflow/config_helpers.go | Adds expression-aware array parsing helper for manually-parsed configs. |
| pkg/workflow/create_pull_request.go | Preprocesses list fields (labels, allowed-repos, allowed-base-branches) before unmarshal. |
| pkg/workflow/add_comment.go | Preprocesses allowed-repos before unmarshal. |
| pkg/workflow/push_to_pull_request_branch.go | Switches list parsing to expression-aware helper for labels / allowed-repos. |
| pkg/workflow/compiler_safe_outputs_builder.go | Adds AddTemplatableStringSlice to emit expression slices as JSON strings. |
| pkg/workflow/compiler_safe_outputs_handlers.go | Uses AddTemplatableStringSlice for the affected handler config keys. |
| pkg/workflow/config_parsing_helpers_test.go | Adds unit tests covering new templatable list behavior end-to-end. |
| pkg/parser/schemas/main_workflow_schema.json | Updates schema to allow string-or-array for the six affected fields and tweaks formatting. |
| pkg/parser/schema_test.go | Updates schema test to accept oneOf for allowed-base-branches. |
Copilot's findings
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comments suppressed due to low confidence (2)
pkg/parser/schemas/main_workflow_schema.json:5909
- For
create-pull-request.allowed-repos, the schema string option is currently unconstrained and the example usesinputs.allowed-repos(hyphenated input names require bracket notation). If only expressions are supported, add an expressionpattern(e.g.^\$\{\{.*\}\}$) to the string option and update the example to${{ inputs['allowed-repos'] }}so the schema doesn’t accept values the compiler will reject.
{
"type": "string",
"description": "Comma-separated list of repository slugs or a GitHub Actions expression resolving to such a list (e.g. '${{ inputs.allowed-repos }}')"
}
pkg/parser/schemas/main_workflow_schema.json:7291
- For
push-to-pull-request-branch.allowed-repos, the schema’s string variant currently accepts any string and documentsinputs.allowed-repos(invalid with hyphens). If runtime only supports expression strings here, add an expressionpattern(e.g.^\$\{\{.*\}\}$) to the string option and update the example to${{ inputs['allowed-repos'] }}so schema validation matches compiler behavior.
{
"type": "string",
"description": "Comma-separated list of repository slugs or a GitHub Actions expression resolving to such a list (e.g. '${{ inputs.allowed-repos }}')"
}
- Files reviewed: 11/11 changed files
- Comments generated: 6
| if s, ok := val.(string); ok { | ||
| if !isExpression(s) { | ||
| return fmt.Errorf("field %q must be an array of strings or a GitHub Actions expression (e.g. '${{ inputs.%s }}'), got string %q", fieldName, fieldName, s) | ||
| } |
There was a problem hiding this comment.
The error message example uses inputs.%s, but GitHub Actions expressions can’t use dot-notation for input names containing hyphens (e.g. allowed-repos, allowed-base-branches); inputs.allowed-repos is parsed as subtraction. Consider generating the example using bracket notation when fieldName contains - (e.g. ${{ inputs['allowed-repos'] }}), or use a non-hyphenated example like ${{ inputs.labels }} to avoid suggesting an invalid expression.
| { | ||
| "type": "string", | ||
| "description": "Comma-separated list of repository slugs or a GitHub Actions expression resolving to such a list (e.g. '${{ inputs.allowed-repos }}')" | ||
| } |
There was a problem hiding this comment.
The allowed-repos schema’s string variant currently allows any string and the description example uses inputs.allowed-repos (invalid for hyphenated input names). If the compiler only accepts GitHub Actions expressions here, consider adding a pattern like ^\$\{\{.*\}\}$ to the string option and updating the example to bracket notation (e.g. ${{ inputs['allowed-repos'] }}) so schema validation and docs match runtime behavior.
This issue also appears on line 5906 of the same file.
| }, | ||
| { | ||
| "type": "string", | ||
| "description": "Comma-separated list of label names or a GitHub Actions expression resolving to such a list (e.g. '${{ inputs.labels }}')" |
There was a problem hiding this comment.
For create-pull-request.labels, the schema’s string variant allows any string and describes comma-separated literals, but the compiler rejects non-expression bare strings. To keep schema validation aligned with actual behavior, add an expression pattern (e.g. ^\$\{\{.*\}\}$) to the string option and adjust the description to clarify it must be a GitHub Actions expression (not a literal comma-separated string).
| "description": "Comma-separated list of label names or a GitHub Actions expression resolving to such a list (e.g. '${{ inputs.labels }}')" | |
| "pattern": "^\\$\\{\\{.*\\}\\}$", | |
| "description": "GitHub Actions expression resolving to a comma-separated list of label names (e.g. '${{ inputs.labels }}')" |
| { | ||
| "type": "string", | ||
| "description": "Comma-separated list of base branch patterns or a GitHub Actions expression resolving to such a list (e.g. '${{ inputs.allowed-base-branches }}')" | ||
| } |
There was a problem hiding this comment.
For create-pull-request.allowed-base-branches, the schema’s string variant permits any string and the example uses inputs.allowed-base-branches (dot-notation is invalid with hyphens). If this field is intended to accept only GitHub Actions expressions, add an expression pattern (e.g. ^\$\{\{.*\}\}$) to the string option and update the example to ${{ inputs['allowed-base-branches'] }}.
| { | ||
| "type": "string", | ||
| "description": "Comma-separated list of label names or a GitHub Actions expression resolving to such a list (e.g. '${{ inputs.required-labels }}')" | ||
| } |
There was a problem hiding this comment.
For push-to-pull-request-branch.labels, the schema string variant is unconstrained and the example uses inputs.required-labels (hyphenated input names require bracket notation). If only expressions are accepted, add an expression pattern (e.g. ^\$\{\{.*\}\}$) to the string option and update the example to ${{ inputs['required-labels'] }} to prevent schema/behavior mismatch.
This issue also appears on line 7288 of the same file.
| "labels": "${{ inputs.required-labels }}", | ||
| }, | ||
| key: "labels", | ||
| expected: []string{"${{ inputs.required-labels }}"}, | ||
| }, | ||
| { | ||
| name: "expression with extra whitespace is still valid", | ||
| input: map[string]any{ | ||
| "allowed-repos": "${{ inputs.allowed-repos }}", | ||
| }, | ||
| key: "allowed-repos", | ||
| expected: []string{"${{ inputs.allowed-repos }}"}, |
There was a problem hiding this comment.
Several new tests use dot-notation to reference inputs keys that contain hyphens (e.g. ${{ inputs.required-labels }}, ${{ inputs.allowed-repos }}). In GitHub Actions expressions, hyphenated keys require bracket notation (e.g. ${{ inputs['required-labels'] }}), otherwise the expression is parsed as subtraction. Updating these test strings will keep the examples aligned with real Actions syntax (this pattern also appears in other new tests below).
| "labels": "${{ inputs.required-labels }}", | |
| }, | |
| key: "labels", | |
| expected: []string{"${{ inputs.required-labels }}"}, | |
| }, | |
| { | |
| name: "expression with extra whitespace is still valid", | |
| input: map[string]any{ | |
| "allowed-repos": "${{ inputs.allowed-repos }}", | |
| }, | |
| key: "allowed-repos", | |
| expected: []string{"${{ inputs.allowed-repos }}"}, | |
| "labels": "${{ inputs['required-labels'] }}", | |
| }, | |
| key: "labels", | |
| expected: []string{"${{ inputs['required-labels'] }}"}, | |
| }, | |
| { | |
| name: "expression with extra whitespace is still valid", | |
| input: map[string]any{ | |
| "allowed-repos": "${{ inputs['allowed-repos'] }}", | |
| }, | |
| key: "allowed-repos", | |
| expected: []string{"${{ inputs['allowed-repos'] }}"}, |
|
@copilot review comments add tests for all forms of list encodings |
…-forms tests
- templatables.go: use bracket notation in error message example when
fieldName contains hyphens (e.g. inputs['allowed-repos'] not inputs.allowed-repos)
- schema: add pattern ^\\$\\{\\{.*\\}\\}$ to all 6 string-variant fields so
the schema enforces expression-only strings; update all examples to use
bracket notation for hyphenated input names
- tests: fix all expression examples to use bracket notation for hyphenated
input names (${{ inputs['allowed-repos'] }}, ${{ inputs['required-labels'] }},
${{ inputs['allowed-base-branches'] }})
- tests: add TestAllListEncodingForms covering literal arrays (multi-item,
single-item, empty), expression strings (inputs/vars/env/fallback forms),
builder JSON output, helper parsing, and error-message content assertions
Agent-Logs-Url: https://github.com/github/gh-aw/sessions/be46f96a-d26a-494c-bf7b-508b2f2daaaf
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Done in a6ba43c. Three things addressed per review feedback:
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:
|
List-valued safe-output fields (
labels,allowed-repos,allowed-base-branches) required literal YAML arrays, preventing reusableworkflow_callworkflows from supplying these constraints as runtime inputs.Changes
New helpers (Go)
preprocessStringArrayFieldAsTemplatable()— detects expression strings inconfigDataand wraps them in[]string{expr}so existing[]stringstruct fields survive YAML unmarshal unchanged; uses bracket notation in error-message examples for hyphenated field names (e.g.${{ inputs['allowed-repos'] }})ParseStringArrayOrExprFromConfig()— expression-aware drop-in forParseStringArrayFromConfig, used by manually-parsed configsAddTemplatableStringSlice()onhandlerConfigBuilder— emits a single-element expression slice as a JSON string (GitHub Actions evaluates it at heredoc-expansion time) and multi-element/literal slices as JSON arraysParser updates
push_to_pull_request_branch.go— usesParseStringArrayOrExprFromConfigforlabelsandallowed-reposcreate_pull_request.go— preprocesseslabels,allowed-repos,allowed-base-branchesbeforeparseConfigScaffoldadd_comment.go— preprocessesallowed-reposcompiler_safe_outputs_handlers.go— switchesAddStringSlice→AddTemplatableStringSlicefor the 6 affected fieldsSchema
type: "array"tooneOf: [{type: "array"}, {type: "string", pattern: "^\\$\\{\\{.*\\}\\}$"}], ensuring schema validation enforces that the string variant only accepts GitHub Actions expressions${{ inputs['allowed-repos'] }},${{ inputs['required-labels'] }})Example
At runtime, GitHub Actions evaluates the expression inside the heredoc before
config.jsonis written. The existing JS handlers (parseAllowedRepos,parseStringListConfig,parseAllowedBaseBranches) already handle comma-separated strings, so no JS changes are needed.Non-expression bare strings are rejected with an actionable error message using syntactically correct notation for the field name.
Testing
Added
TestAllListEncodingFormscovering all supported compile-time list encoding forms:inputs[...](bracket notation),vars.,env., fallback||operatorParseStringArrayOrExprFromConfighelper: all[]string,[]any, and expression forms