[object Object]

← back to Cli Printing Press

fix(cli): populate printer + run_id in generated .printing-press.json (#1442)

a1098f1563e66fec8fa61a2b04c77cce5f041c00 · 2026-05-15 11:50:20 -0700 · Trevin Chow

* fix(cli): populate printer + run_id in generated .printing-press.json

Standalone `printing-press generate` left both fields empty by default, so
publish-validate's manifest contract failed on every freshly generated CLI
that polish hadn't hand-patched.

- resolvePrinterForNew now falls back to `gh api user --jq .login` when
  `git config github.user` is unset, covering machines where the user has
  authenticated gh without setting the git config.
- WriteManifestForGenerate auto-fills an empty RunID with a fresh
  YYYYMMDD-HHMMSS so the manifest always has a non-empty run_id. Phase 5
  callers still need the research-dir-derived ID and root.go keeps its
  --research-dir warning for that case.
- Golden harness normalizes run_id alongside generated_at; the five
  affected fixtures regain a stable shape.

Closes #1348

* fix(cli): harden printer fallback against gh null output and unisolated PATH

Address Greptile P2 review on #1442:

- Reject the literal string "null" from `gh api user --jq .login` (jq emits
  this when the API response lacks `.login` or returns JSON null); without
  the filter the printer field would be set to "null" and still fail
  publish-validate the same way an empty printer does.
- stubBinaryDir now prepends to PATH instead of replacing it entirely so
  TestGenerateLeavesPrinterEmptyWhenGitHandleUnset (which calls the broader
  gen.Generate(), not just resolvePrinterForNew) keeps non-stubbed binaries
  reachable.
- Add TestResolvePrinterForNewRejectsGhNullOutput.

Files touched

Diff

commit a1098f1563e66fec8fa61a2b04c77cce5f041c00
Author: Trevin Chow <trevin@trevinchow.com>
Date:   Fri May 15 11:50:20 2026 -0700

    fix(cli): populate printer + run_id in generated .printing-press.json (#1442)
    
    * fix(cli): populate printer + run_id in generated .printing-press.json
    
    Standalone `printing-press generate` left both fields empty by default, so
    publish-validate's manifest contract failed on every freshly generated CLI
    that polish hadn't hand-patched.
    
    - resolvePrinterForNew now falls back to `gh api user --jq .login` when
      `git config github.user` is unset, covering machines where the user has
      authenticated gh without setting the git config.
    - WriteManifestForGenerate auto-fills an empty RunID with a fresh
      YYYYMMDD-HHMMSS so the manifest always has a non-empty run_id. Phase 5
      callers still need the research-dir-derived ID and root.go keeps its
      --research-dir warning for that case.
    - Golden harness normalizes run_id alongside generated_at; the five
      affected fixtures regain a stable shape.
    
    Closes #1348
    
    * fix(cli): harden printer fallback against gh null output and unisolated PATH
    
    Address Greptile P2 review on #1442:
    
    - Reject the literal string "null" from `gh api user --jq .login` (jq emits
      this when the API response lacks `.login` or returns JSON null); without
      the filter the printer field would be set to "null" and still fail
      publish-validate the same way an empty printer does.
    - stubBinaryDir now prepends to PATH instead of replacing it entirely so
      TestGenerateLeavesPrinterEmptyWhenGitHandleUnset (which calls the broader
      gen.Generate(), not just resolvePrinterForNew) keeps non-stubbed binaries
      reachable.
    - Add TestResolvePrinterForNewRejectsGhNullOutput.
---
 internal/generator/plan_generate.go                | 19 +++++-
 internal/generator/printer_test.go                 | 71 +++++++++++++++++++++-
 internal/pipeline/climanifest.go                   | 18 +++++-
 internal/pipeline/climanifest_test.go              | 10 ++-
 scripts/golden.sh                                  |  5 ++
 .../cafe-bistro/.printing-press.json               |  1 +
 .../printing-press-golden/.printing-press.json     |  1 +
 .../mcp-cloudflare/.printing-press.json            |  1 +
 .../sync-walker-golden/.printing-press.json        |  1 +
 .../tier-routing-golden/.printing-press.json       |  1 +
 10 files changed, 117 insertions(+), 11 deletions(-)

diff --git a/internal/generator/plan_generate.go b/internal/generator/plan_generate.go
index 17c1de2d..42d1bdf2 100644
--- a/internal/generator/plan_generate.go
+++ b/internal/generator/plan_generate.go
@@ -423,10 +423,23 @@ func readManifestPrinter(outputDir string) string {
 	return readManifestField(outputDir, "printer")
 }
 
-// resolvePrinterForNew returns "" instead of a sentinel when github.user is unset.
+// resolvePrinterForNew returns the printer @handle for a brand-new print.
+// Tries `git config github.user`, then `gh api user --jq .login` so a
+// logged-in `gh` covers machines without `git config github.user`. Returns
+// "" instead of a sentinel when both are unset.
 func resolvePrinterForNew() string {
-	if out, err := exec.Command("git", "config", "github.user").Output(); err == nil && len(out) > 0 {
-		return strings.TrimSpace(string(out))
+	if out, err := exec.Command("git", "config", "github.user").Output(); err == nil {
+		if v := strings.TrimSpace(string(out)); v != "" {
+			return v
+		}
+	}
+	if out, err := exec.Command("gh", "api", "user", "--jq", ".login").Output(); err == nil {
+		// jq emits the literal string "null" when .login is absent or JSON null;
+		// filter that explicitly so it doesn't survive into the printer field
+		// and re-fail publish-validate the way an empty printer would.
+		if v := strings.TrimSpace(string(out)); v != "" && v != "null" {
+			return v
+		}
 	}
 	return ""
 }
diff --git a/internal/generator/printer_test.go b/internal/generator/printer_test.go
index c268d83f..4ac6fafc 100644
--- a/internal/generator/printer_test.go
+++ b/internal/generator/printer_test.go
@@ -3,12 +3,31 @@ package generator
 import (
 	"os"
 	"path/filepath"
+	"runtime"
 	"testing"
 
 	"github.com/stretchr/testify/assert"
 	"github.com/stretchr/testify/require"
 )
 
+// stubBinaryDir creates a temp dir with fake `git` and `gh` shell scripts and
+// prepends it to PATH so the stubs shadow real binaries but other binaries
+// remain reachable. ghScript is the body emitted by the `gh` stub (must
+// include a shebang); pass an empty string to omit gh so exec.LookPath fails.
+func stubBinaryDir(t *testing.T, gitScript, ghScript string) string {
+	t.Helper()
+	if runtime.GOOS == "windows" {
+		t.Skip("fake shell scripts are Unix-only")
+	}
+	dir := t.TempDir()
+	require.NoError(t, os.WriteFile(filepath.Join(dir, "git"), []byte(gitScript), 0o755))
+	if ghScript != "" {
+		require.NoError(t, os.WriteFile(filepath.Join(dir, "gh"), []byte(ghScript), 0o755))
+	}
+	t.Setenv("PATH", dir+string(os.PathListSeparator)+os.Getenv("PATH"))
+	return dir
+}
+
 // TestResolvePrinterForExistingManifestWins pins the regen-preservation tier.
 func TestResolvePrinterForExistingManifestWins(t *testing.T) {
 	dir := t.TempDir()
@@ -84,9 +103,14 @@ func TestReadManifestPrinterNameHandlesMissingFile(t *testing.T) {
 
 // TestGenerateLeavesPrinterEmptyWhenGitHandleUnset prevents owner-slug false positives.
 func TestGenerateLeavesPrinterEmptyWhenGitHandleUnset(t *testing.T) {
-	t.Setenv("GIT_CONFIG_COUNT", "1")
-	t.Setenv("GIT_CONFIG_KEY_0", "github.user")
-	t.Setenv("GIT_CONFIG_VALUE_0", "")
+	// Stub both git (returns empty for `git config github.user`) and gh
+	// (exits non-zero — simulates unauthenticated/missing gh) so resolution
+	// falls through to "". Without the gh stub a logged-in test runner
+	// would now pick up the gh fallback and flip the assertion.
+	stubBinaryDir(t,
+		"#!/bin/sh\nexit 0\n",
+		"#!/bin/sh\necho 'not authenticated' >&2\nexit 1\n",
+	)
 
 	apiSpec := minimalSpec("self-owned")
 	outputDir := filepath.Join(t.TempDir(), "self-owned-pp-cli")
@@ -95,3 +119,44 @@ func TestGenerateLeavesPrinterEmptyWhenGitHandleUnset(t *testing.T) {
 
 	assert.Empty(t, gen.Spec.Printer)
 }
+
+// TestResolvePrinterForNewFallsBackToGhWhenGitUnset pins the gh fallback.
+// Covers the common case where `git config github.user` is unset but `gh`
+// is logged in — without this fallback, every fresh print on such a machine
+// emits an empty printer and fails publish-validate's manifest contract.
+func TestResolvePrinterForNewFallsBackToGhWhenGitUnset(t *testing.T) {
+	stubBinaryDir(t,
+		"#!/bin/sh\nexit 0\n",
+		"#!/bin/sh\necho ghfallback-user\n",
+	)
+	assert.Equal(t, "ghfallback-user", resolvePrinterForNew())
+}
+
+// TestResolvePrinterForNewPrefersGitConfigOverGh pins resolution order.
+func TestResolvePrinterForNewPrefersGitConfigOverGh(t *testing.T) {
+	stubBinaryDir(t,
+		"#!/bin/sh\necho git-handle\n",
+		"#!/bin/sh\necho gh-handle\n",
+	)
+	assert.Equal(t, "git-handle", resolvePrinterForNew())
+}
+
+// TestResolvePrinterForNewReturnsEmptyWhenBothUnset pins the empty fallthrough.
+func TestResolvePrinterForNewReturnsEmptyWhenBothUnset(t *testing.T) {
+	stubBinaryDir(t,
+		"#!/bin/sh\nexit 1\n",
+		"#!/bin/sh\nexit 1\n",
+	)
+	assert.Empty(t, resolvePrinterForNew())
+}
+
+// TestResolvePrinterForNewRejectsGhNullOutput guards against jq emitting the
+// literal string "null" when the API response lacks `.login` — accepting it
+// would set printer to "null" and still fail publish-validate downstream.
+func TestResolvePrinterForNewRejectsGhNullOutput(t *testing.T) {
+	stubBinaryDir(t,
+		"#!/bin/sh\nexit 0\n",
+		"#!/bin/sh\necho null\n",
+	)
+	assert.Empty(t, resolvePrinterForNew())
+}
diff --git a/internal/pipeline/climanifest.go b/internal/pipeline/climanifest.go
index ce70ca61..56fc218d 100644
--- a/internal/pipeline/climanifest.go
+++ b/internal/pipeline/climanifest.go
@@ -540,6 +540,10 @@ type GenerateManifestParams struct {
 // a real run_id; otherwise fall back to empty (and warn at the call site).
 var runIDPattern = regexp.MustCompile(`^\d{8}-\d{6}$`)
 
+// runIDTimeFormat is the canonical YYYYMMDD-HHMMSS layout matched by
+// runIDPattern. Kept as a const so the format and pattern can't drift.
+const runIDTimeFormat = "20060102-150405"
+
 // DeriveRunIDFromResearchDir extracts a canonical run_id from a research-dir
 // path, or returns "" when no valid run_id can be derived. The standalone
 // generate command does not load a PipelineState, so it cannot reach
@@ -559,14 +563,24 @@ func DeriveRunIDFromResearchDir(researchDir string) string {
 // WriteManifestForGenerate writes a .printing-press.json manifest into the
 // generated CLI directory. This is the generate-command counterpart of
 // writeCLIManifestForPublish (which operates on PipelineState).
+//
+// An empty p.RunID is auto-filled with a fresh timestamp so the emitted
+// manifest satisfies publish-validate's required-run_id contract. Phase 5
+// dogfood acceptance still needs the original research-dir-derived run_id,
+// and the root.go --research-dir warning informs phase5 callers of that gap.
 func WriteManifestForGenerate(p GenerateManifestParams) error {
+	now := time.Now().UTC()
+	runID := p.RunID
+	if runID == "" {
+		runID = now.Format(runIDTimeFormat)
+	}
 	m := CLIManifest{
 		SchemaVersion:        CurrentCLIManifestSchemaVersion,
-		GeneratedAt:          time.Now().UTC(),
+		GeneratedAt:          now,
 		PrintingPressVersion: version.Version,
 		APIName:              p.APIName,
 		CLIName:              naming.CLI(p.APIName),
-		RunID:                p.RunID,
+		RunID:                runID,
 		Owner:                p.Owner,
 		Printer:              p.Printer,
 		PrinterName:          p.PrinterName,
diff --git a/internal/pipeline/climanifest_test.go b/internal/pipeline/climanifest_test.go
index 18d00e76..d577ec25 100644
--- a/internal/pipeline/climanifest_test.go
+++ b/internal/pipeline/climanifest_test.go
@@ -596,7 +596,7 @@ func TestWriteManifestForGenerateStampsRunID(t *testing.T) {
 	assert.Equal(t, "20260504-190931", got.RunID)
 }
 
-func TestWriteManifestForGenerateOmitsEmptyRunID(t *testing.T) {
+func TestWriteManifestForGenerateAutoFillsEmptyRunID(t *testing.T) {
 	dir := t.TempDir()
 
 	err := WriteManifestForGenerate(GenerateManifestParams{
@@ -607,8 +607,12 @@ func TestWriteManifestForGenerateOmitsEmptyRunID(t *testing.T) {
 
 	data, err := os.ReadFile(filepath.Join(dir, CLIManifestFilename))
 	require.NoError(t, err)
-	// run_id has the omitempty tag; empty value must not appear in serialized JSON.
-	assert.NotContains(t, string(data), `"run_id"`)
+	// publish-validate requires run_id to be non-empty. When the caller has no
+	// research-dir-derived run_id, the emitter falls back to a fresh
+	// YYYYMMDD-HHMMSS so the manifest contract still holds.
+	var got CLIManifest
+	require.NoError(t, json.Unmarshal(data, &got))
+	assert.Regexp(t, `^\d{8}-\d{6}$`, got.RunID)
 }
 
 func TestDeriveRunIDFromResearchDir(t *testing.T) {
diff --git a/scripts/golden.sh b/scripts/golden.sh
index ca7f863c..6786dd6f 100755
--- a/scripts/golden.sh
+++ b/scripts/golden.sh
@@ -41,9 +41,14 @@ normalize_json() {
 
   case "$file" in
     */.printing-press.json)
+      # run_id is auto-filled from time.Now() when the caller doesn't supply
+      # one (the standalone `generate` path the golden harness exercises).
+      # Normalize alongside generated_at / printing_press_version so the
+      # golden doesn't drift on every run.
       jq -S '
         .generated_at = "<GENERATED_AT>"
         | .printing_press_version = "<PRINTING_PRESS_VERSION>"
+        | (if has("run_id") then .run_id = "<RUN_ID>" else . end)
       ' "$file" | normalize_text
       ;;
     */manifest.json)
diff --git a/testdata/golden/expected/generate-golden-api-unicode/cafe-bistro/.printing-press.json b/testdata/golden/expected/generate-golden-api-unicode/cafe-bistro/.printing-press.json
index 72b0e9e6..f4778311 100644
--- a/testdata/golden/expected/generate-golden-api-unicode/cafe-bistro/.printing-press.json
+++ b/testdata/golden/expected/generate-golden-api-unicode/cafe-bistro/.printing-press.json
@@ -15,6 +15,7 @@
   "printer": "printing-press-golden",
   "printer_name": "printing-press-golden",
   "printing_press_version": "<PRINTING_PRESS_VERSION>",
+  "run_id": "<RUN_ID>",
   "schema_version": 1,
   "spec_checksum": "sha256:8cfba131c384836e8dde461511c9fd63cf2cbab7793553067765d515f2007aa1",
   "spec_format": "openapi3",
diff --git a/testdata/golden/expected/generate-golden-api/printing-press-golden/.printing-press.json b/testdata/golden/expected/generate-golden-api/printing-press-golden/.printing-press.json
index b1ab3d6f..e37a040d 100644
--- a/testdata/golden/expected/generate-golden-api/printing-press-golden/.printing-press.json
+++ b/testdata/golden/expected/generate-golden-api/printing-press-golden/.printing-press.json
@@ -16,6 +16,7 @@
   "printer": "printing-press-golden",
   "printer_name": "printing-press-golden",
   "printing_press_version": "<PRINTING_PRESS_VERSION>",
+  "run_id": "<RUN_ID>",
   "schema_version": 1,
   "spec_checksum": "sha256:2ea5c937580b3011c022fa9fcbbd19cba6be4a38a5303c66905cb9bb736991fb",
   "spec_format": "openapi3",
diff --git a/testdata/golden/expected/generate-mcp-api/mcp-cloudflare/.printing-press.json b/testdata/golden/expected/generate-mcp-api/mcp-cloudflare/.printing-press.json
index ca3ffc18..8305799a 100644
--- a/testdata/golden/expected/generate-mcp-api/mcp-cloudflare/.printing-press.json
+++ b/testdata/golden/expected/generate-mcp-api/mcp-cloudflare/.printing-press.json
@@ -15,6 +15,7 @@
   "printer": "printing-press-golden",
   "printer_name": "printing-press-golden",
   "printing_press_version": "<PRINTING_PRESS_VERSION>",
+  "run_id": "<RUN_ID>",
   "schema_version": 1,
   "spec_checksum": "sha256:46c60ba489f2e138ffe6ab01531f2ed550369299820342a7f0093b94d0fc17b4",
   "spec_format": "openapi3",
diff --git a/testdata/golden/expected/generate-sync-walker-api/sync-walker-golden/.printing-press.json b/testdata/golden/expected/generate-sync-walker-api/sync-walker-golden/.printing-press.json
index 1d7d8a71..2f28ce4b 100644
--- a/testdata/golden/expected/generate-sync-walker-api/sync-walker-golden/.printing-press.json
+++ b/testdata/golden/expected/generate-sync-walker-api/sync-walker-golden/.printing-press.json
@@ -15,6 +15,7 @@
   "printer": "printing-press-golden",
   "printer_name": "printing-press-golden",
   "printing_press_version": "<PRINTING_PRESS_VERSION>",
+  "run_id": "<RUN_ID>",
   "schema_version": 1,
   "spec_checksum": "sha256:2aa39c064cd068cb090102e22a446f9542fa06f75453d52e4fa9db5c21f27809",
   "spec_format": "internal",
diff --git a/testdata/golden/expected/generate-tier-routing-api/tier-routing-golden/.printing-press.json b/testdata/golden/expected/generate-tier-routing-api/tier-routing-golden/.printing-press.json
index 23f1bde6..c9158eb8 100644
--- a/testdata/golden/expected/generate-tier-routing-api/tier-routing-golden/.printing-press.json
+++ b/testdata/golden/expected/generate-tier-routing-api/tier-routing-golden/.printing-press.json
@@ -18,6 +18,7 @@
   "printer": "printing-press-golden",
   "printer_name": "printing-press-golden",
   "printing_press_version": "<PRINTING_PRESS_VERSION>",
+  "run_id": "<RUN_ID>",
   "schema_version": 1,
   "spec_checksum": "sha256:a059b4da997e0e835f0cdcd911ef5894fc048b50506bdcd90260a7478efb2958",
   "spec_format": "internal",

← 9d70aee4 chore(ci): add CODEOWNERS for workflows and CODEOWNERS itsel  ·  back to Cli Printing Press  ·  fix(cli): validate &&-chained narrative recipes segment-by-s bb5e2def →