[object Object]

← back to Cli Printing Press

fix(cli): route scorer subcommand --spec URLs through generate's fetch path (#1178)

5e3667df2e2aabef62b0110b7557b93df7fae2cf · 2026-05-12 01:54:39 -0700 · Trevin Chow

* fix(cli): route scorer subcommand --spec URLs through the same fetch path generate uses

The scorer subcommands (dogfood, verify, scorecard) called os.ReadFile
directly on the --spec argument, so http(s) URLs surfaced as
"no such file or directory" while `generate` accepted the same URL fine.
On Windows the error message ("filename, directory name, or volume label
syntax is incorrect") was the loudest, but the bug was platform-agnostic.

Extract a single URL-aware loader into internal/openapi as LoadSpecBytes
(plus an exported FetchOrCacheSpec) and route the pipeline loaders
(loadOpenAPISpec, loadDogfoodOpenAPISpec, fullrun's readSpecBytes) and
the cli readSpec wrapper through it. http(s) sources now get the same
24h on-disk cache the generator uses; filesystem sources behave as
before.

Fixes #1001

* fix(cli): hoist spec_loader small-response regex; document fullrun cache reuse

Addresses two PR-review threads on the loader extraction:

- Hoist the `^\d{3}:\s` pattern in openapi.FetchOrCacheSpec to package
  scope so the small-response sniff doesn't recompile on every call.
- Note in fullrun.readSpecBytes's doc comment that URL sources now share
  the generator's 24h cache, which is the intended improvement: the
  spec.json archived next to the generated CLI matches the spec the
  generator parsed, even when the upstream rolled between reads.

Files touched

Diff

commit 5e3667df2e2aabef62b0110b7557b93df7fae2cf
Author: Trevin Chow <trevin@trevinchow.com>
Date:   Tue May 12 01:54:39 2026 -0700

    fix(cli): route scorer subcommand --spec URLs through generate's fetch path (#1178)
    
    * fix(cli): route scorer subcommand --spec URLs through the same fetch path generate uses
    
    The scorer subcommands (dogfood, verify, scorecard) called os.ReadFile
    directly on the --spec argument, so http(s) URLs surfaced as
    "no such file or directory" while `generate` accepted the same URL fine.
    On Windows the error message ("filename, directory name, or volume label
    syntax is incorrect") was the loudest, but the bug was platform-agnostic.
    
    Extract a single URL-aware loader into internal/openapi as LoadSpecBytes
    (plus an exported FetchOrCacheSpec) and route the pipeline loaders
    (loadOpenAPISpec, loadDogfoodOpenAPISpec, fullrun's readSpecBytes) and
    the cli readSpec wrapper through it. http(s) sources now get the same
    24h on-disk cache the generator uses; filesystem sources behave as
    before.
    
    Fixes #1001
    
    * fix(cli): hoist spec_loader small-response regex; document fullrun cache reuse
    
    Addresses two PR-review threads on the loader extraction:
    
    - Hoist the `^\d{3}:\s` pattern in openapi.FetchOrCacheSpec to package
      scope so the small-response sniff doesn't recompile on every call.
    - Note in fullrun.readSpecBytes's doc comment that URL sources now share
      the generator's 24h cache, which is the intended improvement: the
      spec.json archived next to the generated CLI matches the spec the
      generator parsed, even when the upstream rolled between reads.
---
 internal/cli/root.go                      |  85 +----------------------
 internal/cli/root_test.go                 |   4 +-
 internal/cli/spec_content_check_test.go   |   2 +-
 internal/openapi/spec_loader.go           | 110 ++++++++++++++++++++++++++++++
 internal/openapi/spec_loader_test.go      | 100 +++++++++++++++++++++++++++
 internal/pipeline/dogfood.go              |   2 +-
 internal/pipeline/fullrun.go              |  20 ++----
 internal/pipeline/scorecard.go            |   3 +-
 internal/pipeline/scorecard_tier2_test.go |  34 +++++++++
 9 files changed, 258 insertions(+), 102 deletions(-)

diff --git a/internal/cli/root.go b/internal/cli/root.go
index 40fab783..102cdd07 100644
--- a/internal/cli/root.go
+++ b/internal/cli/root.go
@@ -6,13 +6,10 @@ import (
 	"encoding/json"
 	"errors"
 	"fmt"
-	"io"
-	"net/http"
 	"net/url"
 	"os"
 	"os/exec"
 	"path/filepath"
-	"regexp"
 	"runtime"
 	"strconv"
 	"strings"
@@ -683,13 +680,7 @@ func inferTrafficAnalysisPath(specFiles []string, specSource string) string {
 }
 
 func readSpec(specFile string, refresh bool, skipCache bool) ([]byte, error) {
-	var data []byte
-	var err error
-	if openapi.IsRemoteSpecSource(specFile) {
-		data, err = fetchOrCacheSpec(specFile, refresh, skipCache)
-	} else {
-		data, err = os.ReadFile(specFile)
-	}
+	data, err := openapi.LoadSpecBytes(specFile, refresh, skipCache)
 	if err != nil {
 		return nil, err
 	}
@@ -1229,80 +1220,6 @@ func refuseSymlinkedEntries(dir, label string) error {
 	return nil
 }
 
-func fetchOrCacheSpec(specURL string, refresh bool, skipCache bool) ([]byte, error) {
-	sum := sha256.Sum256([]byte(specURL))
-	cacheKey := hex.EncodeToString(sum[:])
-
-	homeDir, err := os.UserHomeDir()
-	if err != nil {
-		return nil, fmt.Errorf("finding user home directory: %w", err)
-	}
-
-	cacheDir := filepath.Join(homeDir, ".cache", "printing-press", "specs")
-	cachePath := filepath.Join(cacheDir, cacheKey+".json")
-
-	// Read from existing cache even in dry-run mode (no writes needed)
-	if !refresh {
-		info, err := os.Stat(cachePath)
-		switch {
-		case err == nil && time.Since(info.ModTime()) < 24*time.Hour:
-			fmt.Fprintf(os.Stderr, "Using cached spec for %s\n", specURL)
-			data, readErr := os.ReadFile(cachePath)
-			if readErr != nil {
-				return nil, fmt.Errorf("reading cached spec: %w", readErr)
-			}
-			return data, nil
-		case err != nil && !os.IsNotExist(err):
-			return nil, fmt.Errorf("checking cached spec: %w", err)
-		}
-	}
-
-	fmt.Fprintf(os.Stderr, "Fetching spec from %s...\n", specURL)
-	resp, err := http.Get(specURL)
-	if err != nil {
-		return nil, err
-	}
-	defer func() { _ = resp.Body.Close() }()
-
-	if resp.StatusCode < http.StatusOK || resp.StatusCode >= http.StatusMultipleChoices {
-		return nil, fmt.Errorf("unexpected response status: %s", resp.Status)
-	}
-
-	data, err := io.ReadAll(resp.Body)
-	if err != nil {
-		return nil, fmt.Errorf("reading response body: %w", err)
-	}
-
-	// Content-validity check: reject responses that look like error pages
-	// instead of feeding them to the parser (which emits confusing errors).
-	if len(data) < 256 {
-		trimmed := strings.TrimSpace(string(data))
-		if strings.HasPrefix(trimmed, "<") ||
-			regexp.MustCompile(`^\d{3}:\s`).MatchString(trimmed) {
-			return nil, fmt.Errorf("spec_url %s returned a small response that does not look like an OpenAPI spec (%d bytes): %q",
-				specURL, len(data), trunc50(trimmed))
-		}
-	}
-
-	if !skipCache {
-		if err := os.MkdirAll(cacheDir, 0o755); err != nil {
-			return nil, fmt.Errorf("creating cache directory: %w", err)
-		}
-		if err := os.WriteFile(cachePath, data, 0o644); err != nil {
-			return nil, fmt.Errorf("writing cached spec: %w", err)
-		}
-	}
-
-	return data, nil
-}
-
-func trunc50(s string) string {
-	if len(s) > 50 {
-		return s[:50] + "..."
-	}
-	return s
-}
-
 func newVersionCmd() *cobra.Command {
 	var asJSON bool
 
diff --git a/internal/cli/root_test.go b/internal/cli/root_test.go
index fb700dbd..e0ad8573 100644
--- a/internal/cli/root_test.go
+++ b/internal/cli/root_test.go
@@ -4,6 +4,8 @@ import (
 	"net/http"
 	"net/http/httptest"
 	"testing"
+
+	"github.com/mvanhorn/cli-printing-press/v4/internal/openapi"
 )
 
 func TestFetchOrCacheSpec_ContentValidityCheck(t *testing.T) {
@@ -41,7 +43,7 @@ func TestFetchOrCacheSpec_ContentValidityCheck(t *testing.T) {
 			}))
 			defer srv.Close()
 
-			_, err := fetchOrCacheSpec(srv.URL, true, true)
+			_, err := openapi.FetchOrCacheSpec(srv.URL, true, true)
 			if tc.wantErr == "" {
 				if err != nil {
 					t.Fatalf("expected no error, got: %v", err)
diff --git a/internal/cli/spec_content_check_test.go b/internal/cli/spec_content_check_test.go
index 3ac03564..74935539 100644
--- a/internal/cli/spec_content_check_test.go
+++ b/internal/cli/spec_content_check_test.go
@@ -11,7 +11,7 @@ import (
 
 // TestRejectIfNotSpec covers issue #275 F-5's content-validity heuristic.
 // A stale catalog spec_url returns "404: Not Found" or an HTML error page;
-// the existing fetchOrCacheSpec status check catches HTTP-level errors, but
+// the existing openapi.FetchOrCacheSpec status check catches HTTP-level errors, but
 // any path that hands a downloaded body to readSpec without that gate (a
 // user feeding a curl-saved 404 dump as --spec, a stale cache entry, an
 // upstream proxy that masks status codes) bypasses it. rejectIfNotSpec
diff --git a/internal/openapi/spec_loader.go b/internal/openapi/spec_loader.go
new file mode 100644
index 00000000..f82f78bc
--- /dev/null
+++ b/internal/openapi/spec_loader.go
@@ -0,0 +1,110 @@
+package openapi
+
+import (
+	"crypto/sha256"
+	"encoding/hex"
+	"fmt"
+	"io"
+	"net/http"
+	"os"
+	"path/filepath"
+	"regexp"
+	"strings"
+	"time"
+)
+
+// smallResponseErrorPrefix matches the "404: Not Found" / "500: ..." shape
+// some upstreams return as a 200-body payload instead of an HTTP error. The
+// small-response branch in FetchOrCacheSpec uses it to surface a clear
+// rejection at the boundary rather than feeding the body to the parser.
+var smallResponseErrorPrefix = regexp.MustCompile(`^\d{3}:\s`)
+
+// LoadSpecBytes reads OpenAPI spec bytes from either a local filesystem path
+// or an http(s) URL, picking the right transport from the source string.
+// Callers that previously wrapped os.ReadFile gain URL support transparently;
+// the local-path branch is identical to the prior os.ReadFile call.
+//
+// URL responses are cached under ~/.cache/printing-press/specs for 24h to
+// match the generator's intake behavior; pass refresh=true to bypass the
+// cache and skipCache=true to skip writing it.
+func LoadSpecBytes(source string, refresh bool, skipCache bool) ([]byte, error) {
+	if IsRemoteSpecSource(source) {
+		return FetchOrCacheSpec(source, refresh, skipCache)
+	}
+	return os.ReadFile(source)
+}
+
+// FetchOrCacheSpec downloads a spec over http(s) with a 24h on-disk cache.
+// Exported so packages outside internal/cli (notably internal/pipeline) can
+// route URL-sourced specs through the same fetch path the generator uses,
+// keeping scorer subcommands and generate in sync.
+func FetchOrCacheSpec(specURL string, refresh bool, skipCache bool) ([]byte, error) {
+	sum := sha256.Sum256([]byte(specURL))
+	cacheKey := hex.EncodeToString(sum[:])
+
+	homeDir, err := os.UserHomeDir()
+	if err != nil {
+		return nil, fmt.Errorf("finding user home directory: %w", err)
+	}
+
+	cacheDir := filepath.Join(homeDir, ".cache", "printing-press", "specs")
+	cachePath := filepath.Join(cacheDir, cacheKey+".json")
+
+	if !refresh {
+		info, err := os.Stat(cachePath)
+		switch {
+		case err == nil && time.Since(info.ModTime()) < 24*time.Hour:
+			fmt.Fprintf(os.Stderr, "Using cached spec for %s\n", specURL)
+			data, readErr := os.ReadFile(cachePath)
+			if readErr != nil {
+				return nil, fmt.Errorf("reading cached spec: %w", readErr)
+			}
+			return data, nil
+		case err != nil && !os.IsNotExist(err):
+			return nil, fmt.Errorf("checking cached spec: %w", err)
+		}
+	}
+
+	fmt.Fprintf(os.Stderr, "Fetching spec from %s...\n", specURL)
+	resp, err := http.Get(specURL) //nolint:gosec // spec URLs are operator-provided
+	if err != nil {
+		return nil, err
+	}
+	defer func() { _ = resp.Body.Close() }()
+
+	if resp.StatusCode < http.StatusOK || resp.StatusCode >= http.StatusMultipleChoices {
+		return nil, fmt.Errorf("unexpected response status: %s", resp.Status)
+	}
+
+	data, err := io.ReadAll(resp.Body)
+	if err != nil {
+		return nil, fmt.Errorf("reading response body: %w", err)
+	}
+
+	if len(data) < 256 {
+		trimmed := strings.TrimSpace(string(data))
+		if strings.HasPrefix(trimmed, "<") ||
+			smallResponseErrorPrefix.MatchString(trimmed) {
+			return nil, fmt.Errorf("spec_url %s returned a small response that does not look like an OpenAPI spec (%d bytes): %q",
+				specURL, len(data), truncFifty(trimmed))
+		}
+	}
+
+	if !skipCache {
+		if err := os.MkdirAll(cacheDir, 0o755); err != nil {
+			return nil, fmt.Errorf("creating cache directory: %w", err)
+		}
+		if err := os.WriteFile(cachePath, data, 0o644); err != nil {
+			return nil, fmt.Errorf("writing cached spec: %w", err)
+		}
+	}
+
+	return data, nil
+}
+
+func truncFifty(s string) string {
+	if len(s) > 50 {
+		return s[:50] + "..."
+	}
+	return s
+}
diff --git a/internal/openapi/spec_loader_test.go b/internal/openapi/spec_loader_test.go
new file mode 100644
index 00000000..2c0a16c7
--- /dev/null
+++ b/internal/openapi/spec_loader_test.go
@@ -0,0 +1,100 @@
+package openapi
+
+import (
+	"net/http"
+	"net/http/httptest"
+	"os"
+	"path/filepath"
+	"strings"
+	"testing"
+)
+
+// TestLoadSpecBytes_URLAndFile verifies the URL-vs-file dispatch.
+//
+// Before #1001, pipeline subcommands (dogfood/verify/scorecard) called
+// os.ReadFile directly on the --spec argument and rejected http(s) URLs as
+// "no such file or directory" on every platform (Windows worst because the
+// error message was misleading). This test pins the new helper's contract:
+// http(s) sources route through the fetch path, everything else routes to
+// the filesystem, and the dispatch decision is made off the source string
+// — not the running platform.
+func TestLoadSpecBytes_URLAndFile(t *testing.T) {
+	t.Setenv("HOME", t.TempDir())
+
+	specPayload := `{"openapi":"3.0.0","info":{"title":"Test","version":"1.0"},"paths":{}}` + strings.Repeat(" ", 300)
+
+	srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+		w.WriteHeader(http.StatusOK)
+		_, _ = w.Write([]byte(specPayload))
+	}))
+	defer srv.Close()
+
+	t.Run("URL source routes to http fetch", func(t *testing.T) {
+		data, err := LoadSpecBytes(srv.URL, true, true)
+		if err != nil {
+			t.Fatalf("LoadSpecBytes(URL) returned error: %v", err)
+		}
+		if string(data) != specPayload {
+			t.Fatalf("LoadSpecBytes(URL) returned unexpected body: got %d bytes, want %d", len(data), len(specPayload))
+		}
+	})
+
+	t.Run("file source routes to disk read", func(t *testing.T) {
+		dir := t.TempDir()
+		path := filepath.Join(dir, "spec.json")
+		if err := os.WriteFile(path, []byte(specPayload), 0o644); err != nil {
+			t.Fatalf("writing fixture: %v", err)
+		}
+
+		data, err := LoadSpecBytes(path, false, false)
+		if err != nil {
+			t.Fatalf("LoadSpecBytes(file) returned error: %v", err)
+		}
+		if string(data) != specPayload {
+			t.Fatalf("LoadSpecBytes(file) returned unexpected body")
+		}
+	})
+
+	t.Run("URL with http:// prefix is also remote", func(t *testing.T) {
+		if !strings.HasPrefix(srv.URL, "http://") {
+			t.Fatalf("test precondition: httptest URL should start with http://, got %s", srv.URL)
+		}
+		if !IsRemoteSpecSource(srv.URL) {
+			t.Fatalf("IsRemoteSpecSource(%q) = false, want true", srv.URL)
+		}
+	})
+
+	t.Run("Windows-shaped path is not remote", func(t *testing.T) {
+		// Regression guard for #1001: a Windows-shaped filesystem path must
+		// not be misclassified as remote even though it contains colons.
+		path := `C:\Users\dev\spec.json`
+		if IsRemoteSpecSource(path) {
+			t.Fatalf("IsRemoteSpecSource(%q) = true, want false", path)
+		}
+	})
+}
+
+func TestLoadSpecBytes_FileMissing(t *testing.T) {
+	_, err := LoadSpecBytes(filepath.Join(t.TempDir(), "does-not-exist.json"), false, false)
+	if err == nil {
+		t.Fatal("LoadSpecBytes for missing file should error")
+	}
+}
+
+func TestLoadSpecBytes_RemoteServerError(t *testing.T) {
+	t.Setenv("HOME", t.TempDir())
+
+	srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+		w.WriteHeader(http.StatusNotFound)
+		_, _ = w.Write([]byte("404 Not Found"))
+	}))
+	defer srv.Close()
+
+	_, err := LoadSpecBytes(srv.URL, true, true)
+	if err == nil {
+		t.Fatal("LoadSpecBytes against 404 should error")
+	}
+	if !strings.Contains(err.Error(), "404") && !strings.Contains(err.Error(), "Not Found") {
+		t.Fatalf("error should mention HTTP status, got: %v", err)
+	}
+}
diff --git a/internal/pipeline/dogfood.go b/internal/pipeline/dogfood.go
index 8c8dd3ab..083d49d1 100644
--- a/internal/pipeline/dogfood.go
+++ b/internal/pipeline/dogfood.go
@@ -609,7 +609,7 @@ func writeDogfoodResults(report *DogfoodReport, dir string) error {
 }
 
 func loadDogfoodOpenAPISpec(specPath string) (*openAPISpec, error) {
-	data, err := os.ReadFile(specPath)
+	data, err := openapiparser.LoadSpecBytes(specPath, false, false)
 	if err != nil {
 		return nil, fmt.Errorf("reading spec: %w", err)
 	}
diff --git a/internal/pipeline/fullrun.go b/internal/pipeline/fullrun.go
index ca6c44ae..1f0fa103 100644
--- a/internal/pipeline/fullrun.go
+++ b/internal/pipeline/fullrun.go
@@ -3,8 +3,6 @@ package pipeline
 import (
 	"encoding/json"
 	"fmt"
-	"io"
-	"net/http"
 	"os"
 	"os/exec"
 	"path/filepath"
@@ -14,6 +12,7 @@ import (
 	"github.com/mvanhorn/cli-printing-press/v4/internal/artifacts"
 	"github.com/mvanhorn/cli-printing-press/v4/internal/llmpolish"
 	"github.com/mvanhorn/cli-printing-press/v4/internal/naming"
+	"github.com/mvanhorn/cli-printing-press/v4/internal/openapi"
 	"gopkg.in/yaml.v3"
 )
 
@@ -317,19 +316,12 @@ func copySpecToOutput(specFlag, specURL, outputDir string) error {
 }
 
 // readSpecBytes fetches spec content from a URL or reads it from a local file.
+// URL sources go through openapi.LoadSpecBytes's 24h on-disk cache, the same
+// cache the primary `generate` parse path uses; in practice this means the
+// spec.json archived alongside the generated CLI matches the spec the
+// generator parsed, even when the upstream rolled between the two reads.
 func readSpecBytes(specURL string) ([]byte, error) {
-	if strings.HasPrefix(specURL, "http://") || strings.HasPrefix(specURL, "https://") {
-		resp, err := http.Get(specURL) //nolint:gosec // spec URLs are operator-provided
-		if err != nil {
-			return nil, err
-		}
-		defer func() { _ = resp.Body.Close() }()
-		if resp.StatusCode != http.StatusOK {
-			return nil, fmt.Errorf("HTTP %d fetching %s", resp.StatusCode, specURL)
-		}
-		return io.ReadAll(resp.Body)
-	}
-	return os.ReadFile(specURL)
+	return openapi.LoadSpecBytes(specURL, false, false)
 }
 
 // ensureJSON converts YAML content to JSON. If the input is already valid
diff --git a/internal/pipeline/scorecard.go b/internal/pipeline/scorecard.go
index 51c139d7..b027e8d0 100644
--- a/internal/pipeline/scorecard.go
+++ b/internal/pipeline/scorecard.go
@@ -12,6 +12,7 @@ import (
 	"strings"
 
 	"github.com/mvanhorn/cli-printing-press/v4/internal/naming"
+	"github.com/mvanhorn/cli-printing-press/v4/internal/openapi"
 	apispec "github.com/mvanhorn/cli-printing-press/v4/internal/spec"
 	"gopkg.in/yaml.v3"
 )
@@ -1535,7 +1536,7 @@ func loadOpenAPISpec(specPath string) (*openAPISpecInfo, error) {
 		return nil, nil
 	}
 
-	data, err := os.ReadFile(specPath)
+	data, err := openapi.LoadSpecBytes(specPath, false, false)
 	if err != nil {
 		return nil, fmt.Errorf("reading spec: %w", err)
 	}
diff --git a/internal/pipeline/scorecard_tier2_test.go b/internal/pipeline/scorecard_tier2_test.go
index c4d14311..230f361d 100644
--- a/internal/pipeline/scorecard_tier2_test.go
+++ b/internal/pipeline/scorecard_tier2_test.go
@@ -2,6 +2,8 @@ package pipeline
 
 import (
 	"encoding/json"
+	"net/http"
+	"net/http/httptest"
 	"os"
 	"path/filepath"
 	"strings"
@@ -10,6 +12,38 @@ import (
 	"github.com/stretchr/testify/assert"
 )
 
+// TestLoadOpenAPISpec_AcceptsHTTPURL is the regression guard for #1001:
+// scorer subcommands rejected --spec URLs because the loader called
+// os.ReadFile directly. The fix routes through openapi.LoadSpecBytes,
+// which dispatches by scheme. A URL must now load successfully on every
+// platform without a separate "curl to /tmp" workaround.
+func TestLoadOpenAPISpec_AcceptsHTTPURL(t *testing.T) {
+	t.Setenv("HOME", t.TempDir())
+
+	srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+		w.WriteHeader(http.StatusOK)
+		_, _ = w.Write([]byte(`{
+  "openapi": "3.0.0",
+  "info": {"title": "Demo", "version": "1.0"},
+  "paths": {
+    "/things": {"get": {"responses": {"200": {"description": "ok"}}}}
+  },
+  "components": {
+    "securitySchemes": {
+      "bearer_auth": {"type": "http", "scheme": "bearer"}
+    }
+  }
+}`))
+	}))
+	defer srv.Close()
+
+	info, err := loadOpenAPISpec(srv.URL)
+	assert.NoError(t, err)
+	assert.NotNil(t, info)
+	assert.Contains(t, info.Paths, "/things")
+	assert.Contains(t, info.SecuritySchemes, "bearer_auth")
+}
+
 func TestIsThinMCPDescription(t *testing.T) {
 	tests := []struct {
 		name string

← e8c03cff fix(cli): credit auth_protocol on structural OAuth surface,  ·  back to Cli Printing Press  ·  fix(cli): redact $HOME paths from publish-tree JSON artifact a3c3c653 →