Fix JSON-array expressions in approve workflow run safe outputs - #53812
Conversation
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
|
✅ 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.
Pull request overview
Fixes malformed safe-output JSON when dynamic pull-request allowlists evaluate to arrays.
Changes:
- Adds JSON-aware expression marshalling.
- Applies it to both safe-output configurations.
- Adds regression tests and recompiles the workflow.
Show a summary per file
| File | Description |
|---|---|
pkg/workflow/safe_outputs_handler_registry.go |
Uses JSON-slice emission for allowlists. |
pkg/workflow/safe_outputs_config_runtime.go |
Uses expression-aware runtime marshalling. |
pkg/workflow/safe_outputs_config_generation.go |
Uses expression-aware config generation. |
pkg/workflow/compiler_safe_outputs_builder.go |
Adds JSON-expression handling. |
pkg/workflow/approve_workflow_run_test.go |
Tests array substitution. |
pkg/workflow/approve_workflow_run_integration_test.go |
Tests compiled handler configuration. |
.github/workflows/pr-sous-chef.lock.yml |
Regenerates the workflow with unquoted allowlist emission. |
Review details
💡 Add a code-review agent skill for context-aware, tailored reviews. Learn more in the docs.
- Files reviewed: 7/7 changed files
- Comments generated: 1
- Review effort level: Balanced
| return b | ||
| } | ||
| if len(value) == 1 && isExpression(value[0]) { | ||
| b.config[key] = templatableJSONExpression(value[0]) |
There was a problem hiding this comment.
Fixed in 5cf8bcc: AddTemplatableJSONSlice now wraps the expression with ${{ toJSON(...) }} before splicing it in, so an empty-string result serializes to "" (still valid JSON) instead of an empty token. Added a regression test covering the empty-result case.
|
✅ Design Decision Gate 🏗️ completed the design decision gate check.
|
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅
|
|
✅ Ponytail Reviewer completed successfully! Lean already. Ship. Reviewed all non-generated diff (Go source + tests) for over-engineering per ponytail-review skill. The templatableJSONExpression marker/placeholder mechanism, templatableJSONExpressions visitor, and AddTemplatableJSONSlice builder are directly load-bearing for the fix (avoiding double-JSON-encoding an already-JSON expression) - no dead flexibility, no reinvented stdlib, no single-caller abstraction to inline, nothing shrinkable. Test additions are proportionate regression coverage.
|
|
✅ PR Code Quality Reviewer completed the code quality review.
|
Comment MemoryNote This comment is managed by comment memory.It stores persistent context for this thread in the code block at the top of this comment.
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Verdict
I didn't find a blocking issue in the changed lines.
Why this passes review
The risky part of this change was the new raw-JSON expression path for allowed_pull_requests. The implementation updates both handler-manager and runtime config generation to share the same marshaling path, and the new tests cover both the literal-list case and expression substitution parsing back into valid JSON.
I did not find a correctness regression in the changed code after checking the diff and the added regression coverage.
🔎 Code quality review by PR Code Quality Reviewer · gpt54 · 8.8 AIC · ⌖ 12.7 AIC · ⊞ 7K
Comment /review to run again
There was a problem hiding this comment.
The fix correctly removes the wrapping double-quotes around the ${{ needs.approval_allowlist.outputs.eligible_pull_request_numbers }} expression in the GH_AW_SAFE_OUTPUTS_CONFIG env var. Previously, the JSON array was injected as a string value; now it is injected as a raw JSON array, which is what allowed_pull_requests expects. No blocking issues found.
🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · sonnet46 · 19.2 AIC · ⌖ 8.77 AIC · ⊞ 5.7K
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /diagnosing-bugs and /tdd — commenting on two correctness concerns.
📋 Key Themes & Highlights
Key Themes
- Magic-string collision risk: The fixed
templatableJSONExpressionPrefixcan theoretically appear inside a user-supplied expression, causingbytes.ReplaceAllto corrupt unrelated config values. Low probability but worth hardening. - Untested multi-element path:
AddTemplatableJSONSlicesilently falls through to[]stringwhenlen(value) > 1. No test pins this behaviour. - Array shape coverage: Tests substitute string arrays (
["123","456"]) but not numeric arrays ([123,456]), which are a plausible runtime shape.
Positive Highlights
- ✅ Clean design:
templatableJSONExpressionwith a customMarshalJSONkeeps the type system honest and the post-processing explicit. - ✅ Consistent wiring: both
safe_outputs_config_generation.goandsafe_outputs_config_runtime.goupdated together — no split-brain risk. - ✅ Good regression test (
TestApproveWorkflowRunAllowedPullRequestsExpressionEmitsJSONArray) covers the original corrupted-payload scenario end-to-end.
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · sonnet46 · 48.5 AIC · ⌖ 10.3 AIC · ⊞ 7.8K
Comment /matt to run again
| @@ -11,6 +14,14 @@ type handlerConfigBuilder struct { | |||
| config map[string]any | |||
There was a problem hiding this comment.
[/diagnosing-bugs] Collision risk with fixed placeholder prefix: __GH_AW_TEMPLATABLE_JSON_EXPRESSION__: is a static string embedded verbatim in the marshalled JSON before the find-and-replace pass. If a user expression ever contained this prefix, bytes.ReplaceAll would corrupt other values in the config payload.
💡 Safer approach: per-marshal index-based markers
Generate per-call placeholders indexed by position to make collisions impossible:
func marshalSafeOutputsConfig(config map[string]any) ([]byte, error) {
expressions := templatableJSONExpressions(config)
configJSON, err := json.Marshal(config)
if err != nil {
return nil, err
}
for i, expr := range expressions {
placeholder, _ := json.Marshal(fmt.Sprintf("%s%d:", templatableJSONExpressionPrefix, i))
configJSON = bytes.ReplaceAll(configJSON, placeholder, []byte(expr))
}
return configJSON, nil
}And make MarshalJSON on templatableJSONExpression include the index (or use a UUID nonce). This prevents a user-supplied expression like ${{ env.__GH_AW_TEMPLATABLE_JSON_EXPRESSION__:something }} from poisoning replacements.
@copilot please address this.
There was a problem hiding this comment.
Fixed in 4851485: replaced the static sentinel-only placeholder with a per-instance placeholder that embeds a unique numeric id assigned via an atomic counter when the expression is created. This ties uniqueness to the expression instance rather than to marshal traversal order, so a user-supplied value containing the sentinel text can no longer collide with an internal placeholder.
| b.config[key] = templatableJSONExpression(value[0]) | ||
| return b | ||
| } | ||
| b.config[key] = value |
There was a problem hiding this comment.
[/tdd] The multi-element case in AddTemplatableJSONSlice — where len(value) > 1 and some elements are expressions — is untested and silently falls through to b.config[key] = value (a []string). This means mixed literal+expression slices would still be marshalled as a JSON array of quoted strings, not as raw JSON.
💡 Add a test and clarify the intent
Either document that multi-element expression slices are unsupported (and add a test that confirms the current []string path), or add handling:
func TestApproveWorkflowRunAllowedPullRequestsMixedLiterals(t *testing.T) {
// two literal PR numbers — should stay as a JSON array of strings
config := &ApproveWorkflowRunConfig{AllowedPullRequests: []string{"123", "456"}}
handlerConfig := handlerRegistry["approve_workflow_run"](&SafeOutputsConfig{ApproveWorkflowRun: config})
configJSON, err := marshalSafeOutputsConfig(map[string]any{"approve_workflow_run": handlerConfig})
require.NoError(t, err)
assert.Contains(t, string(configJSON), `"allowed_pull_requests":["123","456"]`)
}Without this test the behaviour of the multi-element path is undocumented and could regress silently.
@copilot please address this.
There was a problem hiding this comment.
Documented the intent and added TestApproveWorkflowRunAllowedPullRequestsMixedLiteralsAndExpression in 4851485, confirming a multi-element slice stays a plain JSON array of strings rather than being treated as a templatable expression.
|
|
||
| runtimeConfig := strings.ReplaceAll(string(configJSON), expression, `["123","456"]`) | ||
| var config map[string]map[string]any | ||
| require.NoError(t, json.Unmarshal([]byte(runtimeConfig), &config)) |
There was a problem hiding this comment.
[/tdd] The integration test substitutes the expression with ["123","456"] (strings), but the real runtime value from a GitHub Actions output would be an array of numbers (integers) when PR numbers are returned numerically. It would be worth verifying both ["123","456"] (strings) and [123,456] (numbers) parse without error, since the handler consuming allowed_pull_requests may type-assert the values.
💡 Extend the test
// also test numeric JSON array (the more common runtime shape)
runtimeConfigNumeric := strings.ReplaceAll(string(configJSON), expression, `[123,456]`)
var numericConfig map[string]map[string]any
require.NoError(t, json.Unmarshal([]byte(runtimeConfigNumeric), &numericConfig))
assert.Equal(t, []any{float64(123), float64(456)}, numericConfig["approve_workflow_run"]["allowed_pull_requests"])This ensures the fix works for whichever array shape the upstream expression produces.
@copilot please address this.
There was a problem hiding this comment.
Added a numeric-array subtest in 4851485 (TestApproveWorkflowRunAllowedPullRequestsExpressionEmitsJSONArray/numeric_array) verifying [123,456] parses to []any{float64(123), float64(456)}, alongside the existing string-array case.
🏗️ Design Decision Gate — ADR RequiredThis PR makes significant changes to core business logic (101 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
|
|
@copilot run pr-finisher skill |
…ceholders Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Addressed all four open review threads in 4851485: wrapped the expression in |
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
PR Triage
Fixes safe-output config payload corruption from unescaped JSON-array expressions. Prior automated approval was dismissed — needs a fresh human review pass given the high-risk safe-output surface area.
|
Dynamic
allowed-pull-requestsvalues were emitted as quoted JSON strings. When an expression evaluated to a JSON array, its embedded quotes corrupted both safe-output configuration payloads and prevented the handler from starting.Raw JSON expression emission
approve_workflow_run.allowed_pull_requestsexpressions as JSON values rather than JSON strings.config.jsongeneration.{ "allowed_pull_requests": ${{ needs.approval_allowlist.outputs.eligible_pull_request_numbers }} }Regression coverage
["123","456"]) parses as valid JSON.Generated workflow
pr-sous-chef.lock.ymlwith the corrected configuration emission.