chore: fix assorted PR #756 review nits

internal/agentruntime/runtime.go

@@ -83,6 +83,23 @@ func DashboardHostname(runtime Runtime, id string) string {
 	return Hostname(runtime, id)
 }
 
+// dnsLabelMaxLen is the RFC 1123 DNS label limit.
+const dnsLabelMaxLen = 63
+
+// MaxIDLength returns the longest id that keeps every DNS label this
+// package derives from it (namespace/hostname "<runtime>-<id>", and for
+// Hermes the "-ui" DashboardHostname suffix) within the 63-character DNS
+// label limit. validate.Name alone allows ids up to 63 chars, which is too
+// permissive here since Onboard prepends a runtime prefix before the id
+// ever reaches a Kubernetes object.
+func MaxIDLength(runtime Runtime) int {
+	reserved := len(string(runtime)) + 1 // "<runtime>-"
+	if runtime == Hermes {
+		reserved += len("-ui") // DashboardHostname's "hermes-<id>-ui" label
+	}
+	return dnsLabelMaxLen - reserved
+}
+
 func Hostnames(runtime Runtime, id string) []string {
 	if strings.TrimSpace(id) == "" {
 		return nil

internal/agentruntime/runtime_test.go

@@ -3,11 +3,42 @@ package agentruntime
 import (
 	"os"
 	"path/filepath"
+	"strings"
 	"testing"
 
 	"github.com/ObolNetwork/obol-stack/internal/config"
 )
 
+// TestMaxIDLengthKeepsDerivedLabelsWithinDNSLimit guards the DNS-label
+// overflow finding: an id at MaxIDLength must keep every label this package
+// derives (Namespace/Hostname, and for Hermes DashboardHostname's "-ui"
+// suffix) at or under the 63-character RFC 1123 limit.
+func TestMaxIDLengthKeepsDerivedLabelsWithinDNSLimit(t *testing.T) {
+	for _, rt := range []Runtime{OpenClaw, Hermes} {
+		id := strings.Repeat("a", MaxIDLength(rt))
+
+		if n := len(Namespace(rt, id)); n > 63 {
+			t.Errorf("%s: Namespace(%q) label is %d chars, want <=63", rt, id, n)
+		}
+		if n := len(strings.SplitN(Hostname(rt, id), ".", 2)[0]); n > 63 {
+			t.Errorf("%s: Hostname(%q) label is %d chars, want <=63", rt, id, n)
+		}
+		if n := len(strings.SplitN(DashboardHostname(rt, id), ".", 2)[0]); n > 63 {
+			t.Errorf("%s: DashboardHostname(%q) label is %d chars, want <=63", rt, id, n)
+		}
+
+		// One character longer must overflow at least one derived label —
+		// otherwise MaxIDLength is too conservative, not just safe.
+		tooLong := id + "a"
+		overflow := len(Namespace(rt, tooLong)) > 63 ||
+			len(strings.SplitN(Hostname(rt, tooLong), ".", 2)[0]) > 63 ||
+			len(strings.SplitN(DashboardHostname(rt, tooLong), ".", 2)[0]) > 63
+		if !overflow {
+			t.Errorf("%s: MaxIDLength+1 (%d chars) did not overflow any derived DNS label", rt, len(tooLong))
+		}
+	}
+}
+
 func TestHermesPaths(t *testing.T) {
 	cfg := &config.Config{
 		ConfigDir: "/tmp/obol-config",

internal/hermes/hermes.go

@@ -113,6 +113,13 @@ func Onboard(cfg *config.Config, opts OnboardOptions, u *ui.UI) error {
 	if err := validate.Name(id); err != nil {
 		return fmt.Errorf("invalid agent id: %w", err)
 	}
+	// validate.Name alone allows ids up to 63 chars, but Onboard derives the
+	// "hermes-<id>" namespace/hostname (and DashboardHostname's
+	// "hermes-<id>-ui" label) below — bound id here so those stay ≤63
+	// instead of failing later with an opaque Kubernetes error.
+	if max := agentruntime.MaxIDLength(agentruntime.Hermes); len(id) > max {
+		return fmt.Errorf("agent id %q is too long (%d chars): must be at most %d chars so hermes-<id> fits the 63-character DNS label limit", id, len(id), max)
+	}
 
 	deploymentDir := DeploymentPath(cfg, id)
 	namespace := agentruntime.Namespace(agentruntime.Hermes, id)

internal/hermes/hermes_test.go

@@ -138,6 +138,26 @@ func TestOnboardRejectsUnsafeID(t *testing.T) {
 	}
 }
 
+// TestOnboardRejectsIDTooLongForDNSLabel guards against the "hermes-<id>-ui"
+// DashboardHostname DNS label overflowing 63 characters: validate.Name alone
+// allows a 63-char id, but Onboard derives "hermes-<id>" (and, for the
+// dashboard, "hermes-<id>-ui") from it, so an id at (or near) that limit
+// must be rejected here with a clear error instead of failing later with an
+// opaque Kubernetes error.
+func TestOnboardRejectsIDTooLongForDNSLabel(t *testing.T) {
+	max := agentruntime.MaxIDLength(agentruntime.Hermes)
+
+	tooLong := "a" + strings.Repeat("b", max) // max+1 chars, still a valid DNS label on its own
+	cfg := testConfig(t)
+	err := Onboard(cfg, OnboardOptions{ID: tooLong}, newTestUI())
+	if err == nil {
+		t.Fatalf("Onboard(ID=%d chars) = nil, want error", len(tooLong))
+	}
+	if !strings.Contains(err.Error(), "too long") {
+		t.Errorf("error should explain the id is too long, got: %v", err)
+	}
+}
+
 // TestGenerateConfig_PrimaryIsRoundTrippable guards the LiteLLM model_name
 // contract end-to-end: whatever string the agent's `model.default` is set to
 // MUST match a `model_name` entry in the LiteLLM ConfigMap byte-for-byte,

internal/monetizeapi/agentidentity_test.go

@@ -66,3 +66,22 @@ func TestRemoveAgentIdentityRegistration_UnknownChainAndEmptyChainAreNoops(t *te
 		t.Errorf("base agentId = %q, want 42 unchanged by an empty-chain no-op", got)
 	}
 }
+
+// TestRemoveAgentIdentityRegistration_DoesNotMutateSharedBackingArray guards
+// against RemoveAgentIdentityRegistration filtering status.Registrations in
+// place: a caller (e.g. an informer-cached object) may hold another
+// reference to the same backing array, which an in-place compaction would
+// silently corrupt.
+func TestRemoveAgentIdentityRegistration_DoesNotMutateSharedBackingArray(t *testing.T) {
+	backing := []AgentIdentityRegistration{
+		{Chain: "base", AgentID: "1"},
+		{Chain: "base-sepolia", AgentID: "2"},
+	}
+	shared := AgentIdentityStatus{Registrations: backing}
+
+	_ = RemoveAgentIdentityRegistration(shared, "base")
+
+	if backing[0].Chain != "base" || backing[0].AgentID != "1" {
+		t.Errorf("shared backing array was mutated: backing[0] = %+v, want unchanged {base 1}", backing[0])
+	}
+}

internal/monetizeapi/types.go

@@ -1162,7 +1162,10 @@ func RemoveAgentIdentityRegistration(status AgentIdentityStatus, chain string) A
 	if chain == "" {
 		return status
 	}
-	out := status.Registrations[:0]
+	// Allocate rather than filter status.Registrations in place: this is an
+	// exported helper and a future caller could pass in an informer-cached
+	// object whose backing array must not be mutated out from under it.
+	out := make([]AgentIdentityRegistration, 0, len(status.Registrations))
 	for _, registration := range status.Registrations {
 		if strings.EqualFold(strings.TrimSpace(registration.Chain), chain) {
 			continue

internal/openclaw/onboard_test.go

@@ -1,8 +1,10 @@
 package openclaw
 
 import (
+	"strings"
 	"testing"
 
+	"github.com/ObolNetwork/obol-stack/internal/agentruntime"
 	"github.com/ObolNetwork/obol-stack/internal/ui"
 )
 
@@ -26,3 +28,22 @@ func TestOnboardRejectsUnsafeID(t *testing.T) {
 		}
 	}
 }
+
+// TestOnboardRejectsIDTooLongForDNSLabel guards against the "openclaw-<id>"
+// DNS label overflowing 63 characters: validate.Name alone allows a 63-char
+// id, but Onboard prepends "openclaw-" to it, so an id at (or near) that
+// limit must be rejected here with a clear error instead of failing later
+// with an opaque Kubernetes error.
+func TestOnboardRejectsIDTooLongForDNSLabel(t *testing.T) {
+	max := agentruntime.MaxIDLength(agentruntime.OpenClaw)
+
+	tooLong := "a" + strings.Repeat("b", max) // max+1 chars, still a valid DNS label on its own
+	cfg := testConfig(t)
+	err := Onboard(cfg, OnboardOptions{ID: tooLong}, ui.New(false))
+	if err == nil {
+		t.Fatalf("Onboard(ID=%d chars) = nil, want error", len(tooLong))
+	}
+	if !strings.Contains(err.Error(), "too long") {
+		t.Errorf("error should explain the id is too long, got: %v", err)
+	}
+}

internal/openclaw/openclaw.go

@@ -175,6 +175,12 @@ func Onboard(cfg *config.Config, opts OnboardOptions, u *ui.UI) error {
 	if err := validate.Name(id); err != nil {
 		return fmt.Errorf("invalid agent id: %w", err)
 	}
+	// validate.Name alone allows ids up to 63 chars, but Onboard derives the
+	// "openclaw-<id>" DNS label below — bound id here so that stays ≤63
+	// instead of failing later with an opaque Kubernetes error.
+	if max := agentruntime.MaxIDLength(agentruntime.OpenClaw); len(id) > max {
+		return fmt.Errorf("agent id %q is too long (%d chars): must be at most %d chars so %q-<id> fits the 63-character DNS label limit", id, len(id), max, appName)
+	}
 
 	deploymentDir := DeploymentPath(cfg, id)
 

internal/stack/safety.go

@@ -273,9 +273,11 @@ func pidAlive(pid int) bool {
 	return p.Signal(syscall.Signal(0)) == nil
 }
 
-// errSafetyAborted lets cmd/obol/stack.go distinguish "user said no" from
-// other errors. The CLI maps this to exit code 0 with a brief message
-// instead of a noisy error trace.
+// errSafetyAborted is a sentinel for "user declined a ConfirmRunningServicesLoss
+// prompt". destroyOldBackendIfSwitching returns it so Init stops the whole
+// command before switching backends (Down/Purge instead return nil directly
+// from their own top-level check); Init maps it to a clean exit-0. Exported via
+// ErrSafetyAborted() for callers that want to detect the abort with errors.Is.
 var errSafetyAborted = errors.New("aborted by operator at safety prompt")
 
 // ErrSafetyAborted is exported for callers that want to detect the abort

internal/stack/stack.go

@@ -107,6 +107,11 @@ func Init(cfg *config.Config, u *ui.UI, force bool, backendName string, skipConf
 	// live traffic.
 	if hasExistingConfig && force {
 		if err := destroyOldBackendIfSwitching(cfg, u, backendName, stackID, skipConfirm); err != nil {
+			// A declined safety prompt stops Init entirely (nothing switched,
+			// old cluster left intact) and, like Down/Purge, exits cleanly.
+			if errors.Is(err, errSafetyAborted) {
+				return nil
+			}
 			return err
 		}
 	}
@@ -156,6 +161,9 @@ func destroyOldBackendIfSwitching(cfg *config.Config, u *ui.UI, newBackend, stac
 		return err
 	}
 	if !proceed {
+		// Signal the decline to Init, which stops the whole command before
+		// touching anything (the old cluster is still serving traffic — it
+		// must NOT be left undestroyed while the backend config switches).
 		u.Info("Aborted.")
 		return errSafetyAborted
 	}

internal/tunnel/tunnel.go

@@ -1051,8 +1051,18 @@ func EnsureTunnelForSell(cfg *config.Config, u *ui.UI) (string, error) {
 // reflected immediately, instead of shadowing the offer's route until some
 // later, unrelated tunnel/sell invocation happens to run CreateStorefront
 // again (Canary402).
+//
+// It no-ops quietly when there is no persistent tunnel/hostname state yet
+// (e.g. a first `obol sell ... --hostname X --no-register` before any
+// tunnel has ever been created) — CreateStorefront has nothing to publish
+// in that case, and EnsureTunnelForSell reconciles the storefront once the
+// tunnel comes up.
 func RefreshStorefront(cfg *config.Config) error {
-	return CreateStorefront(cfg, storefrontHostnames(cfg, "")...)
+	hosts := storefrontHostnames(cfg, "")
+	if len(hosts) == 0 {
+		return nil
+	}
+	return CreateStorefront(cfg, hosts...)
 }
 
 // Stop scales the cloudflared deployment to 0 replicas.

internal/tunnel/tunnel_test.go

@@ -211,6 +211,30 @@ func TestCreateStorefront_TearsDownWhenAllHostsOfferBound(t *testing.T) {
 	}
 }
 
+// TestRefreshStorefront_NoOpWithoutTunnelState guards a first `obol sell
+// ... --hostname X --no-register` before any persistent tunnel has ever been
+// created: storefrontHostnames("") has nothing to report (no persistent
+// tunnel state, no quick-tunnel URL to parse), so RefreshStorefront must
+// no-op quietly instead of calling CreateStorefront with zero hostnames and
+// surfacing its "requires at least one hostname" error as a confusing
+// warning. EnsureTunnelForSell reconciles the storefront later once the
+// tunnel exists.
+func TestRefreshStorefront_NoOpWithoutTunnelState(t *testing.T) {
+	cfg := newHostnameTestConfig(t)
+	writeFakeKubeconfig(t, cfg)
+
+	logPath := filepath.Join(cfg.ConfigDir, "kubectl.log")
+	writeFakeKubectl(t, cfg, logPath, "")
+
+	if err := RefreshStorefront(cfg); err != nil {
+		t.Fatalf("RefreshStorefront should no-op without tunnel state, got error: %v", err)
+	}
+
+	if _, err := os.ReadFile(logPath); err == nil {
+		t.Fatal("RefreshStorefront must not invoke kubectl when there is no hostname to publish")
+	}
+}
+
 func TestPatchAgentBaseURL_Insert(t *testing.T) {
 	dir := t.TempDir()
 	path := filepath.Join(dir, "values-obol.yaml")

obolup.sh

@@ -519,10 +519,11 @@ download_with_retries() {
 }
 
 # Verify a downloaded release binary against the published SHA256SUMS file.
-# Fails closed (returns 1) on checksum mismatch, a missing checksum entry, or
-# no local sha256 tool. If SHA256SUMS itself can't be fetched, also fails
-# closed unless OBOL_ALLOW_UNVERIFIED=1 is explicitly set, in which case the
-# install proceeds unverified.
+# Fails closed: returns 2 on a *verified* checksum mismatch (tamper — the caller
+# must abort hard and NOT fall back to a source build), and returns 1 on a
+# missing checksum entry or no local sha256 tool. If SHA256SUMS itself can't be
+# fetched, also fails closed unless OBOL_ALLOW_UNVERIFIED=1 is explicitly set,
+# in which case the install proceeds unverified (return 0).
 verify_release_checksum() {
 	local release_tag="$1"
 	local binary_name="$2"
@@ -570,7 +571,12 @@ verify_release_checksum() {
 		echo "Please re-run the installer. If the problem persists, report an issue at:"
 		echo "  https://github.com/ObolNetwork/obol-stack/issues"
 		echo ""
-		return 1
+		# Exit code 2 signals a *verified* mismatch (as opposed to the
+		# generic 1 used above for "couldn't verify at all") so callers can
+		# tell tamper apart from unavailability and refuse to silently fall
+		# back to another download over the same (possibly compromised)
+		# channel.
+		return 2
 	fi
 
 	log_success "Checksum verified for $binary_name"
@@ -617,10 +623,15 @@ download_release() {
 		return 1
 	fi
 
-	# Verify against the published SHA256SUMS before installing
-	if ! verify_release_checksum "$release_tag" "$binary_name" "$tmp_obol"; then
+	# Verify against the published SHA256SUMS before installing. Propagate
+	# a verified-tamper (exit 2) distinctly from a generic verification
+	# failure (exit 1) so the caller knows falling back to another download
+	# method is unsafe.
+	local verify_status=0
+	verify_release_checksum "$release_tag" "$binary_name" "$tmp_obol" || verify_status=$?
+	if [[ "$verify_status" -ne 0 ]]; then
 		rm -f "$tmp_obol"
-		return 1
+		return "$verify_status"
 	fi
 
 	chmod +x "$tmp_obol"
@@ -715,10 +726,20 @@ install_obol_binary() {
 
 		# If we got a tag, try to download it
 		if [[ -n "$latest_tag" ]]; then
-			if download_release "$latest_tag"; then
+			local dl_status=0
+			download_release "$latest_tag" || dl_status=$?
+			if [[ "$dl_status" -eq 0 ]]; then
 				show_version_change "$current_version"
 				return 0
 			fi
+			if [[ "$dl_status" -eq 2 ]]; then
+				# Verified tamper (checksum mismatch), not mere
+				# unavailability: building from source would just re-fetch
+				# over the same, possibly compromised, channel. Abort hard
+				# instead of silently "recovering" into an unverified install.
+				log_error "Checksum verification failed for $latest_tag — refusing to fall back to building from source over the same network channel."
+				return 1
+			fi
 			log_warn "Download failed, falling back to building from source..."
 		else
 			log_info "No releases found, building from source..."
@@ -730,12 +751,18 @@ install_obol_binary() {
 	else
 		# Specific release requested
 		log_info "Attempting to download release: $release"
-		if download_release "$release"; then
+		local dl_status=0
+		download_release "$release" || dl_status=$?
+		if [[ "$dl_status" -eq 0 ]]; then
 			show_version_change "$current_version"
 			return 0
 		fi
+		if [[ "$dl_status" -eq 2 ]]; then
+			log_error "Checksum verification failed for $release — refusing to fall back to building from source over the same network channel."
+			return 1
+		fi
 
-		log_warn "Release $release not found, building from source..."
+		log_warn "Release $release not found or download failed, building from source..."
 		build_from_source "$release"
 		show_version_change "$current_version"
 	fi