← back to Cli Printing Press
fix(cli): patch verifies target shape + runs build in target dir (#224)
86bdfd2ef5cc79b48e51fadb3bfef8ce4f1ed32c · 2026-04-18 02:53:09 -0700 · Trevin Chow
Two bugs in printing-press patch surfaced during the 21-CLI rollout:
1. The post-patch `go build` ran in the current working directory (the
printing-press repo) instead of the target CLI. Every patched CLI
showed `build_ok: true` regardless of whether the CLI actually
compiled. Fix: set cmd.Dir = opts.Dir.
2. The AST matchers silently no-op'd when root.go didn't match the
expected printing-press shape (missing rootFlags struct or
rootCmd.PersistentFlags / rootCmd.AddCommand calls). Writing drop-ins
anyway produced `undefined: rootFlags` compile failures on
agent-capture and instacart.
Fix: verify shape upfront via checkRootShape before any writes. If
rootFlags/rootCmd.PersistentFlags/rootCmd.AddCommand are missing,
flag as a "shape" collision and refuse. These CLIs need a reprint,
not a patch.
3. Collision detection only checked for existing profile.go/deliver.go/
feedback.go by filename. Steam-web has its domain `newProfileCmd` in
`cmd_profile.go` — different filename, same symbol — so the old
filename check missed it and the build failed with `newProfileCmd
redeclared`.
Fix: scan every *.go file under internal/cli for top-level `func` /
`type` declarations matching the drop-in symbols (newProfileCmd,
newFeedbackCmd, DeliverSink, etc.). Flag as a resource collision
regardless of containing filename.
Validated against the 21-CLI library cohort: 17 clean, 2 --force cases
(Pagliacci feedback collision, steam-web profile collision) both build
green, 2 correctly refused (agent-capture, instacart — shape mismatch)
with clear messaging pointing at reprint.
Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Files touched
M internal/patch/ast_inject.goM internal/patch/ast_inject_test.goM internal/patch/collisions.goM internal/patch/patch.go
Diff
commit 86bdfd2ef5cc79b48e51fadb3bfef8ce4f1ed32c
Author: Trevin Chow <trevin@trevinchow.com>
Date: Sat Apr 18 02:53:09 2026 -0700
fix(cli): patch verifies target shape + runs build in target dir (#224)
Two bugs in printing-press patch surfaced during the 21-CLI rollout:
1. The post-patch `go build` ran in the current working directory (the
printing-press repo) instead of the target CLI. Every patched CLI
showed `build_ok: true` regardless of whether the CLI actually
compiled. Fix: set cmd.Dir = opts.Dir.
2. The AST matchers silently no-op'd when root.go didn't match the
expected printing-press shape (missing rootFlags struct or
rootCmd.PersistentFlags / rootCmd.AddCommand calls). Writing drop-ins
anyway produced `undefined: rootFlags` compile failures on
agent-capture and instacart.
Fix: verify shape upfront via checkRootShape before any writes. If
rootFlags/rootCmd.PersistentFlags/rootCmd.AddCommand are missing,
flag as a "shape" collision and refuse. These CLIs need a reprint,
not a patch.
3. Collision detection only checked for existing profile.go/deliver.go/
feedback.go by filename. Steam-web has its domain `newProfileCmd` in
`cmd_profile.go` — different filename, same symbol — so the old
filename check missed it and the build failed with `newProfileCmd
redeclared`.
Fix: scan every *.go file under internal/cli for top-level `func` /
`type` declarations matching the drop-in symbols (newProfileCmd,
newFeedbackCmd, DeliverSink, etc.). Flag as a resource collision
regardless of containing filename.
Validated against the 21-CLI library cohort: 17 clean, 2 --force cases
(Pagliacci feedback collision, steam-web profile collision) both build
green, 2 correctly refused (agent-capture, instacart — shape mismatch)
with clear messaging pointing at reprint.
Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
---
internal/patch/ast_inject.go | 50 ++++++++++++++++
internal/patch/ast_inject_test.go | 29 ++++++++++
internal/patch/collisions.go | 119 ++++++++++++++++++++++++++++++++------
internal/patch/patch.go | 20 ++++++-
4 files changed, 198 insertions(+), 20 deletions(-)
diff --git a/internal/patch/ast_inject.go b/internal/patch/ast_inject.go
index fdd1d069..b89b6317 100644
--- a/internal/patch/ast_inject.go
+++ b/internal/patch/ast_inject.go
@@ -453,6 +453,56 @@ func nodeReferences(node dst.Node, name string) bool {
return found
}
+// checkRootShape verifies the target root.go matches the printing-press
+// generator's shape that the patcher's AST matchers assume. Returns an
+// empty string on match, or a human-readable mismatch reason. The drop-in
+// files reference `rootFlags` and expect AST injection to land new wiring
+// inside the `rootCmd.PersistentFlags()` / `rootCmd.AddCommand()` blocks;
+// a non-matching shape (e.g. package-global `root` with no rootFlags
+// struct, as some older synthetic CLIs have) would produce a compile
+// failure if we wrote drop-ins anyway.
+func checkRootShape(src []byte) string {
+ file, err := decorator.Parse(src)
+ if err != nil {
+ return fmt.Sprintf("root.go does not parse: %v", err)
+ }
+ hasRootFlags := false
+ hasRootCmdFlags := false
+ hasRootCmdAddCommand := false
+ dst.Inspect(file, func(n dst.Node) bool {
+ switch v := n.(type) {
+ case *dst.TypeSpec:
+ if v.Name.Name == "rootFlags" {
+ if _, ok := v.Type.(*dst.StructType); ok {
+ hasRootFlags = true
+ }
+ }
+ case *dst.ExprStmt:
+ if isPersistentFlagsCall(v) {
+ hasRootCmdFlags = true
+ }
+ if isRootAddCommand(v) {
+ hasRootCmdAddCommand = true
+ }
+ }
+ return true
+ })
+ var missing []string
+ if !hasRootFlags {
+ missing = append(missing, "rootFlags struct")
+ }
+ if !hasRootCmdFlags {
+ missing = append(missing, "rootCmd.PersistentFlags() call")
+ }
+ if !hasRootCmdAddCommand {
+ missing = append(missing, "rootCmd.AddCommand call")
+ }
+ if len(missing) == 0 {
+ return ""
+ }
+ return "root.go missing " + strings.Join(missing, ", ")
+}
+
// parseStmt parses a single Go statement and returns it as a dst.Stmt.
// Wraps the snippet in a synthetic function so go/parser accepts it.
func parseStmt(src string) dst.Stmt {
diff --git a/internal/patch/ast_inject_test.go b/internal/patch/ast_inject_test.go
index 29590924..07c82b16 100644
--- a/internal/patch/ast_inject_test.go
+++ b/internal/patch/ast_inject_test.go
@@ -149,6 +149,35 @@ func TestInjectRootAST_SkipFeedback(t *testing.T) {
"deliver-setup block must still be present")
}
+func TestCheckRootShape_MatchesBase(t *testing.T) {
+ assert.Empty(t, checkRootShape([]byte(baseRootGo)), "base fixture should match shape")
+}
+
+func TestCheckRootShape_RejectsInstacartShape(t *testing.T) {
+ // Instacart / agent-capture use a package-global `var rootCmd`, no
+ // rootFlags struct, and register PersistentFlags/AddCommand via a
+ // different receiver.
+ src := `package cli
+
+import "github.com/spf13/cobra"
+
+var jsonOutput bool
+
+var rootCmd = &cobra.Command{Use: "thing"}
+
+func init() {
+ rootCmd.PersistentFlags().BoolVar(&jsonOutput, "json", false, "")
+ rootCmd.AddCommand(newThingCmd())
+}
+
+func Execute() error { return rootCmd.Execute() }
+`
+ msg := checkRootShape([]byte(src))
+ require.NotEmpty(t, msg)
+ assert.Contains(t, msg, "rootFlags struct",
+ "must flag missing rootFlags struct")
+}
+
// TestInjectRootAST_NoPersistentFlagsBlock exercises the "refuse silently"
// path: if the target root.go doesn't have the expected shape, no mutation.
func TestInjectRootAST_NoPersistentFlagsBlock(t *testing.T) {
diff --git a/internal/patch/collisions.go b/internal/patch/collisions.go
index 2e9e5632..3b4c8fe1 100644
--- a/internal/patch/collisions.go
+++ b/internal/patch/collisions.go
@@ -5,21 +5,50 @@ import (
"fmt"
"os"
"path/filepath"
+ "regexp"
)
-// detectCollisions scans the CLI directory and root.go source for conflicts
-// with the symbols PR #218 introduces. Returns all collisions; the caller
-// decides which are fatal.
+// symbolsByFeature maps each drop-in feature to the canonical top-level Go
+// symbols it introduces. If any of these symbols is declared in an existing
+// file under internal/cli/ — even under an unexpected filename — that's a
+// resource-level collision: writing our drop-in would produce a `redeclared
+// in this block` compile error.
//
-// A resource-level collision (existing profile.go / deliver.go / feedback.go
-// whose content doesn't match the PR #218 drop-in) is fatal — patching would
-// overwrite a working spec-derived capability. An already-patched CLI
-// (matching drop-in content) is not a collision, it's idempotency.
-func detectCollisions(dir string, rootSrc []byte) ([]Collision, error) {
- var collisions []Collision
+// Only top-level func/type declarations are flagged; identifiers used
+// inside other functions are not a collision.
+var symbolsByFeature = map[string][]string{
+ "profile": {"newProfileCmd", "GetProfile", "ApplyProfileToFlags", "Profile"},
+ "deliver": {"DeliverSink", "ParseDeliverSink", "Deliver"},
+ "feedback": {"newFeedbackCmd", "FeedbackEntry"},
+}
+
+// topLevelDeclRe matches `func Name(` and `type Name ` at the start of a
+// line — where top-level declarations live after gofmt. Per-function
+// references (e.g. a call to Deliver inside some other handler) are not
+// flagged because they don't redeclare the symbol.
+var topLevelDeclRe = regexp.MustCompile(`(?m)^(?:func|type)\s+(\w+)\b`)
+// detectCollisions scans the CLI's internal/cli directory for conflicts with
+// the symbols PR #218 introduces. Returns all collisions; the caller decides
+// which are fatal.
+//
+// A resource-level collision (another file in internal/cli declaring one of
+// our drop-in's top-level symbols) is fatal — dropping our file in would
+// produce `redeclared in this block`. Excludes the three drop-in filenames
+// themselves since they may already be present from a prior patch run (in
+// which case detection is idempotency, not collision).
+func detectCollisions(dir string, rootSrc []byte) ([]Collision, error) {
cliDir := filepath.Join(dir, "internal", "cli")
- for _, name := range []string{"profile.go", "deliver.go", "feedback.go"} {
+
+ // Pass 1: the drop-in filenames themselves. If they exist and don't look
+ // like our template output, that's a collision (same shape as before).
+ dropinFiles := map[string]string{
+ "profile.go": "profile",
+ "deliver.go": "deliver",
+ "feedback.go": "feedback",
+ }
+ var collisions []Collision
+ for name, feature := range dropinFiles {
path := filepath.Join(cliDir, name)
data, err := os.ReadFile(path)
if err != nil {
@@ -28,27 +57,79 @@ func detectCollisions(dir string, rootSrc []byte) ([]Collision, error) {
}
return nil, fmt.Errorf("reading %s: %w", path, err)
}
- // A file bearing the printing-press generator header is known to be
- // from a prior patch run or a PR #218 regenerate — not a collision.
if bytes.Contains(data, []byte("Generated by CLI Printing Press")) &&
- bytes.Contains(data, []byte("DO NOT EDIT")) {
- // Additionally check that it references the expected PR #218
- // symbols so an unrelated generated file doesn't get a free pass.
- if looksLikePR218Dropin(name, data) {
- continue
- }
+ bytes.Contains(data, []byte("DO NOT EDIT")) &&
+ looksLikePR218Dropin(name, data) {
+ continue
}
collisions = append(collisions, Collision{
Kind: "resource",
- Symbol: name[:len(name)-3], // strip ".go"
+ Symbol: feature,
File: path,
Message: fmt.Sprintf("%s already exists and is not a PR #218 drop-in — refusing to overwrite", path),
})
}
+ // Pass 2: any OTHER file under internal/cli that declares one of our
+ // drop-in's top-level symbols. Catches cases like steam-web, which has a
+ // pre-existing cmd_profile.go that defines newProfileCmd.
+ entries, err := os.ReadDir(cliDir)
+ if err != nil {
+ if os.IsNotExist(err) {
+ return collisions, nil
+ }
+ return nil, fmt.Errorf("reading %s: %w", cliDir, err)
+ }
+ for _, entry := range entries {
+ if entry.IsDir() {
+ continue
+ }
+ name := entry.Name()
+ if _, isDropin := dropinFiles[name]; isDropin {
+ continue // handled in pass 1
+ }
+ if filepath.Ext(name) != ".go" || hasTestSuffix(name) {
+ continue
+ }
+ path := filepath.Join(cliDir, name)
+ data, err := os.ReadFile(path)
+ if err != nil {
+ return nil, fmt.Errorf("reading %s: %w", path, err)
+ }
+ for _, match := range topLevelDeclRe.FindAllStringSubmatch(string(data), -1) {
+ for feature, syms := range symbolsByFeature {
+ if featureAlreadyColliding(collisions, feature) {
+ continue
+ }
+ for _, sym := range syms {
+ if match[1] == sym {
+ collisions = append(collisions, Collision{
+ Kind: "resource",
+ Symbol: feature,
+ File: path,
+ Message: fmt.Sprintf("%s declares %s, which would conflict with PR #218's %s drop-in", path, sym, feature),
+ })
+ }
+ }
+ }
+ }
+ }
return collisions, nil
}
+func hasTestSuffix(name string) bool {
+ return len(name) > len("_test.go") && name[len(name)-len("_test.go"):] == "_test.go"
+}
+
+func featureAlreadyColliding(collisions []Collision, feature string) bool {
+ for _, c := range collisions {
+ if c.Symbol == feature {
+ return true
+ }
+ }
+ return false
+}
+
// looksLikePR218Dropin returns true when the file content matches the
// signature of the template output for profile/deliver/feedback. Cheap
// heuristic: check for the canonical function name each template defines.
diff --git a/internal/patch/patch.go b/internal/patch/patch.go
index a02c9563..7e4bc83f 100644
--- a/internal/patch/patch.go
+++ b/internal/patch/patch.go
@@ -99,6 +99,22 @@ func Patch(opts Options) (*Report, error) {
for _, c := range fatal {
skipFeatures[c.Symbol] = true
}
+ // Shape-mismatch guard: drop-ins reference rootFlags and assume
+ // `rootCmd.PersistentFlags()` / `rootCmd.AddCommand` call sites exist
+ // for the AST-injection anchors. If the target CLI has a different
+ // shape (e.g. `root.PersistentFlags()` with a package-global rootCmd
+ // and no rootFlags struct), writing drop-ins alone would produce a
+ // compile failure. Verify shape first and refuse loudly.
+ if mismatch := checkRootShape(rootSrc); mismatch != "" {
+ report.Collisions = append(report.Collisions, Collision{
+ Kind: "shape",
+ Symbol: "root.go",
+ File: rootPath,
+ Message: mismatch + " — cannot AST-inject. This CLI needs a reprint rather than a patch.",
+ })
+ return report, nil
+ }
+
patchedRoot, rootChanged, err := injectRootAST(rootSrc, injectOptions{Skip: skipFeatures})
if err != nil {
return nil, fmt.Errorf("AST injection: %w", err)
@@ -149,7 +165,9 @@ func Patch(opts Options) (*Report, error) {
}
if !opts.SkipBuild {
- out, buildErr := exec.Command("go", "build", "./...").CombinedOutput()
+ buildCmd := exec.Command("go", "build", "./...")
+ buildCmd.Dir = opts.Dir // build the target CLI, not the printing-press repo
+ out, buildErr := buildCmd.CombinedOutput()
report.BuildOK = buildErr == nil
if buildErr != nil {
report.BuildOutput = string(out)
← 8c54c4c8 fix(cli): scope goimports to patched files only (#223)
·
back to Cli Printing Press
·
refactor(cli): rename sniff to browser-sniff (#225) 23084c76 →