[object Object]

← back to Cli Printing Press

fix(cli): correct live-check and helper emission (#1637)

8c95570116f6c223f77c5b89956073c1d0e7f81d · 2026-05-18 16:08:24 -0700 · Trevin Chow

Files touched

Diff

commit 8c95570116f6c223f77c5b89956073c1d0e7f81d
Author: Trevin Chow <trevin@trevinchow.com>
Date:   Mon May 18 16:08:24 2026 -0700

    fix(cli): correct live-check and helper emission (#1637)
---
 internal/generator/generator.go                    | 11 ++++
 internal/generator/generator_test.go               | 76 ++++++++++++++++++++++
 internal/generator/templates/helpers.go.tmpl       |  2 +
 internal/pipeline/live_check.go                    | 13 +++-
 internal/pipeline/live_check_test.go               | 17 +++++
 skills/printing-press/SKILL.md                     |  3 +
 .../embedded-paged-api/internal/cli/helpers.go     | 28 --------
 7 files changed, 121 insertions(+), 29 deletions(-)

diff --git a/internal/generator/generator.go b/internal/generator/generator.go
index 607a9056..43627132 100644
--- a/internal/generator/generator.go
+++ b/internal/generator/generator.go
@@ -618,6 +618,7 @@ type HelperFlags struct {
 	HasDataLayer       bool // CLI has a local store (sync/search) → emit provenance helpers
 	HasClientLimit     bool // at least one endpoint needs client-side limit truncation → emit truncateJSONArray
 	HasEmbeddedPaged   bool // at least one GET endpoint has detected embedded paged sub-resources → emit fetchEmbeddedPagedSubresource
+	HasResponseUnwrap  bool // at least one generated command can call extractResponseData
 }
 
 // computeHelperFlags scans the spec's resources to determine which helpers are needed.
@@ -1488,6 +1489,7 @@ func (g *Generator) renderSingleFiles() error {
 		case "helpers.go.tmpl":
 			hFlags := computeHelperFlags(g.Spec)
 			hFlags.HasDataLayer = g.VisionSet.Store
+			hFlags.HasResponseUnwrap = g.VisionSet.Store && promotedCommandsCanUnwrapResponse(g.PromotedCommands)
 			data = &helpersTemplateData{
 				APISpec:     g.Spec,
 				HelperFlags: hFlags,
@@ -1785,6 +1787,15 @@ func buildPromotedCommandPlan(apiSpec *spec.APISpec) ([]PromotedCommand, map[str
 	return promotedCommands, promotedResourceNames, promotedEndpointNames
 }
 
+func promotedCommandsCanUnwrapResponse(commands []PromotedCommand) bool {
+	for _, cmd := range commands {
+		if !cmd.Endpoint.UsesBinaryResponse() {
+			return true
+		}
+	}
+	return false
+}
+
 func (g *Generator) renderResourceCommands(promotedResourceNames map[string]bool, promotedEndpointNames map[string]string) error {
 	// When the spec emits promoted commands, the generator also emits the api
 	// browser (api_discovery.go), whose RunE filters root.Commands() by
diff --git a/internal/generator/generator_test.go b/internal/generator/generator_test.go
index c6ca1cbb..8588ec01 100644
--- a/internal/generator/generator_test.go
+++ b/internal/generator/generator_test.go
@@ -5656,17 +5656,93 @@ func TestGeneratedOutput_PromotedCommandExists(t *testing.T) {
 
 	outputDir := filepath.Join(t.TempDir(), "promtest-pp-cli")
 	gen := New(apiSpec, outputDir)
+	gen.VisionSet = VisionTemplateSet{Store: true}
 	require.NoError(t, gen.Generate())
 
 	// Promoted command file SHOULD exist — it provides a user-friendly shortcut.
 	promotedFile := filepath.Join(outputDir, "internal", "cli", "promoted_users.go")
 	assert.FileExists(t, promotedFile)
 
+	helpersSrc, err := os.ReadFile(filepath.Join(outputDir, "internal", "cli", "helpers.go"))
+	require.NoError(t, err)
+	assert.Contains(t, string(helpersSrc), "func extractResponseData(",
+		"promoted commands call extractResponseData, so helpers.go must emit it when a promoted command exists")
+
 	// The resource parent command should NOT be generated — the promoted command replaces it.
 	// Generating both would leave the parent as dead code (never wired to root).
 	assert.NoFileExists(t, filepath.Join(outputDir, "internal", "cli", "users.go"))
 }
 
+func TestGeneratedOutput_ExtractResponseDataHelperOnlyForPromotedCommands(t *testing.T) {
+	t.Parallel()
+
+	apiSpec := &spec.APISpec{
+		Name:    "noprom",
+		Version: "0.1.0",
+		BaseURL: "https://api.example.com",
+		Auth:    spec.AuthConfig{Type: "api_key", Header: "X-Api-Key", EnvVars: []string{"NO_PROM_API_KEY"}},
+		Config:  spec.ConfigSpec{Format: "toml", Path: "~/.config/noprom-pp-cli/config.toml"},
+		Resources: map[string]spec.Resource{
+			"users": {
+				Description: "Manage users",
+				Endpoints: map[string]spec.Endpoint{
+					"list": {Method: "GET", Path: "/users", Description: "List users"},
+					"get": {
+						Method:      "GET",
+						Path:        "/users/{id}",
+						Description: "Get user",
+						Params:      []spec.Param{{Name: "id", Type: "string", Required: true, Positional: true}},
+					},
+				},
+			},
+		},
+	}
+
+	outputDir := filepath.Join(t.TempDir(), "noprom-pp-cli")
+	gen := New(apiSpec, outputDir)
+	require.NoError(t, gen.Generate())
+
+	helpersSrc, err := os.ReadFile(filepath.Join(outputDir, "internal", "cli", "helpers.go"))
+	require.NoError(t, err)
+	assert.NotContains(t, string(helpersSrc), "func extractResponseData(",
+		"helpers.go must not emit promoted-command-only helpers when no promoted command can call them")
+}
+
+func TestGeneratedOutput_ExtractResponseDataHelperSkippedForPromotedNoStoreCLI(t *testing.T) {
+	t.Parallel()
+
+	apiSpec := &spec.APISpec{
+		Name:    "promnostore",
+		Version: "0.1.0",
+		BaseURL: "https://api.example.com",
+		Auth:    spec.AuthConfig{Type: "api_key", Header: "X-Api-Key", EnvVars: []string{"PROM_NO_STORE_API_KEY"}},
+		Config:  spec.ConfigSpec{Format: "toml", Path: "~/.config/promnostore-pp-cli/config.toml"},
+		Resources: map[string]spec.Resource{
+			"users": {
+				Description: "Manage users",
+				Endpoints: map[string]spec.Endpoint{
+					"list": {Method: "GET", Path: "/users", Description: "List all users"},
+				},
+			},
+		},
+	}
+
+	outputDir := filepath.Join(t.TempDir(), "promnostore-pp-cli")
+	gen := New(apiSpec, outputDir)
+	gen.VisionSet = VisionTemplateSet{MCP: true}
+	require.NoError(t, gen.Generate())
+
+	promotedSrc, err := os.ReadFile(filepath.Join(outputDir, "internal", "cli", "promoted_users.go"))
+	require.NoError(t, err)
+	assert.NotContains(t, string(promotedSrc), "extractResponseData(",
+		"promoted commands without a local store do not call the unwrap helper")
+
+	helpersSrc, err := os.ReadFile(filepath.Join(outputDir, "internal", "cli", "helpers.go"))
+	require.NoError(t, err)
+	assert.NotContains(t, string(helpersSrc), "func extractResponseData(",
+		"helpers.go must not emit extractResponseData when promoted commands cannot call it")
+}
+
 func TestGeneratedOutput_PromotedCommandKeepsSubresourceParents(t *testing.T) {
 	t.Parallel()
 
diff --git a/internal/generator/templates/helpers.go.tmpl b/internal/generator/templates/helpers.go.tmpl
index fbe97902..08341f49 100644
--- a/internal/generator/templates/helpers.go.tmpl
+++ b/internal/generator/templates/helpers.go.tmpl
@@ -1206,6 +1206,7 @@ func printOutputWithFlags(w io.Writer, data json.RawMessage, flags *rootFlags) e
 	return printOutput(w, data, flags.asJSON)
 }
 
+{{- if .HasResponseUnwrap}}
 // extractResponseData unwraps common API response envelopes for display.
 // Many APIs return {"status":"success","data":[...]} instead of a bare array.
 // This extracts the inner data for output helpers (filterFields, compactFields,
@@ -1233,6 +1234,7 @@ func extractResponseData(data json.RawMessage) json.RawMessage {
 		return data
 	}
 }
+{{- end}}
 
 // compactVerboseListFields are prose-shaped fields stripped from list-item
 // projections. On lists, "body"/"content"/"html"/"markdown" are verbose
diff --git a/internal/pipeline/live_check.go b/internal/pipeline/live_check.go
index b26007b0..c8534ef8 100644
--- a/internal/pipeline/live_check.go
+++ b/internal/pipeline/live_check.go
@@ -258,7 +258,7 @@ func resolveBinaryPath(cliDir, name string) (string, error) {
 		if err != nil {
 			continue
 		}
-		if info.Mode()&0o111 == 0 {
+		if !isLiveCheckExecutable(path, info.Mode()) {
 			return "", fmt.Errorf("binary %q is not executable", path)
 		}
 		return path, nil
@@ -266,6 +266,17 @@ func resolveBinaryPath(cliDir, name string) (string, error) {
 	return "", fmt.Errorf("no runnable binary found in %q (tried %v)", cliDir, candidates)
 }
 
+func isLiveCheckExecutable(path string, mode os.FileMode) bool {
+	return isLiveCheckExecutableForGOOS(path, mode, runtime.GOOS)
+}
+
+func isLiveCheckExecutableForGOOS(path string, mode os.FileMode, goos string) bool {
+	if goos == "windows" {
+		return strings.EqualFold(filepath.Ext(path), ".exe")
+	}
+	return mode&0o111 != 0
+}
+
 func liveCheckBinaryCandidates(cliDir, name string) []string {
 	return liveCheckBinaryCandidatesForGOOS(cliDir, name, runtime.GOOS)
 }
diff --git a/internal/pipeline/live_check_test.go b/internal/pipeline/live_check_test.go
index 85169a8a..a6f2fb7f 100644
--- a/internal/pipeline/live_check_test.go
+++ b/internal/pipeline/live_check_test.go
@@ -536,6 +536,23 @@ func TestLiveCheckBinaryCandidatesPreferBuildStageBin(t *testing.T) {
 		"staged build/stage/bin path must be tried before cliDir legacy path, got order %v", cands)
 }
 
+func TestLiveCheckExecutableHonorsWindowsExeExtension(t *testing.T) {
+	t.Parallel()
+
+	assert.True(t,
+		isLiveCheckExecutableForGOOS(`C:\tmp\petstore-pp-cli.exe`, 0o644, "windows"),
+		"Windows executability is extension-based, not POSIX mode-bit-based")
+	assert.False(t,
+		isLiveCheckExecutableForGOOS(`C:\tmp\petstore-pp-cli`, 0o755, "windows"),
+		"Windows live-check should only accept .exe binaries")
+	assert.True(t,
+		isLiveCheckExecutableForGOOS("/tmp/petstore-pp-cli", 0o755, "linux"),
+		"Unix live-check should keep honoring executable bits")
+	assert.False(t,
+		isLiveCheckExecutableForGOOS("/tmp/petstore-pp-cli", 0o644, "linux"),
+		"Unix non-executable files must still be rejected")
+}
+
 func TestLiveCheck_FindsBinaryInBuildStageBin(t *testing.T) {
 	// Verify that RunLiveCheck finds and executes a binary placed at
 	// <cliDir>/build/stage/bin/<name> — the canonical layout written by the
diff --git a/skills/printing-press/SKILL.md b/skills/printing-press/SKILL.md
index 98a1af2d..eb412bc9 100644
--- a/skills/printing-press/SKILL.md
+++ b/skills/printing-press/SKILL.md
@@ -2495,6 +2495,9 @@ func newXxxCmd(flags *rootFlags) *cobra.Command {
 //   issuesCmd := newIssuesCmd(flags)
 //   issuesCmd.AddCommand(newIssuesStaleCmd(flags))
 //   rootCmd.AddCommand(issuesCmd)
+// Leaf commands must declare every non-root flag used in their examples.
+// Do not rely on parent-local flags like --org or --project being accepted by
+// child commands unless the parent registered them with PersistentFlags().
 // Single-word Commands register directly: rootCmd.AddCommand(newXxxCmd(flags)).
 ```
 
diff --git a/testdata/golden/expected/generate-embedded-paged-api/embedded-paged-api/internal/cli/helpers.go b/testdata/golden/expected/generate-embedded-paged-api/embedded-paged-api/internal/cli/helpers.go
index e7a4de48..920be6d4 100644
--- a/testdata/golden/expected/generate-embedded-paged-api/embedded-paged-api/internal/cli/helpers.go
+++ b/testdata/golden/expected/generate-embedded-paged-api/embedded-paged-api/internal/cli/helpers.go
@@ -1005,34 +1005,6 @@ func printOutputWithFlags(w io.Writer, data json.RawMessage, flags *rootFlags) e
 	return printOutput(w, data, flags.asJSON)
 }
 
-// extractResponseData unwraps common API response envelopes for display.
-// Many APIs return {"status":"success","data":[...]} instead of a bare array.
-// This extracts the inner data for output helpers (filterFields, compactFields,
-// printAutoTable) that expect arrays or flat objects.
-//
-// Only unwraps when a "status" field is present and indicates success — this
-// avoids false positives on APIs where "data" is a regular field (e.g., Stripe
-// returns {"data":[...],"has_more":true} where "data" is the list, not an
-// envelope wrapper).
-func extractResponseData(data json.RawMessage) json.RawMessage {
-	var envelope struct {
-		Status string          `json:"status"`
-		Data   json.RawMessage `json:"data"`
-	}
-	if err := json.Unmarshal(data, &envelope); err != nil {
-		return data
-	}
-	if envelope.Data == nil || envelope.Status == "" {
-		return data // No status field = not an envelope, might be regular "data" field
-	}
-	switch envelope.Status {
-	case "success", "ok", "OK", "Success":
-		return envelope.Data
-	default:
-		return data
-	}
-}
-
 // compactVerboseListFields are prose-shaped fields stripped from list-item
 // projections. On lists, "body"/"content"/"html"/"markdown" are verbose
 // noise and the row's identity is carried by id/name/title/etc.

← af9ba8c7 docs(skills): document separate-file pattern for extending e  ·  back to Cli Printing Press  ·  fix(cli): harden browser-sniff PII placeholders (#1639) 7eee7dfc →