Fix Marshal mutating *Node input #371

[truffle-dev]

Fixes #371.

Marshal is documented to treat its input as read-only, but passing a
*Node leaves the caller's tree with every resolvable tag stripped:

var node yaml.Node
yaml.Unmarshal([]byte("type: array\nlimit: 5\n"), &node)
yaml.Marshal(&node)
// node.Content[0].Tag is now "" (was "!!map"), and so on for the
// !!str / !!int children.

Cause

The representer hands a user-supplied *Node straight into the rest of
the dump pipeline: Represent returns a document node as-is, and
nodev returns nested *Node values as-is. The desolver then clears
inferable tags (!!str, !!int, !!map, ...) in place to keep the
output clean, so the mutation lands on the caller's own node.

Fix

Deep-copy the user-supplied node in the representer before it enters the
pipeline. A deepCopyNode helper copies Content, Alias, and
Stream, using a seen map so shared anchors keep their identity and
cyclic alias graphs don't recurse forever. It's wired at both *Node
entry points: the document-node branch in Represent and nodev (which
also covers *Node values nested inside a struct, map, or slice).

Only *Node inputs pay the copy cost; representer-built trees (from
maps, structs, slices) are already fresh and untouched.

Verification

• go test . -run TestMarshalDoesNotMutateNode passes with the fix and
  fails without it (tags wiped on the input node).
• go test ./...: main, internal, and cmd suites green; the
  yaml-test-suite results are unchanged at 1383 pass / 225 known-fail.

[Copilot]

Pull request overview

Fixes a mutation bug where yaml.Marshal (and related encode paths) modified caller-supplied *yaml.Node trees in-place by ensuring user-provided nodes are deep-copied before entering the dump pipeline.

Changes:

• Add a regression test asserting yaml.Marshal(&node) does not mutate the input node tree.
• Introduce deepCopyNode and use it for both document-node inputs (Represent) and nested node values (nodev) so downstream stages (notably the desolver) never touch caller-owned nodes.

Reviewed changes

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

File	Description
node_test.go	Adds TestMarshalDoesNotMutateNode to prevent regressions where Marshal strips resolved tags (and other fields) from the input tree.
internal/libyaml/representer.go	Deep-copies user-supplied nodes at representer entry points (document nodes and nested node values) via a cycle-safe, identity-preserving deepCopyNode.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[ccoVeille]

I have asked Copilot to review in a separate agent discussion here is what was reported

I reviewed PR **#375**’s file changes and here are the likely remaining edge cases / risks:

### What looks solid
- Deep copy is applied at both `*Node` ingress points:
  - `Represent` when input is already a `DocumentNode`
  - `nodev` for nested `*Node` inside structs/maps/slices
- `seen map[*Node]*Node` correctly preserves shared identity and prevents infinite recursion on cycles/aliases.
- Test `TestMarshalDoesNotMutateNode` covers the core regression.

### Potential edge cases / risks

1) **`Stream.Encoding` copy style**
- In `deepCopyNode`, `s := &Stream{Encoding: n.Stream.Encoding}`.
- If `Encoding` is a pointer/slice/map/interface-with-pointer-semantics (not sure without `Stream` type definition), this may still be shallow for that field.
- If it’s scalar/enum, no issue.

2) **Line/Column copied even though representer-built trees might ignore them**
- Copy includes `Line`/`Column`. Usually harmless.
- But if any downstream behavior unexpectedly depends on these positions for represented nodes, copying preserves original positions (probably desired, but worth noting).

3) **Performance on very large node graphs**
- This is O(N) memory/time per marshal for `*Node` inputs.
- Acceptable for correctness, but workloads marshaling huge prebuilt node graphs repeatedly may notice overhead.
- A benchmark would be useful (`Marshal(*Node)` before/after with large anchors/aliases).

4) **Test coverage breadth**
- Current test validates tag preservation on a simple map/int/string shape.
- Missing targeted tests for:
  - alias cycles
  - shared subtrees (same node referenced twice)
  - comments/style preservation after marshal
  - nested `*Node` inside struct/map/slice path (the `nodev` hook)

5) **Semantic expectation: identity no longer preserved through marshal pipeline**
- Previously (buggy) behavior mutated original nodes.
- Now marshal operates on clone, so any internal side-effect expectations (if anyone relied on them) disappear.
- That is the correct API behavior, but could surprise anyone depending on old side effects.

### Overall assessment
The fix direction is correct and likely robust for the reported bug (#371).  
The main things I’d still add are **2–4 focused tests** (aliases/shared nodes/nested `*Node`) plus an optional micro-benchmark.