fix: four silent-wrong-result bugs in render, build, and network create

CHANGELOG.md

@@ -12,6 +12,22 @@ The agent-ergonomics arc lands across four sequenced PRs:
 **#153** (`trond mcp`) → **#154** (`trond recipe`).
 
 ### Added
+- **`jvm.extra_opts`** — an escape hatch for JVM flags outside the closed
+  heap/GC field set, appended last so they win on any last-flag-wins
+  option. Needed because trond runs the JAR directly and so never reads
+  java-tron's `gradle/java-tron.vmoptions`, which its distribution launcher
+  (`bin/FullNode`) does: `-Dio.netty.allocator.type=pooled` is the live
+  example — java-tron sets it to opt out of netty 4.2's adaptive allocator,
+  and without the hatch a trond-deployed node silently runs on the
+  allocator upstream deliberately avoided. Restricted to `-D<key>=<value>`
+  and `-XX:…`; whitespace and quoting characters are refused, so one entry
+  is always exactly one argument.
+- `apply.Options.SkipMonitoring` suppresses the per-node monitoring stack
+  while leaving `Intent.Monitoring` intact for rendering. `network create`
+  needs both halves: `RenderHOCON` keys its metrics auto-enable off the
+  field, but the network owns one stack scraping every node — without the
+  flag each node deployed its own Prometheus and the last one won, leaving
+  a stack that looks healthy while observing a fraction of the network.
 - **Agent integration arc (ai-ops): machine-observable, provably-private rigs.**
   - (#190/#193/#196) **Private-net safety gate (C1).** `is_private` is a
     queryable fact in `status`/`list`/`inspect`. A persistent
@@ -192,6 +208,43 @@ The agent-ergonomics arc lands across four sequenced PRs:
   (was a documented TODO before); refuses `/` and empty paths
 
 ### Fixed
+- **`config_overrides` rendered Go syntax, not HOCON.** `hoconValue` fell
+  back to `fmt.%v` for slices and maps, emitting `[map[address:T… voteCount:5000]]`
+  — which no HOCON parser accepts — so every list-valued override was
+  unusable and a multi-witness `genesis.block.witnesses` (the two-SR private
+  net tron-docker documents) could not be expressed in an intent at all. The
+  same function used `fmt.%q` for strings, which emits Go's `\x01` for a
+  control byte rather than JSON's `\u0001`. Both now render through a JSON
+  encoder (HOCON is a JSON superset) with HTML escaping off so URLs survive
+  verbatim. Numbers deliberately stay on `fmt` — `%v` is already
+  JSON-compatible there and routing them through the encoder would change
+  every rendered config for no correctness gain.
+- **`build.revision` labelled the artifact without building it.** The git
+  worktree checkout ran only when `build.patches` was non-empty, so an
+  explicit branch/tag/sha compiled whatever the working tree happened to
+  hold and then stamped the artifact — cache key, manifest, and
+  `status.build_revision` — with the revision that was asked for. Two
+  `trond build --revision <ref>` runs against different refs could hand back
+  byte-identical artifacts under different labels, silently reducing a
+  base-vs-head comparison to comparing an artifact with itself. The
+  worktree now runs whenever an explicit non-HEAD revision is requested.
+  `revision: HEAD` still builds the working tree, dirty edits included —
+  that is the dev inner loop, and the dirty state is already folded into
+  the cache key.
+- **`network create` bypassed `internal/apply.Apply`.** Its hand-rolled
+  render + deploy + state loop had drifted from the core in three ways: it
+  never called `internal/build`, so a node declaring `build:` rendered an
+  empty `image:` and deployed nothing usable — no error, no warning, and a
+  green `config validate`; it hardcoded JDK 17 for JVM arg selection
+  instead of probing the target; and it hardcoded the docker runtime
+  instead of honouring `target.runtime`. Every node now goes through
+  `Apply`, projected to a single-node intent with its own name and hash so
+  idempotency stays per node.
+- **`Apply` did not persist `P2PPort`.** `network add` builds a joining
+  node's peer list from the `P2PPort` of every node in state and skips any
+  entry where it is zero, so a node deployed through `Apply` was invisible
+  as a peer: the late joiner came up with an empty peer list, never
+  connected, and neither command said why.
 - Witness private key inlined into rendered HOCON — typesafe-config does
   not perform `${ENV}` substitution, the literal `${SR_KEY}` was being
   read as a 9-char witness key and the SR shut down with WITNESS_INIT(1)

cmd/network/create.go

@@ -9,7 +9,9 @@ import (
 	"time"
 
 	"github.com/spf13/cobra"
+	"gopkg.in/yaml.v3"
 
+	"github.com/tronprotocol/tron-deployment/internal/apply"
 	"github.com/tronprotocol/tron-deployment/internal/guard"
 	"github.com/tronprotocol/tron-deployment/internal/intent"
 	"github.com/tronprotocol/tron-deployment/internal/output"
@@ -128,74 +130,60 @@ func runCreate(cmd *cobra.Command, args []string) error {
 
 	var deployed []map[string]any
 
-	for i, node := range parsed.Nodes {
-		nodeName := fmt.Sprintf("%s-node%d", parsed.Name, i)
-
-		rendered, err := render.RenderHOCONWithSecrets(templateDir, parsed, &node)
+	// Every node goes through internal/apply.Apply, the same core `trond
+	// apply` uses. This loop used to hand-roll render + Deploy + state,
+	// which quietly diverged from Apply in three ways: it never called
+	// internal/build (so a node with `build:` rendered an empty image:
+	// and deployed nothing usable, with no error), it hardcoded JDK 17
+	// for JVM arg selection instead of probing the target, and it
+	// hardcoded the docker runtime instead of honouring target.runtime.
+	for i := range parsed.Nodes {
+		sub, nodeName, intentHash, err := nodeIntent(parsed, i)
 		if err != nil {
-			return fmt.Errorf("render config for node %d: %w", i, err)
+			return output.NewError("VALIDATION_ERROR", output.ExitValidationError, err.Error())
 		}
-		// Deploy path — needs the real witness key inlined.
-		hocon := rendered.Deployable()
-
-		memGB := render.ParseMemoryGB(node.Resources.Memory)
-		if memGB == 0 {
-			memGB = 16
-		}
-		jvmArgs := render.JVMArgsString(memGB, 17, node.JVM)
-		composeYAML := render.RenderCompose(nodeName, parsed, &node, "", jvmArgs, "")
-
-		opts := runtime.DeployOpts{
-			Name:        nodeName,
-			ConfigData:  []byte(hocon),
-			ComposeData: []byte(composeYAML),
-		}
-
-		rt := runtime.NewDockerRuntime(tgt, workDir)
-		if err := rt.Deploy(cmd.Context(), opts); err != nil {
+		res, err := apply.Apply(cmd.Context(), apply.Options{
+			Intent:         sub,
+			Target:         tgt,
+			Store:          store,
+			State:          deployState,
+			IntentHash:     intentHash,
+			Existing:       store.GetNode(deployState, nodeName),
+			TemplateDir:    templateDir,
+			DeploymentsDir: workDir,
+			IntentPath:     createIntentPath,
+			// Now that create is an Apply caller it inherits the core's
+			// state-based --require-private gate, same as cmd/apply.go.
+			// guard.Enforce above only sees the intent's network LABEL;
+			// this also checks the network recorded in state for a node
+			// already deployed under the same name (#203).
+			RequirePrivate: guard.Requested(),
+			// The network owns one monitoring stack covering every node
+			// (deployNetworkMonitoring below). Intent.Monitoring stays set
+			// so RenderHOCON still auto-enables the node's metrics port.
+			SkipMonitoring: true,
+		})
+		if err != nil {
 			deployed = append(deployed, map[string]any{
 				"name":   nodeName,
-				"type":   node.Type,
+				"type":   parsed.Nodes[i].Type,
 				"status": "error",
 				"error":  err.Error(),
 			})
 			continue
 		}
 
-		// Capture the (post-defaults, post-auto-allocation) ports in state
-		// so inspect / health / diagnose / events can target the right
-		// host endpoint without re-reading the intent file.
-		mn := state.ManagedNode{
-			Name:    nodeName,
-			Version: node.Version,
-			Network: parsed.Network,
-			Target: state.NodeTarget{
-				Type:         parsed.Target.Type,
-				Host:         parsed.Target.Host,
-				User:         parsed.Target.User,
-				Port:         parsed.Target.Port,
-				IdentityFile: parsed.Target.IdentityFile,
-			},
-			Runtime:     "docker",
-			Status:      "running",
-			LastApplied: time.Now().UTC(),
-			HTTPPort:    node.Ports.HTTP,
-			GRPCPort:    node.Ports.GRPC,
-			P2PPort:     node.Ports.P2P,
-			MetricsPort: node.Ports.Metrics,
-			Labels:      node.Labels,
+		entry := map[string]any{
+			"name":      nodeName,
+			"type":      parsed.Nodes[i].Type,
+			"status":    "running",
+			"outcome":   res.Outcome,
+			"endpoints": res.Endpoints,
 		}
-		store.UpsertNode(deployState, mn)
-
-		deployed = append(deployed, map[string]any{
-			"name":   nodeName,
-			"type":   node.Type,
-			"status": "running",
-			"endpoints": map[string]string{
-				"http": fmt.Sprintf("http://127.0.0.1:%d", node.Ports.HTTP),
-				"grpc": fmt.Sprintf("127.0.0.1:%d", node.Ports.GRPC),
-			},
-		})
+		if res.Build != nil {
+			entry["build"] = res.Build
+		}
+		deployed = append(deployed, entry)
 	}
 
 	result := map[string]any{
@@ -353,3 +341,35 @@ type monitoringResult struct {
 	error string
 	urls  map[string]string
 }
+
+// nodeIntent projects the multi-node network intent down to the
+// single-node intent apply.Apply consumes, returning it alongside the
+// node's deployed name and its own intent hash.
+//
+// Apply keys everything off Intent.Name — the compose project, the state
+// entry, Result.Name — and reads only Intent.Nodes[0], so the projection
+// has to rename as well as slice. Everything else (target, network,
+// monitoring, template dir) is shared and copied through unchanged.
+//
+// The hash is computed over the projected intent rather than over the
+// network intent file, so idempotency is per node: editing one node's
+// ports must redeploy that node and leave its siblings at no_change.
+func nodeIntent(parsed *intent.Intent, i int) (sub *intent.Intent, name, hash string, err error) {
+	if i < 0 || i >= len(parsed.Nodes) {
+		return nil, "", "", fmt.Errorf("node index %d out of range (%d nodes)", i, len(parsed.Nodes))
+	}
+	name = fmt.Sprintf("%s-node%d", parsed.Name, i)
+
+	// Shallow struct copy is deliberate: the shared fields (Target,
+	// Monitoring, ...) are read-only from here on, and Apply must see the
+	// same values every node saw. Only Name and Nodes are re-pointed.
+	clone := *parsed
+	clone.Name = name
+	clone.Nodes = []intent.NodeSpec{parsed.Nodes[i]}
+
+	data, err := yaml.Marshal(&clone)
+	if err != nil {
+		return nil, "", "", fmt.Errorf("hash node %d intent: %w", i, err)
+	}
+	return &clone, name, apply.IntentHashFromBytes(data), nil
+}

cmd/network/node_intent_test.go

@@ -0,0 +1,161 @@
+package network
+
+import (
+	"testing"
+
+	"github.com/tronprotocol/tron-deployment/internal/intent"
+)
+
+func twoNodeIntent() *intent.Intent {
+	return &intent.Intent{
+		Name:    "twosr",
+		Network: "private",
+		Target:  intent.Target{Type: "local", Runtime: "docker"},
+		Monitoring: &intent.Monitoring{
+			Enabled: intent.BoolPtr(true),
+		},
+		Nodes: []intent.NodeSpec{
+			{Type: "witness", Version: "latest", Ports: intent.PortMapping{HTTP: 8090, P2P: 18888}},
+			{Type: "fullnode", Version: "latest", Ports: intent.PortMapping{HTTP: 8091, P2P: 18889}},
+		},
+	}
+}
+
+// TestNodeIntent_ProjectsOneNodeAndRenames covers the two things Apply keys
+// off: it reads only Nodes[0], and it uses Intent.Name for the compose
+// project, the state entry and Result.Name. A projection that sliced without
+// renaming would deploy every node under the network's own name and each
+// would overwrite the previous one's state entry.
+func TestNodeIntent_ProjectsOneNodeAndRenames(t *testing.T) {
+	parsed := twoNodeIntent()
+
+	for i, wantName := range []string{"twosr-node0", "twosr-node1"} {
+		sub, name, hash, err := nodeIntent(parsed, i)
+		if err != nil {
+			t.Fatalf("nodeIntent(%d): %v", i, err)
+		}
+		if name != wantName {
+			t.Errorf("name = %q, want %q", name, wantName)
+		}
+		if sub.Name != wantName {
+			t.Errorf("sub.Name = %q, want %q — Apply names the deployment from this", sub.Name, wantName)
+		}
+		if len(sub.Nodes) != 1 {
+			t.Fatalf("sub.Nodes has %d entries, want exactly 1 (Apply only reads Nodes[0])", len(sub.Nodes))
+		}
+		if sub.Nodes[0].Type != parsed.Nodes[i].Type {
+			t.Errorf("sub carries node %q, want %q", sub.Nodes[0].Type, parsed.Nodes[i].Type)
+		}
+		if hash == "" {
+			t.Error("empty intent hash — Apply rejects that in validateOptions")
+		}
+
+		// Shared context must survive the projection: these drive target
+		// resolution, the HOCON template choice and the metrics auto-enable.
+		if sub.Network != parsed.Network {
+			t.Errorf("sub.Network = %q, want %q", sub.Network, parsed.Network)
+		}
+		if sub.Target != parsed.Target {
+			t.Errorf("sub.Target = %+v, want %+v", sub.Target, parsed.Target)
+		}
+		if sub.Monitoring != parsed.Monitoring {
+			t.Error("sub lost Monitoring — RenderHOCON keys the metrics auto-enable off it")
+		}
+	}
+}
+
+// TestNodeIntent_HashIsPerNode is the idempotency contract. Apply short
+// circuits to no_change when the incoming hash equals the stored one, so a
+// hash shared across nodes would make node 1 look unchanged the moment node 0
+// had been applied — the whole network would deploy once and then go quiet.
+func TestNodeIntent_HashIsPerNode(t *testing.T) {
+	parsed := twoNodeIntent()
+
+	_, _, h0, err := nodeIntent(parsed, 0)
+	if err != nil {
+		t.Fatal(err)
+	}
+	_, _, h1, err := nodeIntent(parsed, 1)
+	if err != nil {
+		t.Fatal(err)
+	}
+	if h0 == h1 {
+		t.Error("both nodes hashed identically; per-node idempotency would collapse")
+	}
+}
+
+// TestNodeIntent_HashIsStable — same input, same hash. Without this a
+// re-run of `network create` reports every node as updated forever.
+func TestNodeIntent_HashIsStable(t *testing.T) {
+	_, _, a, err := nodeIntent(twoNodeIntent(), 0)
+	if err != nil {
+		t.Fatal(err)
+	}
+	_, _, b, err := nodeIntent(twoNodeIntent(), 0)
+	if err != nil {
+		t.Fatal(err)
+	}
+	if a != b {
+		t.Errorf("hash not stable across calls: %s vs %s", a, b)
+	}
+}
+
+// TestNodeIntent_HashTracksThatNodeOnly — editing node 1 must not disturb
+// node 0's hash, otherwise a one-node change redeploys the whole network.
+func TestNodeIntent_HashTracksThatNodeOnly(t *testing.T) {
+	before := twoNodeIntent()
+	_, _, h0Before, err := nodeIntent(before, 0)
+	if err != nil {
+		t.Fatal(err)
+	}
+
+	after := twoNodeIntent()
+	after.Nodes[1].Ports.HTTP = 19999
+	_, _, h0After, err := nodeIntent(after, 0)
+	if err != nil {
+		t.Fatal(err)
+	}
+	_, _, h1After, err := nodeIntent(after, 1)
+	if err != nil {
+		t.Fatal(err)
+	}
+
+	if h0Before != h0After {
+		t.Error("changing node 1 changed node 0's hash — a single-node edit would redeploy siblings")
+	}
+	_, _, h1Before, err := nodeIntent(before, 1)
+	if err != nil {
+		t.Fatal(err)
+	}
+	if h1Before == h1After {
+		t.Error("changing node 1's port did not change its hash — the edit would deploy as no_change")
+	}
+}
+
+// TestNodeIntent_DoesNotMutateSource guards the shallow copy: runCreate calls
+// this once per node off the same parsed intent, so a projection that wrote
+// through to the source would corrupt every later node.
+func TestNodeIntent_DoesNotMutateSource(t *testing.T) {
+	parsed := twoNodeIntent()
+	origName := parsed.Name
+	origCount := len(parsed.Nodes)
+
+	if _, _, _, err := nodeIntent(parsed, 0); err != nil {
+		t.Fatal(err)
+	}
+	if parsed.Name != origName {
+		t.Errorf("source intent renamed to %q", parsed.Name)
+	}
+	if len(parsed.Nodes) != origCount {
+		t.Errorf("source node list resized to %d, want %d", len(parsed.Nodes), origCount)
+	}
+}
+
+func TestNodeIntent_OutOfRange(t *testing.T) {
+	parsed := twoNodeIntent()
+	for _, i := range []int{-1, 2, 99} {
+		if _, _, _, err := nodeIntent(parsed, i); err == nil {
+			t.Errorf("index %d: want an error, got nil", i)
+		}
+	}
+}