fix: deploy hygiene — validate before persist, filter env, honest badge, real hostname (CashPilot-Desktop-ada) (#99)

Four pre-existing latents from the PR #96 review:

1. DeployService persisted credentials BEFORE validation, so a rejected deploy
   left invalid/blank creds lingering (and lighting the 'Configured' badge for
   a service that could never start). Validate via the new
   Manager.ValidateCredentials first; persist only if it passes.
2. buildEnv copied every stored-cred key into the container env unfiltered.
   Restrict to catalog-declared keys so an orphaned pre-migration key can't
   leak into a container. (The native twin keeps its documented passthrough.)
3. The 'Configured' badge was len(creds)>0, so an orphaned old-key blob read as
   configured though a deploy would fail. Key it off the current required fields
   via Manager.RequiredCredentialsMet.
4. An unedited deploy form resubmitted the raw 'cashpilot-{hostname}' default as
   an override, which buildEnv only substituted on defaults (and to the literal
   'desktop') — producing a device literally named cashpilot-{hostname}. Expand
   {hostname} to the real host on defaults AND overrides (both runtimes), and
   surface AppState.hostname so the form renders the real value.

Tests: buildEnv filter+hostname, ValidateCredentials/RequiredCredentialsMet,
DeployService validate-before-persist. go test -race + vet + gofmt clean, tsc clean.

Author: GeiserX

app.go

@@ -223,6 +223,10 @@ type AppState struct {
 	// keyed by slug (e.g. the MystNodes per-node earnings breakdown). The frontend
 	// parses the raw JSON per service; the backend stores and forwards it opaquely.
 	ServiceDetails map[string]string `json:"serviceDetails"`
+	// Hostname is this machine's name, so a deploy form can render a {hostname}-defaulted
+	// field with the real value the deploy path will substitute (instead of the literal
+	// "cashpilot-{hostname}" the raw catalog default would otherwise show and submit).
+	Hostname string `json:"hostname"`
 }
 
 type Notification struct {
@@ -374,6 +378,7 @@ func (a *App) GetAppState() (AppState, error) {
 		Summary:          a.computeEarningsSummary(earnings),
 		Health:           a.store.HealthScores(7),
 		ServiceDetails:   a.store.ListServiceDetails(),
+		Hostname:         runtime.DeviceHostname(),
 	}, nil
 }
 
@@ -652,7 +657,7 @@ func (a *App) GetSettingsState() (SettingsState, error) {
 		collectors = append(collectors, CollectorSetting{
 			Slug:       svc.Slug,
 			Name:       svc.Name,
-			Configured: len(creds) > 0,
+			Configured: a.credsConfigured(svc.Slug, creds),
 			Collector:  svc.Collector.Type,
 		})
 	}
@@ -880,15 +885,28 @@ func (a *App) DeployService(slug string, values map[string]string) (store.Deploy
 	if err := a.ready(); err != nil {
 		return store.Deployment{}, err
 	}
-	if len(values) > 0 {
-		if err := a.store.SaveCredentials(slug, values); err != nil {
+	// The credentials this deploy will use: the submitted values (which replace the
+	// stored blob) when provided, else whatever is already stored.
+	creds := values
+	if len(creds) == 0 {
+		stored, err := a.store.GetCredentials(slug)
+		if err != nil {
 			return store.Deployment{}, err
 		}
+		creds = stored
 	}
-	creds, err := a.store.GetCredentials(slug)
-	if err != nil {
+	// Validate BEFORE persisting, so a rejected deploy never leaves invalid/blank creds
+	// lingering — they could never deploy, yet they would linger and light the
+	// "Configured" badge for a service that cannot actually start.
+	if err := a.services.ValidateCredentials(slug, creds); err != nil {
+		a.emitError("deploy", err)
 		return store.Deployment{}, err
 	}
+	if len(values) > 0 {
+		if err := a.store.SaveCredentials(slug, values); err != nil {
+			return store.Deployment{}, err
+		}
+	}
 	deployment, err := a.services.Deploy(a.ctx, slug, creds)
 	if err != nil {
 		a.emitError("deploy", err)
@@ -1344,6 +1362,21 @@ func hostnameOrDefault(value string) string {
 	return value
 }
 
+// credsConfigured reports whether a service should show as "Configured" in Settings:
+// it must have stored credentials AND those credentials must satisfy the catalog's
+// current required fields. Keying off required fields (not merely len(creds) > 0) stops
+// an orphaned pre-migration blob — non-empty but missing the current keys — from
+// falsely reading as configured when a deploy would in fact fail until re-entry.
+func (a *App) credsConfigured(slug string, creds map[string]string) bool {
+	if len(creds) == 0 {
+		return false
+	}
+	if a.services == nil {
+		return true
+	}
+	return a.services.RequiredCredentialsMet(slug, creds)
+}
+
 // ensureFleetAPIKey loads the fleet bearer token into memory at startup, keeping it
 // out of config.json. It prefers the OS keychain (with a 0600 file fallback) via
 // config.FleetKey/SetFleetKey. A legacy plaintext token still in config.json is

app_test.go

@@ -18,6 +18,7 @@ import (
 	"github.com/GeiserX/CashPilot-Desktop/internal/config"
 	"github.com/GeiserX/CashPilot-Desktop/internal/exchange"
 	"github.com/GeiserX/CashPilot-Desktop/internal/runtime"
+	"github.com/GeiserX/CashPilot-Desktop/internal/services"
 	"github.com/GeiserX/CashPilot-Desktop/internal/store"
 )
 
@@ -662,3 +663,50 @@ func TestNotificationsFlagsOutdated(t *testing.T) {
 		t.Fatalf("notifications() did not include an 'update available' warning: %+v", items)
 	}
 }
+
+// TestDeployServiceValidatesBeforePersist covers CashPilot-Desktop-ada fix 1: a deploy
+// whose credentials fail validation must be rejected WITHOUT persisting the invalid blob
+// (the pre-fix flow saved first, leaving lingering creds that lit the "Configured" badge
+// for a service that could never actually deploy).
+func TestDeployServiceValidatesBeforePersist(t *testing.T) {
+	t.Setenv("CASHPILOT_DESKTOP_DATA_DIR", t.TempDir())
+	cfg, err := config.NewManager()
+	if err != nil {
+		t.Fatalf("config.NewManager error: %v", err)
+	}
+	st, err := store.Open(cfg.DataDir())
+	if err != nil {
+		t.Fatalf("store.Open error: %v", err)
+	}
+	t.Cleanup(func() { _ = st.Close() })
+
+	cat, err := catalog.LoadEmbedded(fstest.MapFS{
+		"services/bandwidth/req.yml": {Data: []byte(
+			"name: Req\nslug: req\ncategory: bandwidth\nstatus: active\ndocker:\n  image: req/image:1.0.0\n  env:\n    - key: TOKEN\n      label: Token\n      required: true\n")},
+	})
+	if err != nil {
+		t.Fatalf("catalog.LoadEmbedded error: %v", err)
+	}
+
+	provider := runtime.NewDockerProvider()
+	app := &App{
+		cfg:      cfg,
+		store:    st,
+		catalog:  cat,
+		runtime:  provider,
+		services: services.NewManager(provider, cat, st),
+		ctx:      context.Background(),
+	}
+
+	// Non-empty but invalid (missing the required TOKEN): the runtime is never reached.
+	if _, err := app.DeployService("req", map[string]string{"WRONG": "x"}); err == nil {
+		t.Fatal("expected a validation error for the missing required field")
+	}
+	got, err := st.GetCredentials("req")
+	if err != nil {
+		t.Fatalf("GetCredentials error: %v", err)
+	}
+	if len(got) != 0 {
+		t.Fatalf("a rejected deploy must not persist creds, got %v", got)
+	}
+}

frontend/src/main.ts

@@ -1260,7 +1260,7 @@ function renderWizardServiceSetup(service: Service) {
         ${fields.map((item) => `
           <label>
             <span>${escapeHtml(item.label)}${item.required ? " *" : ""}</span>
-            <input data-wizard-env="${item.key}" type="${item.secret ? "password" : "text"}" placeholder="${escapeHtml(item.description)}" value="${escapeHtml(item.default || "")}" />
+            <input data-wizard-env="${item.key}" type="${item.secret ? "password" : "text"}" placeholder="${escapeHtml(item.description)}" value="${escapeHtml((item.default || "").replaceAll("{hostname}", state?.hostname || "desktop"))}" />
           </label>
         `).join("") || `<p class="muted">No credentials are required by the catalog for this service.</p>`}
       </div>

frontend/src/wails.d.ts

@@ -42,6 +42,9 @@ export interface AppState {
   currencies: string[];
   summary: EarningsSummary;
   serviceDetails: Record<string, string> | null;
+  // This machine's hostname, so a {hostname}-defaulted deploy field renders the real
+  // value the deploy path will substitute rather than the literal "{hostname}".
+  hostname: string;
 }
 
 // MystNode mirrors the Go mystNode struct: one Mysterium node's per-node

internal/runtime/native.go

@@ -690,14 +690,17 @@ func buildNativeEnv(svc catalog.Service, overrides map[string]string) map[string
 	env := make(map[string]string)
 	for _, item := range svc.Native.Env {
 		if item.Default != "" {
-			env[item.Key] = strings.ReplaceAll(item.Default, "{hostname}", "desktop")
+			env[item.Key] = item.Default
 		}
 	}
 	for key, value := range overrides {
 		env[key] = value
 	}
+	// Expand {hostname} to the real hostname on both defaults and overrides (an unedited
+	// form resubmits the raw default), matching the Docker buildEnv path.
+	hostname := DeviceHostname()
 	for key, value := range env {
-		env[key] = substitute(value, env)
+		env[key] = substitute(strings.ReplaceAll(value, "{hostname}", hostname), env)
 	}
 	return env
 }

internal/runtime/runtime.go

@@ -8,6 +8,7 @@ import (
 	"fmt"
 	"io"
 	"math"
+	"os"
 	"os/exec"
 	"regexp"
 	goruntime "runtime"
@@ -543,18 +544,39 @@ func containerName(slug string) string {
 	return "cashpilot-" + slug
 }
 
+// DeviceHostname returns this machine's hostname for {hostname} expansion, falling
+// back to "desktop" when the OS can't report one. Exported so the app layer can offer
+// the same value as the form default the deploy path will substitute.
+func DeviceHostname() string {
+	if h, err := os.Hostname(); err == nil && h != "" {
+		return h
+	}
+	return "desktop"
+}
+
 func buildEnv(svc catalog.Service, overrides map[string]string) map[string]string {
 	env := make(map[string]string)
+	declared := make(map[string]bool, len(svc.Docker.Env))
 	for _, item := range svc.Docker.Env {
+		declared[item.Key] = true
 		if item.Default != "" {
-			env[item.Key] = strings.ReplaceAll(item.Default, "{hostname}", "desktop")
+			env[item.Key] = item.Default
 		}
 	}
+	// Only catalog-declared keys reach the container: an orphaned stored credential
+	// (e.g. a pre-migration USER_ID/DEVICE_NAME left in the blob) must not leak into
+	// the environment of a service whose schema no longer includes it.
 	for key, value := range overrides {
-		env[key] = value
+		if declared[key] {
+			env[key] = value
+		}
 	}
+	// Expand {hostname} on BOTH defaults and overrides to the real hostname: an unedited
+	// form submits the raw "cashpilot-{hostname}" default back as an override, which the
+	// old default-only substitution left as a literal container name.
+	hostname := DeviceHostname()
 	for key, value := range env {
-		env[key] = substitute(value, env)
+		env[key] = substitute(strings.ReplaceAll(value, "{hostname}", hostname), env)
 	}
 	return env
 }

internal/runtime/runtime_test.go

@@ -705,34 +705,34 @@ func TestParseMemoryBytes(t *testing.T) {
 		want    int64
 		wantErr bool
 	}{
-		{in: "768m", want: 768 * 1024 * 1024},        // mysterium
-		{in: "2g", want: 2 * 1024 * 1024 * 1024},     // storj
-		{in: "1536m", want: 1536 * 1024 * 1024},      // anyone-protocol
-		{in: "256m", want: 256 * 1024 * 1024},        // honeygain/earnapp/proxyrack
-		{in: "128m", want: 128 * 1024 * 1024},        // expendable earners
-		{in: "512k", want: 512 * 1024},               // kibibytes
-		{in: "1024", want: 1024},                     // bare byte count, no suffix
-		{in: "1024b", want: 1024},                    // bare byte count, explicit trailing "b"
-		{in: "2gb", want: 2 * 1024 * 1024 * 1024},    // explicit trailing "b"
-		{in: "1gb", want: 1024 * 1024 * 1024},        // explicit trailing "b"
-		{in: "512mb", want: 512 * 1024 * 1024},       // explicit trailing "b"
-		{in: "4kb", want: 4 * 1024},                  // explicit trailing "b"
-		{in: "1t", want: 1024 * 1024 * 1024 * 1024},  // tebibytes
-		{in: "1tb", want: 1024 * 1024 * 1024 * 1024}, // tebibytes, explicit trailing "b"
-		{in: "768M", want: 768 * 1024 * 1024},        // case-insensitive suffix
-		{in: " 256m ", want: 256 * 1024 * 1024},      // surrounding whitespace
-		{in: "1.5g", want: 1536 * 1024 * 1024},       // fractional mantissa
-		{in: "", wantErr: true},                      // empty
-		{in: "   ", wantErr: true},                   // whitespace only
-		{in: "b", wantErr: true},                      // unit only, no number
-		{in: "m", wantErr: true},                      // unit only, no number
-		{in: "abc", wantErr: true},                    // not a number
-		{in: "12x", wantErr: true},                    // trailing character isn't a known unit
-		{in: "g2", wantErr: true},                     // unit-like character isn't trailing
-		{in: "0", wantErr: true},                      // non-positive
-		{in: "0m", wantErr: true},                      // non-positive
-		{in: "0g", wantErr: true},                      // non-positive
-		{in: "-5m", wantErr: true},                     // negative
+		{in: "768m", want: 768 * 1024 * 1024},            // mysterium
+		{in: "2g", want: 2 * 1024 * 1024 * 1024},         // storj
+		{in: "1536m", want: 1536 * 1024 * 1024},          // anyone-protocol
+		{in: "256m", want: 256 * 1024 * 1024},            // honeygain/earnapp/proxyrack
+		{in: "128m", want: 128 * 1024 * 1024},            // expendable earners
+		{in: "512k", want: 512 * 1024},                   // kibibytes
+		{in: "1024", want: 1024},                         // bare byte count, no suffix
+		{in: "1024b", want: 1024},                        // bare byte count, explicit trailing "b"
+		{in: "2gb", want: 2 * 1024 * 1024 * 1024},        // explicit trailing "b"
+		{in: "1gb", want: 1024 * 1024 * 1024},            // explicit trailing "b"
+		{in: "512mb", want: 512 * 1024 * 1024},           // explicit trailing "b"
+		{in: "4kb", want: 4 * 1024},                      // explicit trailing "b"
+		{in: "1t", want: 1024 * 1024 * 1024 * 1024},      // tebibytes
+		{in: "1tb", want: 1024 * 1024 * 1024 * 1024},     // tebibytes, explicit trailing "b"
+		{in: "768M", want: 768 * 1024 * 1024},            // case-insensitive suffix
+		{in: " 256m ", want: 256 * 1024 * 1024},          // surrounding whitespace
+		{in: "1.5g", want: 1536 * 1024 * 1024},           // fractional mantissa
+		{in: "", wantErr: true},                          // empty
+		{in: "   ", wantErr: true},                       // whitespace only
+		{in: "b", wantErr: true},                         // unit only, no number
+		{in: "m", wantErr: true},                         // unit only, no number
+		{in: "abc", wantErr: true},                       // not a number
+		{in: "12x", wantErr: true},                       // trailing character isn't a known unit
+		{in: "g2", wantErr: true},                        // unit-like character isn't trailing
+		{in: "0", wantErr: true},                         // non-positive
+		{in: "0m", wantErr: true},                        // non-positive
+		{in: "0g", wantErr: true},                        // non-positive
+		{in: "-5m", wantErr: true},                       // negative
 		{in: "100000000000000000000000t", wantErr: true}, // overflows a 64-bit byte count
 	}
 	for _, tc := range cases {
@@ -829,3 +829,22 @@ func TestApplyResourceLimitsRejectsBadValue(t *testing.T) {
 		}
 	}
 }
+
+// TestBuildEnvFiltersUndeclaredAndSubstitutesHostname covers CashPilot-Desktop-ada:
+// an orphaned (non-catalog) override key is dropped, and {hostname} is expanded to the
+// real host on BOTH defaults and overrides (an unedited form resubmits the raw default).
+func TestBuildEnvFiltersUndeclaredAndSubstitutesHostname(t *testing.T) {
+	svc := catalog.Service{Docker: catalog.DockerConfig{Env: []catalog.EnvVar{
+		{Key: "DEVICE_ID", Default: "cashpilot-{hostname}"},
+	}}}
+	env := buildEnv(svc, map[string]string{"ORPHAN": "leak", "DEVICE_ID": "cashpilot-{hostname}"})
+	if _, ok := env["ORPHAN"]; ok {
+		t.Fatalf("undeclared override leaked into env: %q", env["ORPHAN"])
+	}
+	if strings.Contains(env["DEVICE_ID"], "{hostname}") {
+		t.Fatalf("{hostname} left unexpanded in an override: %q", env["DEVICE_ID"])
+	}
+	if want := "cashpilot-" + DeviceHostname(); env["DEVICE_ID"] != want {
+		t.Fatalf("DEVICE_ID = %q, want %q", env["DEVICE_ID"], want)
+	}
+}

internal/services/manager.go

@@ -334,3 +334,30 @@ func validateRequired(svc catalog.Service, credentials map[string]string) error
 	}
 	return nil
 }
+
+// ValidateCredentials reports whether the given credentials would let the service
+// deploy — mirroring Deploy's pre-checks (known, deployable, required fields present) —
+// so a caller can validate BEFORE persisting anything.
+func (m *Manager) ValidateCredentials(slug string, credentials map[string]string) error {
+	svc, ok := m.catalog.Get(slug)
+	if !ok {
+		return fmt.Errorf("unknown service: %s", slug)
+	}
+	if svc.ManualOnly {
+		return fmt.Errorf("%s is tracked manually and has no Docker image", svc.Name)
+	}
+	return validateRequired(svc, credentials)
+}
+
+// RequiredCredentialsMet reports whether every required field for the service is present
+// in the given credentials. Unlike ValidateCredentials it does not reject a manually
+// tracked service, so the settings "Configured" badge can reflect whether the stored
+// blob actually satisfies the current schema — not merely that it is non-empty, which an
+// orphaned pre-migration blob (wrong keys) would also be.
+func (m *Manager) RequiredCredentialsMet(slug string, credentials map[string]string) bool {
+	svc, ok := m.catalog.Get(slug)
+	if !ok {
+		return false
+	}
+	return validateRequired(svc, credentials) == nil
+}

internal/services/manager_test.go

@@ -558,3 +558,36 @@ func TestRefreshRecordsResolvedRuntimeKindPerProvider(t *testing.T) {
 		t.Fatalf("expected earnapp recorded under native-process, got ok=%v runtime=%q", ok, dep.Runtime)
 	}
 }
+
+// TestValidateCredentials covers the deploy pre-validation used to validate BEFORE
+// persisting (CashPilot-Desktop-ada): it mirrors Deploy's known/deployable/required gates.
+func TestValidateCredentials(t *testing.T) {
+	m := NewManager(&fakeProvider{}, newTestCatalog(t), newTestStore(t))
+	if err := m.ValidateCredentials("does-not-exist", nil); err == nil || !strings.Contains(err.Error(), "unknown service") {
+		t.Fatalf("unknown service: got %v", err)
+	}
+	if err := m.ValidateCredentials("manual-svc", nil); err == nil || !strings.Contains(err.Error(), "tracked manually") {
+		t.Fatalf("manual only: got %v", err)
+	}
+	if err := m.ValidateCredentials("example", nil); err == nil || !strings.Contains(err.Error(), "missing required field") {
+		t.Fatalf("missing required: got %v", err)
+	}
+	if err := m.ValidateCredentials("example", map[string]string{"TOKEN": "x"}); err != nil {
+		t.Fatalf("valid creds should pass, got %v", err)
+	}
+}
+
+// TestRequiredCredentialsMet covers the "Configured" badge check: true only when the
+// current required fields are present, so an orphaned blob (wrong keys) reads false.
+func TestRequiredCredentialsMet(t *testing.T) {
+	m := NewManager(&fakeProvider{}, newTestCatalog(t), newTestStore(t))
+	if m.RequiredCredentialsMet("does-not-exist", nil) {
+		t.Fatal("unknown service must not be met")
+	}
+	if m.RequiredCredentialsMet("example", map[string]string{"ORPHAN": "old"}) {
+		t.Fatal("an orphaned blob missing the required TOKEN must not read as met")
+	}
+	if !m.RequiredCredentialsMet("example", map[string]string{"TOKEN": "x"}) {
+		t.Fatal("the required TOKEN present must read as met")
+	}
+}