From 29e55fe5b9d578ca6b050d170c542b32eca5f569 Mon Sep 17 00:00:00 2001
From: Sam Morrow <sammorrowdrums@github.com>
Date: Thu, 16 Apr 2026 15:52:53 +0200
Subject: [PATCH] refactor: remove redundant nil-client fallback in skills
 publish (#13168)

Remove the dead code block that silently swallowed errors from
opts.HttpClient() and opts.Config() when creating an API client.
Instead, create the client with proper error propagation inside the
remote-checks block where it is actually needed.

Changes:
- Remove the error-swallowing client == nil fallback (lines 363-376)
- Create the API client inside the remote repo checks block with
  proper error returns from HttpClient() and Config()
- Resolve host from repoInfo first, then fall back to config
- Remove the now-unreachable 'client == nil' early exit before publish

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
---
 pkg/cmd/skills/publish/publish.go | 71 ++++++++++++++-----------------
 1 file changed, 33 insertions(+), 38 deletions(-)

diff --git a/pkg/cmd/skills/publish/publish.go b/pkg/cmd/skills/publish/publish.go
index 9f981104f..213afeba5 100644
--- a/pkg/cmd/skills/publish/publish.go
+++ b/pkg/cmd/skills/publish/publish.go
@@ -178,7 +178,10 @@ func publishRun(opts *PublishOptions) error {
 
 	canPrompt := opts.IO.CanPrompt()
 
-	// Use injected client or create one from the factory HttpClient
+	// Use injected client or create one from the factory HttpClient.
+	// Initialization is deferred until after local validation so that
+	// simple errors (missing skills/, bad SKILL.md, etc.) are reported
+	// without requiring an HTTP client.
 	client := opts.client
 	host := opts.host
 
@@ -336,52 +339,49 @@ func publishRun(opts *PublishOptions) error {
 		owner = repoInfo.Repo.RepoOwner()
 		repo = repoInfo.Repo.RepoName()
 	}
+
 	hasTopic := false
 	var existingTags []tagEntry
 	if owner != "" && repo != "" {
+		// Create API client from factory if not already injected (tests inject directly).
+		if client == nil {
+			httpClient, err := opts.HttpClient()
+			if err != nil {
+				return err
+			}
+			client = api.NewClientFromHTTP(httpClient)
+		}
+
 		if host == "" && repoInfo != nil {
 			host = repoInfo.Repo.RepoHost()
 		}
-		if host != "" {
-			if err := source.ValidateSupportedHost(host); err != nil {
+		if host == "" {
+			cfg, err := opts.Config()
+			if err != nil {
 				return err
 			}
+			host, _ = cfg.Authentication().DefaultHost()
+		}
+		if err := source.ValidateSupportedHost(host); err != nil {
+			return err
 		}
 
-		// Create API client for remote checks if not already injected
-		if client == nil {
-			httpClient, httpErr := opts.HttpClient()
-			if httpErr == nil {
-				apiClient := api.NewClientFromHTTP(httpClient)
-				cfg, cfgErr := opts.Config()
-				if cfgErr == nil {
-					host, _ = cfg.Authentication().DefaultHost()
-					if err := source.ValidateSupportedHost(host); err != nil {
-						return err
-					}
-					client = apiClient
-				}
-			}
+		// Security and ruleset checks (advisory, always shown)
+		var skillAbsDirs []string
+		for _, skill := range skills {
+			skillAbsDirs = append(skillAbsDirs, filepath.Join(dir, filepath.FromSlash(skill.Path)))
 		}
+		securityDiags := checkSecuritySettings(client, host, owner, repo, skillAbsDirs)
+		diagnostics = append(diagnostics, securityDiags...)
 
-		if client != nil {
-			// Security and ruleset checks (advisory, always shown)
-			var skillAbsDirs []string
-			for _, skill := range skills {
-				skillAbsDirs = append(skillAbsDirs, filepath.Join(dir, filepath.FromSlash(skill.Path)))
-			}
-			securityDiags := checkSecuritySettings(client, host, owner, repo, skillAbsDirs)
-			diagnostics = append(diagnostics, securityDiags...)
+		rulesetDiags := checkTagProtection(client, host, owner, repo)
+		diagnostics = append(diagnostics, rulesetDiags...)
 
-			rulesetDiags := checkTagProtection(client, host, owner, repo)
-			diagnostics = append(diagnostics, rulesetDiags...)
+		// Check topic (needed for publish flow, not a blocking error)
+		hasTopic = repoHasTopic(client, host, owner, repo)
 
-			// Check topic (needed for publish flow, not a blocking error)
-			hasTopic = repoHasTopic(client, host, owner, repo)
-
-			// Fetch existing tags (needed for version suggestion)
-			existingTags = fetchTags(client, host, owner, repo)
-		}
+		// Fetch existing tags (needed for version suggestion)
+		existingTags = fetchTags(client, host, owner, repo)
 	} else {
 		diagnostics = append(diagnostics, detectMissingRepoDiagnostic(opts.GitClient, dir)...)
 	}
@@ -425,11 +425,6 @@ func publishRun(opts *PublishOptions) error {
 		return nil
 	}
 
-	if client == nil {
-		fmt.Fprintf(opts.IO.ErrOut, "\nValidation passed but could not create API client. Check your authentication configuration.\n")
-		return nil
-	}
-
 	fmt.Fprintf(opts.IO.ErrOut, "\nPublishing to %s/%s...\n\n", owner, repo)
 
 	return runPublishRelease(opts, client, host, owner, repo, dir, repoInfo.RemoteName, hasTopic, existingTags)