fix: build execution result JSON via json.Marshal (was stuck at 'approved')

The final "UPDATE executions SET result=$::jsonb" built its payload with
fmt.Sprintf and only escaped newlines. apt/pct output contains quotes,
backslashes and control chars, so the payload was invalid JSON, the jsonb
cast failed, and the (unchecked) UPDATE was silently discarded — the
execution stayed 'approved' with a NULL result even though the LXC was fully
provisioned (verified live: vmid auto-assigned, container running, service
installed, post_install ran).

- executeApprovedAction: marshal result via json.Marshal; log UPDATE errors
- add jsonErr() helper; route all pct_create failure-path results through it
- mcp/server.go: add jsonOut() for restart/systemctl/pct_exec inline results
- regression test for JSON validity on quote/backslash/control-char output

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

internal/httpapi/pct_create_test.go

@@ -68,6 +68,25 @@ func TestResolveTemplate(t *testing.T) {
 	}
 }
 
+// TestJSONErrValidForNastyOutput guards the bug where command output with
+// quotes/backslashes/newlines produced invalid JSON, failing the ::jsonb cast
+// and silently dropping the execution's final status update.
+func TestJSONErrValidForNastyOutput(t *testing.T) {
+	nasty := "CT 132 already exists on node \"hubris\"\n\tpath C:\\x\r\n\x00 100%"
+	for _, payload := range [][]byte{
+		jsonErr("%s", nasty),
+		jsonErr("list templates on %s: %s", "host:strong", nasty),
+	} {
+		var m map[string]any
+		if err := json.Unmarshal(payload, &m); err != nil {
+			t.Fatalf("jsonErr produced invalid JSON: %v\npayload=%s", err, payload)
+		}
+		if _, ok := m["error"]; !ok {
+			t.Errorf("missing error key: %s", payload)
+		}
+	}
+}
+
 func TestSanitizePkgs(t *testing.T) {
 	in := []string{"docker.io", "git", "rm -rf /", "curl;wget", "python3-pip", ""}
 	got := sanitizePkgs(in)

internal/httpapi/phase3.go

@@ -169,7 +169,7 @@ func executeApprovedAction(ctx context.Context, pool *db.Pool, execID uuid.UUID,
 	if err != nil {
 		slog.Error("httpapi: resolve host for approved execution", "error", err, "target", targetSlug)
 		pool.Exec(ctx, `UPDATE executions SET status='failed', result=$2::jsonb WHERE entity_id=$1`,
-			execID, fmt.Sprintf(`{"error":"%s"}`, err.Error()))
+			execID, jsonErr("%s", err.Error()))
 		emitExecutionEvent(ctx, pool, execID, "failed", map[string]any{"target": targetSlug, "error": err.Error()})
 		return
 	}
@@ -226,7 +226,7 @@ func executeApprovedAction(ctx context.Context, pool *db.Pool, execID uuid.UUID,
 		if err := json.Unmarshal([]byte(params), &cfg); err != nil {
 			slog.Error("httpapi: pct_create parse params", "error", err, "params", params)
 			pool.Exec(ctx, `UPDATE executions SET status='failed', result=$2::jsonb WHERE entity_id=$1`,
-				execID, fmt.Sprintf(`{"error":"invalid pct_create params: %v"}`, err))
+				execID, jsonErr("invalid pct_create params: %v", err))
 			emitExecutionEvent(ctx, pool, execID, "failed", map[string]any{"target": targetSlug, "error": err.Error()})
 			return
 		}
@@ -274,7 +274,7 @@ func executeApprovedAction(ctx context.Context, pool *db.Pool, execID uuid.UUID,
 		}
 		if tplErr != nil {
 			pool.Exec(ctx, `UPDATE executions SET status='failed', result=$2::jsonb WHERE entity_id=$1`,
-				execID, fmt.Sprintf(`{"error":"list templates on %s: %s"}`, targetSlug, strings.ReplaceAll(tplErr.Error(), `"`, `'`)))
+				execID, jsonErr("list templates on %s: %s", targetSlug, tplErr.Error()))
 			emitExecutionEvent(ctx, pool, execID, "failed", map[string]any{"target": targetSlug, "error": tplErr.Error()})
 			return
 		}
@@ -282,7 +282,7 @@ func executeApprovedAction(ctx context.Context, pool *db.Pool, execID uuid.UUID,
 		if cfg.Template == "" {
 			msg := fmt.Sprintf("no usable LXC template on %s. Available: %v", targetSlug, available)
 			pool.Exec(ctx, `UPDATE executions SET status='failed', result=$2::jsonb WHERE entity_id=$1`,
-				execID, fmt.Sprintf(`{"error":%q}`, msg))
+				execID, jsonErr("%s", msg))
 			emitExecutionEvent(ctx, pool, execID, "failed", map[string]any{"target": targetSlug, "error": msg})
 			return
 		}
@@ -306,7 +306,7 @@ func executeApprovedAction(ctx context.Context, pool *db.Pool, execID uuid.UUID,
 			if nerr != nil || cerr != nil || nextID == 0 {
 				msg := fmt.Sprintf("VMID %d is already in use on the cluster and could not resolve a free id", cfg.VMID)
 				pool.Exec(ctx, `UPDATE executions SET status='failed', result=$2::jsonb WHERE entity_id=$1`,
-					execID, fmt.Sprintf(`{"error":%q}`, msg))
+					execID, jsonErr("%s", msg))
 				emitExecutionEvent(ctx, pool, execID, "failed", map[string]any{"target": targetSlug, "error": msg})
 				return
 			}
