Resolve YAML aliases and merge keys inside dependency maps and build steps

[cpuguy83]

Fixes #1134.

YAML aliases and <<: merge keys that reference a top-level anchor failed to load
with could not find alias when used inside a dependency package map — and, as an
audit turned up, inside build.steps as well. The same anchor resolves when the
merge is applied at a level that is decoded as a whole (e.g. the dependencies:
node), which is the tell for the root cause.

Why

The custom unmarshallers that attach source maps re-decoded child AST nodes in
isolation, without the document's anchor table:

• PackageConstraints.UnmarshalYAML used yaml.NodeToValue (no reference readers).
• PackageDependencyList.UnmarshalYAML hand-iterated the mapping and never expanded
  <<: merge-key nodes.
• BuildStepList.UnmarshalYAML hand-iterated the sequence and decoded each step with
  yaml.NodeToValue, duplicating what BuildStep.UnmarshalYAML already does.

None of these isolated decodes carried the document's anchors, so an alias landing
inside them could not be resolved. This regressed in ef94b3d8 when the node-based
unmarshallers were introduced for source maps.

The project already has the right primitive: getDecoder(ctx) builds a decoder
configured with yaml.ReferenceReaders(baseDoc), which repopulates the anchor
table. The fix routes these decodes through it and lets the YAML library expand
merge keys and resolve aliases natively.

Changes

• Decode PackageConstraints through the reference-reader decoder so an alias used
  as a package value resolves against the top-level anchor.
• Decode the whole package mapping in one pass so <<: merge keys and aliases are
  expanded natively, keeping the source-map refinement (start line pulled up to the
  package key) for directly-specified entries; merged entries keep the source map of
  their anchor definition.
• Decode build.steps as a whole sequence and let BuildStep resolve aliases/merge
  keys and attach its own source map (removing the redundant, buggy manual loop).
• Fix dependency lists written in the sequence-of-names form being silently dropped:
  the decoded result was built but never assigned.

Audit

I checked every custom UnmarshalYAML / node re-decode and verified each with a real
spec. The only broken sites were the dependency maps and build.steps (both fixed
here). Scalar sourceMappedValue fields (test command, dependency names, uid/gid)
and the full-document decodes are fine, because they are reached through a
getDecoder-decoded parent (or already contain the anchor definitions).

Tests

• deps_test.go: alias-as-value, merge-key-inside-map, merge at the dependencies
  level, explicit-overrides-merged precedence, aliases for runtime lists, the
  sequence-of-names form, and source-map attribution for merged vs. direct packages.
• spec_test.go: aliased and merge-key build steps resolve to the anchor's step.

[Copilot]

Pull request overview

This PR fixes a YAML parsing regression (issue #1134) where aliases used as a package's value, and <<: merge keys referencing a top-level anchor, failed to load inside dependency package maps with could not find alias. The custom unmarshallers in deps.go previously re-decoded child nodes without the document's anchor table. The fix decodes package constraints and the whole package mapping through the reference-reader–aware decoder, and additionally repairs the sequence-of-names dependency form that was being silently dropped.

Changes:

• Decode PackageConstraints and the entire dependency mapping via getDecoder(ctx).DecodeFromNodeContext so anchors and <<: merge keys resolve, while preserving the source-map "pull up to the package key" behavior for directly-specified entries (and skipping merge-key nodes).
• Assign the decoded result (*l = result) so dependency lists written as a sequence of package names are no longer dropped.
• Add deps_test.go covering alias/merge resolution, precedence, runtime lists, the sequence form, and merged-vs-direct source-map attribution.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.

File	Description
deps.go	Reworks PackageConstraints/PackageDependencyList unmarshalling to decode through the reference-reader decoder for anchor/merge-key resolution, skips merge-key nodes during source-map refinement, and fixes the unassigned sequence-form result.
deps_test.go	New unit tests validating alias-as-value, merge-key-inside-map, dependencies-level merge, explicit-over-merged precedence, runtime aliases, sequence-of-names loading, and source-map attribution.

[invidian]

It's hardly my domain, but patches looks reasonable. I left few comments with test improvements suggestions, but they are pretty optional.

[invidian]

nit: all should probably run in parallel (as default).

[cpuguy83]

Done, added t.Parallel() to the func and both subtests.

[invidian]

Ditto parallel.

[cpuguy83]

Done, all deps tests run in parallel now.

[invidian]

nit: some extra test cases to eliminate few mutants:

diff --git deps_test.go deps_test.go
index 6bbe283c..9806c9ea 100644
--- deps_test.go
+++ deps_test.go
@@ -95,6 +95,55 @@ dependencies:
 	})
 }

