Skip to content

Commit aafa257

Browse files
toabctlclaude
andcommitted
fix(build): make MutateWith deterministic for forwarded inputs
When pipeline A `uses:` B and B `uses:` C, and B forwards a same-named input down to C (`with: { foo: ${{inputs.foo}} }`), compilePipeline merges the parent's resolved substitution map into the child's `with`. The merged map then holds two entries that collide on one key in MutateWith: "${{inputs.foo}}" -> "<resolved parent value>" (inherited from parent) "foo" -> "${{inputs.foo}}" (child's forwarded input) The insert loop wrote both to nw["${{inputs.foo}}"] and Go's randomized map iteration decided the winner. When the self-reference won it resolved to itself (single pass) and stayed unsubstituted, so the literal `${{` leaked into Runs and stripComments failed with "invalid parameter name". The same collision also silently produced the inherited value instead of a legitimate literal override (`with: { foo: secret }`) ~13% of runs. Resolve deterministically by precedence: seed the inherited "${{...}}" entries first, then apply this pipeline's own inputs authoritatively (iterating input names in sorted order), resolving each value against the parent scope so a forwarded self-reference becomes the parent's value and a literal override always wins. This composes to arbitrary `uses:` depth because each level passes a fully-resolved map to the next. Tests (all repeated to defeat randomized map order; they fail on the old code and pass with this change): - Test_MutateWith_ForwardedSameNamedInput: forwarded self-ref, literal override, child-default-wins, no-collision, sibling-ref, multi-key. - Test_MutateWith_DeepForwarding: 6-level forwarding chain. - Test_CompilePipelines_NestedForwardedInput: end-to-end compile of a 3-level nested `uses:`, reproducing the stripComments failure. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Signed-off-by: Thomas Bechtold <thomas.bechtold@chainguard.dev>
1 parent 6e37def commit aafa257

2 files changed

Lines changed: 184 additions & 4 deletions

File tree

pkg/build/pipeline.go

Lines changed: 21 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -23,6 +23,7 @@ import (
2323
"os/signal"
2424
"path"
2525
"path/filepath"
26+
"sort"
2627
"strconv"
2728
"strings"
2829

@@ -40,15 +41,31 @@ const WorkDir = "/home/build"
4041
func (sm *SubstitutionMap) MutateWith(with map[string]string) (map[string]string, error) {
4142
nw := maps.Clone(sm.Substitutions)
4243

44+
// Deterministic (Go map order is random) merge of `with`: seed the parent's
45+
// inherited "${{...}}" entries, then this pipeline's plain inputs in sorted
46+
// order, resolved vs the parent so a forwarded self-ref takes its value.
4347
for k, v := range with {
44-
// already mutated?
4548
if strings.HasPrefix(k, "${{") {
4649
nw[k] = v
47-
} else {
48-
nk := fmt.Sprintf("${{inputs.%s}}", k)
49-
nw[nk] = v
5050
}
5151
}
52+
inputKeys := make([]string, 0, len(with))
53+
for k := range with {
54+
if !strings.HasPrefix(k, "${{") {
55+
inputKeys = append(inputKeys, k)
56+
}
57+
}
58+
sort.Strings(inputKeys)
59+
for _, k := range inputKeys {
60+
v := with[k]
61+
nk := fmt.Sprintf("${{inputs.%s}}", k)
62+
// Resolve against the parent scope; keep raw if not yet in scope (a
63+
// sibling input) — the mutation loop below resolves it.
64+
if rv, err := util.MutateStringFromMap(nw, v); err == nil {
65+
v = rv
66+
}
67+
nw[nk] = v
68+
}
5269

5370
// do the actual mutations
5471
for k, v := range nw {

pkg/build/pipeline_test.go

Lines changed: 163 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -15,6 +15,7 @@
1515
package build
1616

1717
import (
18+
"maps"
1819
"os"
1920
"path/filepath"
2021
"testing"
@@ -107,6 +108,168 @@ func Test_MutateWith(t *testing.T) {
107108
}
108109
}
109110

111+
// Regression: a nested `uses:` that forwards a same-named input (compile.go
112+
// merges the parent's resolved "${{inputs.X}}" with the child's own "X") must
113+
// resolve deterministically regardless of Go's random map order.
114+
func Test_MutateWith_ForwardedSameNamedInput(t *testing.T) {
115+
sm := &SubstitutionMap{Substitutions: map[string]string{}}
116+
117+
for _, tc := range []struct {
118+
name string
119+
with map[string]string
120+
wants map[string]string // key -> expected resolved value
121+
}{{
122+
name: "forwarded self-reference resolves to parent value",
123+
with: map[string]string{
124+
"${{inputs.admin-password}}": "adminpw", // inherited from parent
125+
"admin-password": "${{inputs.admin-password}}", // forwarded down
126+
},
127+
wants: map[string]string{"${{inputs.admin-password}}": "adminpw"},
128+
}, {
129+
name: "literal override wins over inherited value",
130+
with: map[string]string{
131+
"${{inputs.admin-password}}": "adminpw",
132+
"admin-password": "literalsecret",
133+
},
134+
wants: map[string]string{"${{inputs.admin-password}}": "literalsecret"},
135+
}, {
136+
name: "child default wins over inherited parent value",
137+
with: map[string]string{
138+
"${{inputs.tls-cert-dir}}": "/tmp/tls", // parent's value
139+
"tls-cert-dir": "", // child's default (validateWith filled it)
140+
},
141+
wants: map[string]string{"${{inputs.tls-cert-dir}}": ""},
142+
}, {
143+
name: "plain input with no collision resolves normally",
144+
with: map[string]string{"foo": "bar"},
145+
wants: map[string]string{"${{inputs.foo}}": "bar"},
146+
}, {
147+
name: "sibling reference resolves regardless of order",
148+
with: map[string]string{
149+
"a": "${{inputs.b}}",
150+
"b": "bee",
151+
},
152+
wants: map[string]string{
153+
"${{inputs.a}}": "bee",
154+
"${{inputs.b}}": "bee",
155+
},
156+
}, {
157+
name: "multiple forwarded inputs all resolve to parent values",
158+
with: map[string]string{
159+
"${{inputs.password}}": "pw",
160+
"password": "${{inputs.password}}",
161+
"${{inputs.cert-dir}}": "/d",
162+
"cert-dir": "${{inputs.cert-dir}}",
163+
},
164+
wants: map[string]string{
165+
"${{inputs.password}}": "pw",
166+
"${{inputs.cert-dir}}": "/d",
167+
},
168+
}} {
169+
t.Run(tc.name, func(t *testing.T) {
170+
// Run many times to defeat randomized map iteration order.
171+
for i := range 1000 {
172+
got, err := sm.MutateWith(tc.with)
173+
if err != nil {
174+
t.Fatalf("MutateWith: %v", err)
175+
}
176+
for k, want := range tc.wants {
177+
if v := got[k]; v != want {
178+
t.Fatalf("iter %d: %s: got %q, want %q", i, k, v, want)
179+
}
180+
}
181+
}
182+
})
183+
}
184+
}
185+
186+
// Forwarding the same-named input down many levels must keep resolving to the
187+
// top value at any depth (each level passes a fully-resolved map to the next).
188+
func Test_MutateWith_DeepForwarding(t *testing.T) {
189+
sm := &SubstitutionMap{Substitutions: map[string]string{}}
190+
for i := range 1000 {
191+
// Top level: the input takes its real value.
192+
m, err := sm.MutateWith(map[string]string{"admin-password": "adminpw"})
193+
if err != nil {
194+
t.Fatalf("iter %d top: %v", i, err)
195+
}
196+
// Each deeper level inherits the parent's resolved map and forwards the
197+
// same-named input (the self-referential `X: ${{inputs.X}}`).
198+
for lvl := 1; lvl <= 5; lvl++ {
199+
child := maps.Clone(m)
200+
child["admin-password"] = "${{inputs.admin-password}}"
201+
m, err = sm.MutateWith(child)
202+
if err != nil {
203+
t.Fatalf("iter %d level %d: %v", i, lvl, err)
204+
}
205+
}
206+
if v := m["${{inputs.admin-password}}"]; v != "adminpw" {
207+
t.Fatalf("iter %d: 6-level forwarded value = %q, want %q", i, v, "adminpw")
208+
}
209+
}
210+
}
211+
212+
// Functional: compile a 3-level nested `uses:` (parent->child->grandchild) that
213+
// forwards a same-named input down to a `runs:` block. Pre-fix this
214+
// intermittently failed stripComments with "invalid parameter name".
215+
func Test_CompilePipelines_NestedForwardedInput(t *testing.T) {
216+
ctx := slogtest.Context(t)
217+
dir := t.TempDir()
218+
write := func(name, body string) {
219+
t.Helper()
220+
require.NoError(t, os.WriteFile(filepath.Join(dir, name), []byte(body), 0o644))
221+
}
222+
write("parent.yaml", `
223+
inputs:
224+
password:
225+
default: defaultpw
226+
pipeline:
227+
- uses: child
228+
with:
229+
password: ${{inputs.password}}
230+
`)
231+
write("child.yaml", `
232+
inputs:
233+
password:
234+
default: childdefault
235+
pipeline:
236+
- uses: grandchild
237+
with:
238+
password: ${{inputs.password}}
239+
`)
240+
write("grandchild.yaml", `
241+
inputs:
242+
password:
243+
default: grandchilddefault
244+
pipeline:
245+
- runs: |
246+
echo "pw=${{inputs.password}}"
247+
`)
248+
249+
// repeats is an iteration count (NOT nesting depth); the pre-fix bug was
250+
// intermittent (random map order), so repeat to fail reliably.
251+
const repeats = 200
252+
for i := range repeats {
253+
cfg := config.Configuration{
254+
Package: config.Package{Name: "foo", Version: "1.2.3"},
255+
Pipeline: []config.Pipeline{{
256+
Uses: "parent",
257+
With: map[string]string{"password": "topsecret"},
258+
}},
259+
}
260+
c := &Compiled{PipelineDirs: []string{dir}}
261+
sm, err := NewSubstitutionMap(&cfg, "", "", nil)
262+
require.NoError(t, err)
263+
264+
require.NoError(t, c.CompilePipelines(ctx, sm, cfg.Pipeline), "iter %d", i)
265+
266+
// parent -> child -> grandchild -> runs step
267+
runs := cfg.Pipeline[0].Pipeline[0].Pipeline[0].Pipeline[0].Runs
268+
require.Contains(t, runs, "pw=topsecret", "iter %d: runs=%q", i, runs)
269+
require.NotContains(t, runs, "${{", "iter %d: unresolved template in runs=%q", i, runs)
270+
}
271+
}
272+
110273
func Test_substitutionNeedPackages(t *testing.T) {
111274
ctx := slogtest.Context(t)
112275
pkg := config.Package{

0 commit comments

Comments
 (0)