@@ -429,22 +429,30 @@ func executeApprovedAction(ctx context.Context, pool *db.Pool, execID uuid.UUID,
 	default:
 		slog.Error("httpapi: unknown gated action for approved execution", "action", action, "execution_id", execID)
 		pool.Exec(ctx, `UPDATE executions SET status='failed', result=$2::jsonb WHERE entity_id=$1`,
-			execID, fmt.Sprintf(`{"error":"unknown action: %s"}`, action))
+			execID, jsonErr("unknown action: %s", action))
 		return
 	}
 
 	durationMs := int(time.Since(startedAt).Milliseconds())
-	result := fmt.Sprintf(`{"output":"%s"}`, strings.ReplaceAll(output, "\n", "\\n"))
 	status := "completed"
 	verified := true
+	// Build result via json.Marshal, not string interpolation. Command output
+	// (apt/pct) contains quotes, backslashes and control chars; the old
+	// fmt.Sprintf only escaped "\n", producing invalid JSON that failed the
+	// ::jsonb cast — so this UPDATE was silently discarded and the execution
+	// was stuck at "approved" forever even though provisioning succeeded.
+	resMap := map[string]any{"output": output}
 	if err != nil {
-		result = fmt.Sprintf(`{"output":"%s","error":"%s"}`, strings.ReplaceAll(output, "\n", "\\n"), err.Error())
+		resMap["error"] = err.Error()
 		status = "failed"
 		verified = false
 	}
+	resultJSON, _ := json.Marshal(resMap)
 
-	pool.Exec(ctx, `UPDATE executions SET status=$2, result=$3::jsonb, duration_ms=$4, verified=$5, started_at=$6, completed_at=$7 WHERE entity_id=$1`,
-		execID, status, result, durationMs, verified, startedAt, time.Now())
+	if _, uerr := pool.Exec(ctx, `UPDATE executions SET status=$2, result=$3::jsonb, duration_ms=$4, verified=$5, started_at=$6, completed_at=$7 WHERE entity_id=$1`,
+		execID, status, resultJSON, durationMs, verified, startedAt, time.Now()); uerr != nil {
+		slog.Error("httpapi: finalize execution status", "error", uerr, "execution_id", execID, "intended_status", status)
+	}
 
 	emitExecutionEvent(ctx, pool, execID, status, map[string]any{
 		"action": action, "target": targetSlug, "duration_ms": durationMs,
@@ -454,6 +462,15 @@ func executeApprovedAction(ctx context.Context, pool *db.Pool, execID uuid.UUID,
 		"execution_id", execID, "action", action, "status", status, "duration_ms", durationMs)
 }
 
+// jsonErr builds a valid {"error": "..."} JSON payload for an execution's
+// result column. Always use this instead of fmt.Sprintf'ing JSON by hand —
+// error text and command output routinely contain quotes/backslashes that
+// break a hand-built string and fail the ::jsonb cast.
+func jsonErr(format string, args ...any) []byte {
+	b, _ := json.Marshal(map[string]any{"error": fmt.Sprintf(format, args...)})
+	return b
+}
+
 // resolveTemplate maps a requested template name to one actually present in
 // the host's template cache. Exact match wins; a bare distro hint (e.g.
 // "debian-13" or "debian") matches by prefix; empty picks the newest debian

internal/mcp/server.go

@@ -332,7 +332,7 @@ func newServer(pool *db.Pool, agentID uuid.UUID) *mcp.Server {
 				result = fmt.Sprintf("restart %s: ERROR %v", svc, err)
 			}
 			pool.Exec(ctx, `UPDATE executions SET status='completed', result=$2::jsonb WHERE entity_id=$1`,
-				id, fmt.Sprintf(`{"output":"%s"}`, strings.ReplaceAll(out, "\n", "\\n")))
+				id, jsonOut(out))
 			return textResult(result), nil
 
 		case "systemctl":
@@ -353,7 +353,7 @@ func newServer(pool *db.Pool, agentID uuid.UUID) *mcp.Server {
 				result = fmt.Sprintf("systemctl %s %s: ERROR %v", params, svc, err)
 			}
 			pool.Exec(ctx, `UPDATE executions SET status='completed', result=$2::jsonb WHERE entity_id=$1`,
-				id, fmt.Sprintf(`{"output":"%s"}`, strings.ReplaceAll(out, "\n", "\\n")))
+				id, jsonOut(out))
 			return textResult(result), nil
 
 		case "pct_exec":
@@ -377,7 +377,7 @@ func newServer(pool *db.Pool, agentID uuid.UUID) *mcp.Server {
 				result = fmt.Sprintf("pct exec %s: ERROR %v", pveID, err)
 			}
 			pool.Exec(ctx, `UPDATE executions SET status='completed', result=$2::jsonb WHERE entity_id=$1`,
-				id, fmt.Sprintf(`{"output":"%s"}`, strings.ReplaceAll(out, "\n", "\\n")))
+				id, jsonOut(out))
 			return textResult(result), nil
 
 		case "apt_upgrade":
@@ -872,6 +872,15 @@ func textResult(s string) *mcp.CallToolResult {
 	}
 }
 
+// jsonOut builds a valid {"output": "..."} JSON payload for an execution's
+// result column. Command output contains quotes/backslashes/control chars, so
+// it must be JSON-marshaled — a hand-built string fails the ::jsonb cast and
+// silently drops the status update, leaving the execution stuck.
+func jsonOut(out string) []byte {
+	b, _ := json.Marshal(map[string]any{"output": out})
+	return b
+}
+
 func queryEntity(ctx context.Context, pool *db.Pool, idOrSlug string) *mcp.CallToolResult {
 	var id uuid.UUID
 	if u, err := uuid.Parse(idOrSlug); err == nil {