diff --git a/internal/httpapi/pct_create_test.go b/internal/httpapi/pct_create_test.go index 3391a85..3a7151a 100644 --- a/internal/httpapi/pct_create_test.go +++ b/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) diff --git a/internal/httpapi/phase3.go b/internal/httpapi/phase3.go index 011386c..14a5b89 100644 --- a/internal/httpapi/phase3.go +++ b/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 diff --git a/internal/mcp/server.go b/internal/mcp/server.go index 3b33974..0d239c4 100644 --- a/internal/mcp/server.go +++ b/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 {