From 24b0b1ac0a67e95a03612568e482d92252be4701 Mon Sep 17 00:00:00 2001 From: nodeselector Date: Fri, 24 Jul 2026 08:24:19 -0500 Subject: [PATCH 1/2] lockfile: emit domain-specific duplicate dependency key parse error MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Today a lockfile with two identical keys under `dependencies:` slips dispatch-time validation: workflowparser.ParseWithCalledWorkflows returns nil at parse (unlike algo-hex/missing-field defects, which return a lockfile ParseError and yield a 422 at non-UI dispatch), so the run is created (HTTP 202) and only dies later as an opaque startup_failure with no API/GraphQL-retrievable reason. On no-policy repos this is a pre-enforcement fail-open where a dup lock can silently drop pins. This makes a duplicate dependency key a first-class parse rejection in the same parseInternal path that produces the other lockfile field errors. rejectDuplicateDependencyKeys walks the parsed yaml.Node tree for the top-level `dependencies` MappingNode and scans its key children for raw-string duplicates BEFORE root.Decode(&f) — so it does not depend on implicit yaml.v3 Decode strictness — returning a positioned *ParseError: `duplicate dependency key %q in lockfile dependencies (first defined at line %d)` with Line/Column from the offending node. It surfaces identically to the algo-hex/missing-field checks, so launch emits a clear dispatch-time 422. Node-shape defensive: unexpected shapes fall through to the normal decode path. Adds a regression test (conflicting-SHA fixture) asserting Parse and ParseWithPolicy return an error containing "duplicate dependency key" that is a *ParseError with a non-zero Line — proving the domain path (422) rather than a bare yaml error. --- go/pkg/lockfile/compat.go | 35 ++++++++++++++++++++++++++++ go/pkg/lockfile/compat_test.go | 42 ++++++++++++++++++++++++++++++++++ 2 files changed, 77 insertions(+) diff --git a/go/pkg/lockfile/compat.go b/go/pkg/lockfile/compat.go index 9deb9c9..63bd7d0 100644 --- a/go/pkg/lockfile/compat.go +++ b/go/pkg/lockfile/compat.go @@ -53,6 +53,12 @@ func parseInternal(contents []byte, policy *VersionPolicy, paths []string) (File if err := yaml.Unmarshal(contents, &root); err != nil { return File{}, newYAMLParseError(err) } + // yaml.v3's Decode rejects duplicate mapping keys, but with a generic + // message. Detect duplicate dependency keys first so we can return a + // domain-specific, positioned error before the generic decode error fires. + if pe := rejectDuplicateDependencyKeys(&root); pe != nil { + return File{}, pe + } var f File if err := root.Decode(&f); err != nil { return File{}, newYAMLParseError(err) @@ -477,3 +483,32 @@ func validateKnownFieldsVersioned(f *File, paths []string, version string) *Pars } return nil } + +// rejectDuplicateDependencyKeys walks the top-level `dependencies` mapping and +// returns a positioned ParseError on the first duplicate key. yaml.v3's Decode +// would reject duplicates too, but with a generic message; this yields a +// domain-specific one. Defensive: any unexpected node shape returns nil and +// lets the normal decode path handle it. +func rejectDuplicateDependencyKeys(root *yaml.Node) *ParseError { + m := docMapping(root) + if m == nil { + return nil + } + _, deps := mappingEntry(m, "dependencies") + if deps == nil || deps.Kind != yaml.MappingNode { + return nil + } + seen := make(map[string]int, len(deps.Content)/2) + for i := 0; i+1 < len(deps.Content); i += 2 { + k := deps.Content[i] + if first, dup := seen[k.Value]; dup { + return &ParseError{ + Line: k.Line, + Column: k.Column, + Msg: fmt.Sprintf("duplicate dependency key %q in lockfile dependencies (first defined at line %d)", k.Value, first), + } + } + seen[k.Value] = k.Line + } + return nil +} diff --git a/go/pkg/lockfile/compat_test.go b/go/pkg/lockfile/compat_test.go index 04cc805..2391a4c 100644 --- a/go/pkg/lockfile/compat_test.go +++ b/go/pkg/lockfile/compat_test.go @@ -237,3 +237,45 @@ func TestDefaultPolicy(t *testing.T) { assert.Equal(t, "v0.0.1", p.Min) assert.Equal(t, Version, p.Max) } + +func TestParse_DuplicateDependencyKey_ConflictingSHAs(t *testing.T) { + // Two identical dependency keys with conflicting commit SHAs must be + // rejected with a domain-specific, positioned parse error rather than + // yaml.v3's generic "mapping key already defined" message. + input := `version: 'v0.0.2' +workflows: + '.github/workflows/scenario-duplicate-deps-conflicting-failure.yml': + - 'nodeselector/actions-test-fixtures@dup-test' +dependencies: + 'nodeselector/actions-test-fixtures@dup-test': + ref: 'dup-test' + commit: 'sha1-33a384c001ed694ba938667a1d5ace65d6c49de3' + owner_id: 29457092 + repo_id: 1203329948 + 'nodeselector/actions-test-fixtures@dup-test': + ref: 'dup-test' + commit: 'sha1-0000000000000000000000000000000000000000' + owner_id: 29457092 + repo_id: 1203329948 +` + for _, tc := range []struct { + name string + parse func() (File, error) + }{ + {"Parse", func() (File, error) { return Parse([]byte(input)) }}, + {"ParseWithPolicy", func() (File, error) { return ParseWithPolicy([]byte(input), DefaultPolicy()) }}, + } { + t.Run(tc.name, func(t *testing.T) { + _, err := tc.parse() + require.Error(t, err) + assert.Contains(t, err.Error(), "duplicate dependency key") + assert.Contains(t, err.Error(), "nodeselector/actions-test-fixtures@dup-test") + + // Must be the domain path (positioned *ParseError), not a bare + // yaml.v3 error — this is what surfaces as a dispatch-time 422. + var pe *ParseError + require.ErrorAs(t, err, &pe) + assert.NotZero(t, pe.Line, "duplicate key error must carry a line position") + }) + } +} From e74b8e68f034776276839ff90d557dae3330a053 Mon Sep 17 00:00:00 2001 From: nodeselector Date: Mon, 27 Jul 2026 12:32:44 -0500 Subject: [PATCH 2/2] lockfile: only dedup-check real string scalar dependency keys Guard rejectDuplicateDependencyKeys to skip non-scalar / non-!!str key nodes before consulting the seen map. Alias keys or empty-Value collection keys could otherwise collide on Node.Value and report a misleading domain-specific duplicate, contradicting the documented fallback that unexpected node shapes defer to the normal decode path. --- go/pkg/lockfile/compat.go | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/go/pkg/lockfile/compat.go b/go/pkg/lockfile/compat.go index 63bd7d0..78bda77 100644 --- a/go/pkg/lockfile/compat.go +++ b/go/pkg/lockfile/compat.go @@ -501,6 +501,12 @@ func rejectDuplicateDependencyKeys(root *yaml.Node) *ParseError { seen := make(map[string]int, len(deps.Content)/2) for i := 0; i+1 < len(deps.Content); i += 2 { k := deps.Content[i] + // Only compare real string scalar keys. Aliases or non-scalar keys can + // share a Value (e.g. empty), so skip them and let Decode surface the + // normal shape error rather than a misleading duplicate. + if k.Kind != yaml.ScalarNode || k.Tag != "!!str" { + continue + } if first, dup := seen[k.Value]; dup { return &ParseError{ Line: k.Line,