+func TestPackageDependencyListSequenceFormSourceMap(t *testing.T) {
+	t.Run("a package listed in sequence form keeps a source map pointing at its list entry", func(t *testing.T) {
+		doc := depsSpecHeader + `
+dependencies:
+  build:
+    - somepkg
+`
+		spec, err := LoadSpec([]byte(doc))
+		assert.NilError(t, err)
+
+		pc := spec.Dependencies.Build["somepkg"]
+		assert.Assert(t, pc._sourceMap != nil)
+		assert.Equal(t, int(pc._sourceMap.pos.Start.Line), lineOf(doc, "- somepkg"))
+	})
+}
+
+func TestPackageDependencyEmptySequence(t *testing.T) {
+	t.Run("an empty dependency sequence leaves the list unset rather than an empty map", func(t *testing.T) {
+		spec := loadDepsSpec(t, `
+dependencies:
+  build: []
+`)
+		assert.Assert(t, spec.Dependencies.Build == nil)
+	})
+}
+
+func TestPackageDependencyDecodeErrors(t *testing.T) {
+	t.Run("a package constraint that cannot be decoded surfaces an error", func(t *testing.T) {
+		err := loadDepsSpecErr(t, `
+dependencies:
+  build:
+    somepkg:
+      version:
+        not: a-list
+`)
+		assert.ErrorContains(t, err, "unmarshal package constraints")
+	})
+
+	t.Run("a non-string package key surfaces an error", func(t *testing.T) {
+		err := loadDepsSpecErr(t, `
+dependencies:
+  build:
+    123:
+      version: [">= 1.0"]
+`)
+		assert.ErrorContains(t, err, "expected string key")
+	})
+}
+
 func TestPackageDependencySourceMaps(t *testing.T) {
 	// Line numbers are derived from the document text below so the assertions
 	// are not tied to a hand-counted offset. `merged-pkg` is declared inside the
@@ -138,13 +187,9 @@ dependencies:
 	})
 }

-// loadDepsSpec loads a spec composed of a minimal valid header plus the given
-// body (typically an anchor and a dependencies block) and fails the test if it
-// cannot be loaded.
-func loadDepsSpec(t *testing.T, body string) *Spec {
-	t.Helper()
-
-	const header = `
+// depsSpecHeader is a minimal valid spec header that the dependency tests prefix
+// to a dependencies block so the document loads.
+const depsSpecHeader = `
 name: t
 version: "1.0"
 revision: "1"
@@ -153,12 +198,28 @@ vendor: t
 license: MIT
 description: t
 `
-	spec, err := LoadSpec([]byte(header + body))
+
+// loadDepsSpec loads a spec composed of a minimal valid header plus the given
+// body (typically an anchor and a dependencies block) and fails the test if it
+// cannot be loaded.
+func loadDepsSpec(t *testing.T, body string) *Spec {
+	t.Helper()
+
+	spec, err := LoadSpec([]byte(depsSpecHeader + body))
 	assert.NilError(t, err)
 	assert.Assert(t, spec.Dependencies != nil)
 	return spec
 }

+// loadDepsSpecErr loads a spec composed of the minimal header plus body and
+// returns the resulting error (expected to be non-nil for the caller).
+func loadDepsSpecErr(t *testing.T, body string) error {
+	t.Helper()
+
+	_, err := LoadSpec([]byte(depsSpecHeader + body))
+	return err
+}
+
 // lineOf returns the 1-based line number of the first line in doc that contains
 // substr, or 0 if it is not found.
 func lineOf(doc, substr string) int {

[cpuguy83]

Added the sequence-form source-map, empty-sequence, and decode-error cases, plus the depsSpecHeader const and loadDepsSpecErr helper. Thanks.

[invidian]

nit: one less variable and small simplification based on mutation tests from Copilot.

diff --git deps.go deps.go
index 66a52900..807324c3 100644
--- deps.go
+++ deps.go
@@ -126,14 +126,15 @@ func (l *PackageDependencyList) UnmarshalYAML(ctx context.Context, node ast.Node
                if !ok {
                        return goerrors.New("expected string key for package dependency")
                }
-               pc, ok := result[key.Value]
-               if !ok || pc._sourceMap == nil {
+               pc := result[key.Value]
+               if pc._sourceMap == nil {
                        continue
                }
-               if pc._sourceMap.pos.Start.Line > int32(key.GetToken().Position.Line) {
-                       pc._sourceMap.pos.Start.Line = int32(key.GetToken().Position.Line)
-                       result[key.Value] = pc
-               }
+               // The value node never precedes its key in the document, so the entry's
+               // start line is always at or below the key line; pulling it up to the key
+               // line is always the wanted result. _sourceMap is a pointer shared with
+               // the map entry, so mutating it in place needs no write-back.
+               pc._sourceMap.pos.Start.Line = int32(key.GetToken().Position.Line)
        }

        *l = result

[cpuguy83]

Adopted the ok-var removal and dropped the redundant write-back since _sourceMap is shared by pointer. Kept the guard though: for an alias used as a value (somepkg: *c) the entry can start above its key line, so pulling up unconditionally would move it off the anchor. Added TestPackageDependencyAliasValueSourceMap to pin that.