← back to Cli Printing Press

fix(cli): unbreak HasStore + non-GET promoted commands (#425) (#459)

a3ea32edaf982e8d258339dbf2c0b006ea53c4ae · 2026-05-01 09:02:09 -0700 · Trevin Chow

* fix(generator): route HasStore + non-GET promoted commands through verb branch (#425)

Promoted-command template called resolveRead unconditionally when
HasStore was true, but resolveRead is GET-only internally. For a
HasStore + POST endpoint (e.g., postman-explore-pp-cli's search-all),
the generator emitted code that built a `params` map and called
resolveRead with no body — silently broken: the live-API path tried
to GET a POST-only endpoint, and the cached path used a GET-shape
key against a store that never wrote one.

The interim fix narrows the resolveRead branch to GET only, mirroring
the typed-form pattern in command_endpoint.go.tmpl. HasStore + non-GET
now falls through to the verb branch (live API call via c.Post / c.Put
/ c.Patch / c.Delete) and synthesizes a `prov := attachFreshness(
DataProvenance{Source: "live"}, flags)` so the downstream HasStore
provenance UI block keeps compiling and producing consistent JSON
output.

The full fix — a body-aware `resolvePostRead` helper that hashes the
canonical JSON body into a cache key — is left for #425's follow-on
work, when a second store-backed POST-search consumer materializes
to inform the cache-key design. Today only postman-explore-pp-cli
exercises this combination, and live-call POST is no worse than the
typed `<resource> <endpoint>` form which has the same behavior.

New tests:
- TestPromotedHasStorePostRoutesThroughVerbBranch: HasStore + POST
  emits c.Post(path, body) and does NOT emit resolveRead
- TestPromotedHasStoreGetStillUsesResolveRead: HasStore + GET keeps
  routing through resolveRead (the cached fast-path)

Goldens updated: only the printing-press-golden fixture's promoted_public.go
loses three lines of stale comment that no longer describe the template
behavior. The generated code itself is byte-identical for the GET case.

* refactor(generator): simplify pass — hoist prov synthesis + add DELETE test

Three follow-ups from the post-fix review:

1. **Hoist `prov := attachFreshness(...)` synthesis.** The previous version
   inlined the same line in both the DELETE branch and the POST/PUT/PATCH
   branch — exactly the lockstep-edit hazard /simplify exists to surface.
   Move it to one block after the verb-chain ladder, gated on
   `{{- if and .HasStore (ne (upper .Endpoint.Method) "GET") -}}`. Cuts six
   template lines and removes the duplication.

2. **Add `TestPromotedHasStoreDeleteSynthesizesProv`.** The DELETE +
   HasStore path had zero direct test coverage — `TestGenerateProjectsCompile`
   fixtures all run with `VisionSet.Store = false`. Mirrors the POST test
   shape and asserts c.Delete + the synthesized prov line.

3. **Trim test doc comments.** The original comments narrated WHAT the
   tests do; the test names already cover that. Trim each to ~3-4 lines
   covering the non-obvious WHY (interim-fix scope, byte-compat intent,
   why DELETE coverage is its own case).

Files touched

• A internal/generator/promoted_poststore_test.go
• M internal/generator/templates/command_promoted.go.tmpl
• M testdata/golden/expected/generate-golden-api/printing-press-golden/internal/cli/promoted_public.go

Diff

commit a3ea32edaf982e8d258339dbf2c0b006ea53c4ae
Author: Trevin Chow <trevin@trevinchow.com>
Date:   Fri May 1 09:02:09 2026 -0700

    fix(cli): unbreak HasStore + non-GET promoted commands (#425) (#459)
    
    * fix(generator): route HasStore + non-GET promoted commands through verb branch (#425)
    
    Promoted-command template called resolveRead unconditionally when
    HasStore was true, but resolveRead is GET-only internally. For a
    HasStore + POST endpoint (e.g., postman-explore-pp-cli's search-all),
    the generator emitted code that built a `params` map and called
    resolveRead with no body — silently broken: the live-API path tried
    to GET a POST-only endpoint, and the cached path used a GET-shape
    key against a store that never wrote one.
    
    The interim fix narrows the resolveRead branch to GET only, mirroring
    the typed-form pattern in command_endpoint.go.tmpl. HasStore + non-GET
    now falls through to the verb branch (live API call via c.Post / c.Put
    / c.Patch / c.Delete) and synthesizes a `prov := attachFreshness(
    DataProvenance{Source: "live"}, flags)` so the downstream HasStore
    provenance UI block keeps compiling and producing consistent JSON
    output.
    
    The full fix — a body-aware `resolvePostRead` helper that hashes the
    canonical JSON body into a cache key — is left for #425's follow-on
    work, when a second store-backed POST-search consumer materializes
    to inform the cache-key design. Today only postman-explore-pp-cli
    exercises this combination, and live-call POST is no worse than the
    typed `<resource> <endpoint>` form which has the same behavior.
    
    New tests:
    - TestPromotedHasStorePostRoutesThroughVerbBranch: HasStore + POST
      emits c.Post(path, body) and does NOT emit resolveRead
    - TestPromotedHasStoreGetStillUsesResolveRead: HasStore + GET keeps
      routing through resolveRead (the cached fast-path)
    
    Goldens updated: only the printing-press-golden fixture's promoted_public.go
    loses three lines of stale comment that no longer describe the template
    behavior. The generated code itself is byte-identical for the GET case.
    
    * refactor(generator): simplify pass — hoist prov synthesis + add DELETE test
    
    Three follow-ups from the post-fix review:
    
    1. **Hoist `prov := attachFreshness(...)` synthesis.** The previous version
       inlined the same line in both the DELETE branch and the POST/PUT/PATCH
       branch — exactly the lockstep-edit hazard /simplify exists to surface.
       Move it to one block after the verb-chain ladder, gated on
       `{{- if and .HasStore (ne (upper .Endpoint.Method) "GET") -}}`. Cuts six
       template lines and removes the duplication.
    
    2. **Add `TestPromotedHasStoreDeleteSynthesizesProv`.** The DELETE +
       HasStore path had zero direct test coverage — `TestGenerateProjectsCompile`
       fixtures all run with `VisionSet.Store = false`. Mirrors the POST test
       shape and asserts c.Delete + the synthesized prov line.
    
    3. **Trim test doc comments.** The original comments narrated WHAT the
       tests do; the test names already cover that. Trim each to ~3-4 lines
       covering the non-obvious WHY (interim-fix scope, byte-compat intent,
       why DELETE coverage is its own case).
---
 internal/generator/promoted_poststore_test.go      | 135 +++++++++++++++++++++
 .../generator/templates/command_promoted.go.tmpl   |  19 ++-
 .../internal/cli/promoted_public.go                |   3 -
 3 files changed, 144 insertions(+), 13 deletions(-)

diff --git a/internal/generator/promoted_poststore_test.go b/internal/generator/promoted_poststore_test.go
new file mode 100644
index 00000000..3c362bba
--- /dev/null
+++ b/internal/generator/promoted_poststore_test.go
@@ -0,0 +1,135 @@
+package generator
+
+import (
+	"os"
+	"path/filepath"
+	"testing"
+
+	"github.com/mvanhorn/cli-printing-press/v3/internal/naming"
+	"github.com/mvanhorn/cli-printing-press/v3/internal/spec"
+	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
+)
+
+// TestPromotedHasStorePostRoutesThroughVerbBranch is #425's interim-fix
+// guard: HasStore + POST must route through c.Post (live API call) rather
+// than resolveRead, which is GET-only internally. Pre-fix the template
+// emitted resolveRead unconditionally for HasStore and produced
+// uncompilable code for non-GET endpoints.
+func TestPromotedHasStorePostRoutesThroughVerbBranch(t *testing.T) {
+	t.Parallel()
+
+	apiSpec := minimalSpec("poststore")
+	apiSpec.Resources = map[string]spec.Resource{
+		"searches": {
+			Description: "Search across resources by free-text query",
+			Endpoints: map[string]spec.Endpoint{
+				"searchAll": {
+					Method:      "POST",
+					Path:        "/search-all",
+					Description: "Search by free-text query across all entities",
+					Body: []spec.Param{
+						{Name: "query", Type: "string", Description: "Free-text query"},
+					},
+				},
+			},
+		},
+	}
+
+	outputDir := filepath.Join(t.TempDir(), naming.CLI(apiSpec.Name))
+	gen := New(apiSpec, outputDir)
+	gen.VisionSet = VisionTemplateSet{Store: true}
+	require.NoError(t, gen.Generate())
+
+	promotedPath := filepath.Join(outputDir, "internal", "cli", "promoted_searches.go")
+	require.FileExists(t, promotedPath,
+		"single-endpoint POST resource with read-shaped operationId 'searchAll' must produce a promoted command")
+
+	src, err := os.ReadFile(promotedPath)
+	require.NoError(t, err)
+	got := string(src)
+
+	assert.NotContains(t, got, "resolveRead(",
+		"HasStore + POST must NOT route through resolveRead (GET-only internally)")
+	assert.Contains(t, got, "c.Post(path, body)",
+		"HasStore + POST must route through the verb branch with a built body")
+	assert.Contains(t, got, `attachFreshness(DataProvenance{Source: "live"}, flags)`,
+		"non-GET HasStore commands must synthesize a live-call prov so the downstream HasStore block compiles")
+}
+
+// TestPromotedHasStoreGetStillUsesResolveRead is the byte-compat guard for
+// the GET happy path. The golden harness also covers this; the assertion
+// is faster and more explicit when the template gate gets touched.
+func TestPromotedHasStoreGetStillUsesResolveRead(t *testing.T) {
+	t.Parallel()
+
+	apiSpec := minimalSpec("getstore")
+	apiSpec.Resources = map[string]spec.Resource{
+		"status": {
+			Description: "Public status",
+			Endpoints: map[string]spec.Endpoint{
+				"get": {
+					Method:      "GET",
+					Path:        "/status",
+					Description: "Get current status",
+				},
+			},
+		},
+	}
+
+	outputDir := filepath.Join(t.TempDir(), naming.CLI(apiSpec.Name))
+	gen := New(apiSpec, outputDir)
+	gen.VisionSet = VisionTemplateSet{Store: true}
+	require.NoError(t, gen.Generate())
+
+	promotedPath := filepath.Join(outputDir, "internal", "cli", "promoted_status.go")
+	src, err := os.ReadFile(promotedPath)
+	require.NoError(t, err)
+	got := string(src)
+
+	assert.Contains(t, got, "resolveRead(",
+		"HasStore + GET must keep routing through resolveRead (the cached fast-path)")
+	assert.NotContains(t, got, "c.Get(path, params)",
+		"HasStore + GET must not also emit a direct c.Get call (would mean both branches fired)")
+}
+
+// TestPromotedHasStoreDeleteSynthesizesProv covers the DELETE branch the
+// other tests don't reach. The provenance synthesis lives in a single
+// post-chain block, so a regression that drops it from one verb shape
+// drops it from all of them — but DELETE has its own pre-existing
+// `data, _, err := c.Delete(path)` call site, so an explicit assertion
+// guards against template refactors that re-introduce the dup.
+func TestPromotedHasStoreDeleteSynthesizesProv(t *testing.T) {
+	t.Parallel()
+
+	apiSpec := minimalSpec("delstore")
+	apiSpec.Resources = map[string]spec.Resource{
+		"sessions": {
+			Description: "Session management",
+			Endpoints: map[string]spec.Endpoint{
+				"revokeAll": {
+					Method:      "DELETE",
+					Path:        "/sessions",
+					Description: "Revoke all active sessions",
+				},
+			},
+		},
+	}
+
+	outputDir := filepath.Join(t.TempDir(), naming.CLI(apiSpec.Name))
+	gen := New(apiSpec, outputDir)
+	gen.VisionSet = VisionTemplateSet{Store: true}
+	require.NoError(t, gen.Generate())
+
+	promotedPath := filepath.Join(outputDir, "internal", "cli", "promoted_sessions.go")
+	src, err := os.ReadFile(promotedPath)
+	require.NoError(t, err)
+	got := string(src)
+
+	assert.NotContains(t, got, "resolveRead(",
+		"HasStore + DELETE must NOT route through resolveRead")
+	assert.Contains(t, got, "c.Delete(path)",
+		"HasStore + DELETE must route through c.Delete")
+	assert.Contains(t, got, `attachFreshness(DataProvenance{Source: "live"}, flags)`,
+		"HasStore + DELETE must synthesize a live-call prov for the downstream provenance block")
+}
diff --git a/internal/generator/templates/command_promoted.go.tmpl b/internal/generator/templates/command_promoted.go.tmpl
index e333edd7..f07b7253 100644
--- a/internal/generator/templates/command_promoted.go.tmpl
+++ b/internal/generator/templates/command_promoted.go.tmpl
@@ -136,7 +136,7 @@ func new{{camel .PromotedName}}PromotedCmd(flags *rootFlags) *cobra.Command {
 			}, nil, flagAll, "{{.Endpoint.Pagination.CursorParam}}", "{{.Endpoint.Pagination.NextCursorPath}}", "{{.Endpoint.Pagination.HasMoreField}}")
 {{- end}}
 {{- else}}
-{{- if or .HasStore (eq (upper .Endpoint.Method) "GET") (eq (upper .Endpoint.Method) "HEAD") (eq (upper .Endpoint.Method) "OPTIONS")}}
+{{- if or (eq (upper .Endpoint.Method) "GET") (eq (upper .Endpoint.Method) "HEAD") (eq (upper .Endpoint.Method) "OPTIONS")}}
 			params := map[string]string{}
 {{- range $i, $p := .Endpoint.Params}}
 {{- if and .Positional (not (pathContainsParam $.Endpoint.Path .Name))}}
@@ -149,21 +149,17 @@ func new{{camel .PromotedName}}PromotedCmd(flags *rootFlags) *cobra.Command {
 {{- end}}
 {{- end}}
 {{- end}}
-{{- if .HasStore}}
-			// resolveRead is GET-only internally, so HasStore + non-GET should
-			// be marked Hidden in the printed CLI and reached via the typed
-			// `<resource> <endpoint>` form instead.
+{{- if and .HasStore (eq (upper .Endpoint.Method) "GET")}}
 			data, prov, err := resolveRead(cmd.Context(), c, flags, "{{lower .ResourceName}}", {{if .Endpoint.Pagination}}true{{else}}false{{end}}, path, params, nil)
 {{- else if eq (upper .Endpoint.Method) "GET"}}
 			data, err := c.Get(path, params)
 {{- else if eq (upper .Endpoint.Method) "DELETE"}}
 			data, _, err := c.Delete(path)
 {{- else if or (eq (upper .Endpoint.Method) "POST") (eq (upper .Endpoint.Method) "PUT") (eq (upper .Endpoint.Method) "PATCH")}}
-			// Build the JSON body from --field flags. Query/path params
-			// are intentionally not folded into the body here — the typed
-			// `<resource> <endpoint>` command is the place to combine
-			// query params with body for non-GET verbs. Promoted commands
-			// cover the common case: body-only POST/PUT/PATCH.
+			// HasStore + non-GET falls through to a live API call here
+			// rather than through resolveRead (GET-only internally); a
+			// body-aware cached read helper is filed as #425 for when a
+			// second store-backed POST-search consumer ships.
 			body := map[string]any{}
 {{bodyMap .Endpoint.Body "\t\t\t"}}			data, _, err := c.{{pascal (lower .Endpoint.Method)}}(path, body)
 {{- else}}
@@ -172,6 +168,9 @@ func new{{camel .PromotedName}}PromotedCmd(flags *rootFlags) *cobra.Command {
 			// should add a typed client method and a dedicated template branch.
 			data, err := c.Get(path, params)
 {{- end}}
+{{- if and .HasStore (ne (upper .Endpoint.Method) "GET")}}
+			prov := attachFreshness(DataProvenance{Source: "live"}, flags)
+{{- end}}
 {{- end}}
 			if err != nil {
 				return classifyAPIError(err)
diff --git a/testdata/golden/expected/generate-golden-api/printing-press-golden/internal/cli/promoted_public.go b/testdata/golden/expected/generate-golden-api/printing-press-golden/internal/cli/promoted_public.go
index 558b7814..212bb3b1 100644
--- a/testdata/golden/expected/generate-golden-api/printing-press-golden/internal/cli/promoted_public.go
+++ b/testdata/golden/expected/generate-golden-api/printing-press-golden/internal/cli/promoted_public.go
@@ -27,9 +27,6 @@ func newPublicPromotedCmd(flags *rootFlags) *cobra.Command {
 
 			path := "/public/status"
 			params := map[string]string{}
-			// resolveRead is GET-only internally, so HasStore + non-GET should
-			// be marked Hidden in the printed CLI and reached via the typed
-			// `<resource> <endpoint>` form instead.
 			data, prov, err := resolveRead(cmd.Context(), c, flags, "public", false, path, params, nil)
 			if err != nil {
 				return classifyAPIError(err)

← 052187a7 chore(main): release 3.1.0 (#419)  ·  back to Cli Printing Press  ·  feat(cli): add regen-merge subcommand for library template s 4c7bd42d →