← back to Cli Printing Press
feat(cli): patch skips AST mutations owned by colliding features (#222)
331809da1185d0c4758565b90506d33692257d3c · 2026-04-18 02:37:25 -0700 · Trevin Chow
When --force is used to proceed past a resource-level collision, the
patcher now also skips the AST mutations owned by the colliding feature.
Previously --force would skip the companion drop-in but still inject the
corresponding wiring into root.go, producing a compile failure on the
next build (e.g. Pagliacci-pizza has a spec-derived feedback resource,
so its own feedback.go already defines newFeedbackCmd; --force would
inject a second AddCommand(newFeedbackCmd) and break the build).
Each feature now owns its full mutation set:
profile → profileName field, --profile flag, profile-lookup block,
newProfileCmd AddCommand
deliver → deliverSpec/deliverBuf/deliverSink fields, bytes/io/os
imports, --deliver flag, deliver-setup block, post-Execute
flush
feedback → newFeedbackCmd AddCommand
A skipped feature's mutations are all omitted. The remaining features
patch cleanly.
Validated against Pagliacci-pizza: `patch --force` creates 2 drop-ins
(profile.go + deliver.go), modifies root.go, reports 1 collision, build
passes, Pagliacci's own `feedback` subcommand survives, new `profile`
command and `--deliver` / `--profile` flags are live.
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/patch.go
Diff
commit 331809da1185d0c4758565b90506d33692257d3c
Author: Trevin Chow <trevin@trevinchow.com>
Date: Sat Apr 18 02:37:25 2026 -0700
feat(cli): patch skips AST mutations owned by colliding features (#222)
When --force is used to proceed past a resource-level collision, the
patcher now also skips the AST mutations owned by the colliding feature.
Previously --force would skip the companion drop-in but still inject the
corresponding wiring into root.go, producing a compile failure on the
next build (e.g. Pagliacci-pizza has a spec-derived feedback resource,
so its own feedback.go already defines newFeedbackCmd; --force would
inject a second AddCommand(newFeedbackCmd) and break the build).
Each feature now owns its full mutation set:
profile → profileName field, --profile flag, profile-lookup block,
newProfileCmd AddCommand
deliver → deliverSpec/deliverBuf/deliverSink fields, bytes/io/os
imports, --deliver flag, deliver-setup block, post-Execute
flush
feedback → newFeedbackCmd AddCommand
A skipped feature's mutations are all omitted. The remaining features
patch cleanly.
Validated against Pagliacci-pizza: `patch --force` creates 2 drop-ins
(profile.go + deliver.go), modifies root.go, reports 1 collision, build
passes, Pagliacci's own `feedback` subcommand survives, new `profile`
command and `--deliver` / `--profile` flags are live.
Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
---
internal/patch/ast_inject.go | 222 +++++++++++++++++++++++++-------------
internal/patch/ast_inject_test.go | 32 ++++--
internal/patch/patch.go | 10 +-
3 files changed, 181 insertions(+), 83 deletions(-)
diff --git a/internal/patch/ast_inject.go b/internal/patch/ast_inject.go
index 27646464..fdd1d069 100644
--- a/internal/patch/ast_inject.go
+++ b/internal/patch/ast_inject.go
@@ -9,22 +9,42 @@ import (
"github.com/dave/dst/decorator"
)
+// injectOptions controls which feature mutations are applied. When a feature
+// is in Skip, every mutation it owns is omitted: its rootFlags fields, its
+// imports, its flag registration, its PersistentPreRunE block, its
+// AddCommand call, and (for deliver) its post-Execute flush. This keeps the
+// patched CLI buildable when a companion drop-in is skipped due to a
+// resource-level collision.
+type injectOptions struct {
+ // Skip contains feature names: "profile", "deliver", "feedback".
+ Skip map[string]bool
+}
+
+func (o injectOptions) skip(feature string) bool {
+ return o.Skip != nil && o.Skip[feature]
+}
+
// injectRootAST applies PR #218's root.go mutations to src and returns the
// patched source + whether any mutation occurred. Mutations are idempotent:
// a second call against already-patched source returns (src, false, nil).
-func injectRootAST(src []byte) ([]byte, bool, error) {
+func injectRootAST(src []byte, opts injectOptions) ([]byte, bool, error) {
file, err := decorator.Parse(src)
if err != nil {
return nil, false, fmt.Errorf("parse root.go: %w", err)
}
changed := false
- changed = addRootFlagsFields(file) || changed
- changed = addImports(file, "bytes", "io", "os") || changed
- changed = addPersistentFlags(file) || changed
- changed = addPreRunBlocks(file) || changed
- changed = addCommands(file) || changed
- changed = addPostExecuteFlush(file) || changed
+ changed = addRootFlagsFields(file, opts) || changed
+ // Imports (bytes/io/os) are only used by deliver's mutations.
+ if !opts.skip("deliver") {
+ changed = addImports(file, "bytes", "io", "os") || changed
+ }
+ changed = addPersistentFlags(file, opts) || changed
+ changed = addPreRunBlocks(file, opts) || changed
+ changed = addCommands(file, opts) || changed
+ if !opts.skip("deliver") {
+ changed = addPostExecuteFlush(file) || changed
+ }
if !changed {
return src, false, nil
@@ -38,8 +58,8 @@ func injectRootAST(src []byte) ([]byte, bool, error) {
}
// addRootFlagsFields appends profileName, deliverSpec, deliverBuf, deliverSink
-// to the rootFlags struct.
-func addRootFlagsFields(file *dst.File) bool {
+// to the rootFlags struct. Fields owned by skipped features are omitted.
+func addRootFlagsFields(file *dst.File, opts injectOptions) bool {
changed := false
dst.Inspect(file, func(n dst.Node) bool {
ts, ok := n.(*dst.TypeSpec)
@@ -50,31 +70,38 @@ func addRootFlagsFields(file *dst.File) bool {
if !ok {
return true
}
- for _, name := range []string{"profileName", "deliverSpec"} {
- if !structHasField(st, name) {
+ if !opts.skip("profile") && !structHasField(st, "profileName") {
+ st.Fields.List = append(st.Fields.List, &dst.Field{
+ Names: []*dst.Ident{{Name: "profileName"}},
+ Type: &dst.Ident{Name: "string"},
+ })
+ changed = true
+ }
+ if !opts.skip("deliver") {
+ if !structHasField(st, "deliverSpec") {
st.Fields.List = append(st.Fields.List, &dst.Field{
- Names: []*dst.Ident{{Name: name}},
+ Names: []*dst.Ident{{Name: "deliverSpec"}},
Type: &dst.Ident{Name: "string"},
})
changed = true
}
- }
- if !structHasField(st, "deliverBuf") {
- st.Fields.List = append(st.Fields.List, &dst.Field{
- Names: []*dst.Ident{{Name: "deliverBuf"}},
- Type: &dst.StarExpr{X: &dst.SelectorExpr{
- X: &dst.Ident{Name: "bytes"},
- Sel: &dst.Ident{Name: "Buffer"},
- }},
- })
- changed = true
- }
- if !structHasField(st, "deliverSink") {
- st.Fields.List = append(st.Fields.List, &dst.Field{
- Names: []*dst.Ident{{Name: "deliverSink"}},
- Type: &dst.Ident{Name: "DeliverSink"},
- })
- changed = true
+ if !structHasField(st, "deliverBuf") {
+ st.Fields.List = append(st.Fields.List, &dst.Field{
+ Names: []*dst.Ident{{Name: "deliverBuf"}},
+ Type: &dst.StarExpr{X: &dst.SelectorExpr{
+ X: &dst.Ident{Name: "bytes"},
+ Sel: &dst.Ident{Name: "Buffer"},
+ }},
+ })
+ changed = true
+ }
+ if !structHasField(st, "deliverSink") {
+ st.Fields.List = append(st.Fields.List, &dst.Field{
+ Names: []*dst.Ident{{Name: "deliverSink"}},
+ Type: &dst.Ident{Name: "DeliverSink"},
+ })
+ changed = true
+ }
}
return false
})
@@ -120,19 +147,43 @@ func addImports(file *dst.File, pkgs ...string) bool {
}
// addPersistentFlags inserts --profile and --deliver after the last existing
-// PersistentFlags() registration inside Execute().
-func addPersistentFlags(file *dst.File) bool {
+// PersistentFlags() registration inside Execute(). Skipped features are
+// omitted; if both profile and deliver are skipped, no flag is added.
+func addPersistentFlags(file *dst.File, opts injectOptions) bool {
changed := false
dst.Inspect(file, func(n dst.Node) bool {
fn, ok := n.(*dst.FuncDecl)
if !ok || fn.Name.Name != "Execute" {
return true
}
- // Idempotency: skip if --profile already registered.
- for _, stmt := range fn.Body.List {
- if persistentFlagsRegisters(stmt, "profile") {
- return false
+ var newStmts []dst.Stmt
+ if !opts.skip("profile") {
+ // Idempotency: skip if --profile already registered.
+ already := false
+ for _, stmt := range fn.Body.List {
+ if persistentFlagsRegisters(stmt, "profile") {
+ already = true
+ break
+ }
+ }
+ if !already {
+ newStmts = append(newStmts, parseStmt(`rootCmd.PersistentFlags().StringVar(&flags.profileName, "profile", "", "Apply values from a saved profile")`))
+ }
+ }
+ if !opts.skip("deliver") {
+ already := false
+ for _, stmt := range fn.Body.List {
+ if persistentFlagsRegisters(stmt, "deliver") {
+ already = true
+ break
+ }
}
+ if !already {
+ newStmts = append(newStmts, parseStmt(`rootCmd.PersistentFlags().StringVar(&flags.deliverSpec, "deliver", "", "Route output to a sink: stdout (default), file:<path>, webhook:<url>")`))
+ }
+ }
+ if len(newStmts) == 0 {
+ return false
}
lastFlagIdx := -1
for i, stmt := range fn.Body.List {
@@ -143,10 +194,6 @@ func addPersistentFlags(file *dst.File) bool {
if lastFlagIdx < 0 {
return false
}
- newStmts := []dst.Stmt{
- parseStmt(`rootCmd.PersistentFlags().StringVar(&flags.profileName, "profile", "", "Apply values from a saved profile")`),
- parseStmt(`rootCmd.PersistentFlags().StringVar(&flags.deliverSpec, "deliver", "", "Route output to a sink: stdout (default), file:<path>, webhook:<url>")`),
- }
fn.Body.List = append(fn.Body.List[:lastFlagIdx+1], append(newStmts, fn.Body.List[lastFlagIdx+1:]...)...)
changed = true
return false
@@ -155,8 +202,8 @@ func addPersistentFlags(file *dst.File) bool {
}
// addPreRunBlocks inserts the deliver-setup and profile-lookup blocks at the
-// top of the PersistentPreRunE function body.
-func addPreRunBlocks(file *dst.File) bool {
+// top of the PersistentPreRunE function body. Skipped features are omitted.
+func addPreRunBlocks(file *dst.File, opts injectOptions) bool {
changed := false
dst.Inspect(file, func(n dst.Node) bool {
assign, ok := n.(*dst.AssignStmt)
@@ -174,34 +221,38 @@ func addPreRunBlocks(file *dst.File) bool {
if !ok {
return false
}
- // Idempotency: skip if deliverSpec already referenced anywhere in body.
- if nodeReferences(fn, "deliverSpec") {
+ var prepend []dst.Stmt
+ if !opts.skip("deliver") && !nodeReferences(fn, "deliverSpec") {
+ prepend = append(prepend, parseStmt(`if flags.deliverSpec != "" {
+ sink, err := ParseDeliverSink(flags.deliverSpec)
+ if err != nil {
+ return err
+ }
+ flags.deliverSink = sink
+ if sink.Scheme != "stdout" && sink.Scheme != "" {
+ flags.deliverBuf = &bytes.Buffer{}
+ cmd.SetOut(io.MultiWriter(os.Stdout, flags.deliverBuf))
+ }
+ }`))
+ }
+ if !opts.skip("profile") && !nodeReferences(fn, "profileName") {
+ prepend = append(prepend, parseStmt(`if flags.profileName != "" {
+ profile, err := GetProfile(flags.profileName)
+ if err != nil {
+ return err
+ }
+ if profile == nil {
+ return fmt.Errorf("profile %q not found", flags.profileName)
+ }
+ if err := ApplyProfileToFlags(cmd, profile); err != nil {
+ return err
+ }
+ }`))
+ }
+ if len(prepend) == 0 {
return false
}
- deliverBlock := parseStmt(`if flags.deliverSpec != "" {
- sink, err := ParseDeliverSink(flags.deliverSpec)
- if err != nil {
- return err
- }
- flags.deliverSink = sink
- if sink.Scheme != "stdout" && sink.Scheme != "" {
- flags.deliverBuf = &bytes.Buffer{}
- cmd.SetOut(io.MultiWriter(os.Stdout, flags.deliverBuf))
- }
- }`)
- profileBlock := parseStmt(`if flags.profileName != "" {
- profile, err := GetProfile(flags.profileName)
- if err != nil {
- return err
- }
- if profile == nil {
- return fmt.Errorf("profile %q not found", flags.profileName)
- }
- if err := ApplyProfileToFlags(cmd, profile); err != nil {
- return err
- }
- }`)
- fn.Body.List = append([]dst.Stmt{deliverBlock, profileBlock}, fn.Body.List...)
+ fn.Body.List = append(prepend, fn.Body.List...)
changed = true
return false
})
@@ -209,19 +260,42 @@ func addPreRunBlocks(file *dst.File) bool {
}
// addCommands appends newProfileCmd and newFeedbackCmd AddCommand calls after
-// the last existing rootCmd.AddCommand entry.
-func addCommands(file *dst.File) bool {
+// the last existing rootCmd.AddCommand entry. Skipped features are omitted.
+func addCommands(file *dst.File, opts injectOptions) bool {
changed := false
dst.Inspect(file, func(n dst.Node) bool {
fn, ok := n.(*dst.FuncDecl)
if !ok || fn.Name.Name != "Execute" {
return true
}
- for _, stmt := range fn.Body.List {
- if rootAddsCommand(stmt, "newProfileCmd") {
- return false
+ var newStmts []dst.Stmt
+ if !opts.skip("profile") {
+ already := false
+ for _, stmt := range fn.Body.List {
+ if rootAddsCommand(stmt, "newProfileCmd") {
+ already = true
+ break
+ }
+ }
+ if !already {
+ newStmts = append(newStmts, parseStmt(`rootCmd.AddCommand(newProfileCmd(&flags))`))
}
}
+ if !opts.skip("feedback") {
+ already := false
+ for _, stmt := range fn.Body.List {
+ if rootAddsCommand(stmt, "newFeedbackCmd") {
+ already = true
+ break
+ }
+ }
+ if !already {
+ newStmts = append(newStmts, parseStmt(`rootCmd.AddCommand(newFeedbackCmd(&flags))`))
+ }
+ }
+ if len(newStmts) == 0 {
+ return false
+ }
lastAddIdx := -1
for i, stmt := range fn.Body.List {
if isRootAddCommand(stmt) {
@@ -231,10 +305,6 @@ func addCommands(file *dst.File) bool {
if lastAddIdx < 0 {
return false
}
- newStmts := []dst.Stmt{
- parseStmt(`rootCmd.AddCommand(newProfileCmd(&flags))`),
- parseStmt(`rootCmd.AddCommand(newFeedbackCmd(&flags))`),
- }
fn.Body.List = append(fn.Body.List[:lastAddIdx+1], append(newStmts, fn.Body.List[lastAddIdx+1:]...)...)
changed = true
return false
diff --git a/internal/patch/ast_inject_test.go b/internal/patch/ast_inject_test.go
index 22d44afd..29590924 100644
--- a/internal/patch/ast_inject_test.go
+++ b/internal/patch/ast_inject_test.go
@@ -58,7 +58,7 @@ func Execute() error {
`
func TestInjectRootAST_MutatesAllSixTargets(t *testing.T) {
- out, changed, err := injectRootAST([]byte(baseRootGo))
+ out, changed, err := injectRootAST([]byte(baseRootGo), injectOptions{})
require.NoError(t, err)
require.True(t, changed, "expected mutation")
src := string(out)
@@ -87,7 +87,7 @@ func TestInjectRootAST_MutatesAllSixTargets(t *testing.T) {
}
func TestInjectRootAST_PreservesNovelCommands(t *testing.T) {
- out, _, err := injectRootAST([]byte(baseRootGo))
+ out, _, err := injectRootAST([]byte(baseRootGo), injectOptions{})
require.NoError(t, err)
src := string(out)
@@ -109,18 +109,18 @@ func TestInjectRootAST_PreservesNovelCommands(t *testing.T) {
}
func TestInjectRootAST_Idempotent(t *testing.T) {
- first, changed, err := injectRootAST([]byte(baseRootGo))
+ first, changed, err := injectRootAST([]byte(baseRootGo), injectOptions{})
require.NoError(t, err)
require.True(t, changed)
- second, changedAgain, err := injectRootAST(first)
+ second, changedAgain, err := injectRootAST(first, injectOptions{})
require.NoError(t, err)
assert.False(t, changedAgain, "second run should be a no-op")
assert.Equal(t, string(first), string(second), "second run must not alter bytes")
}
func TestInjectRootAST_PreservesVersionAndImports(t *testing.T) {
- out, _, err := injectRootAST([]byte(baseRootGo))
+ out, _, err := injectRootAST([]byte(baseRootGo), injectOptions{})
require.NoError(t, err)
src := string(out)
@@ -129,6 +129,26 @@ func TestInjectRootAST_PreservesVersionAndImports(t *testing.T) {
assert.Contains(t, src, `"github.com/spf13/cobra"`, "cobra import must survive")
}
+// TestInjectRootAST_SkipFeedback exercises the Pagliacci case: a spec has its
+// own feedback resource so feedback.go is not dropped in, and the AST patch
+// must skip `rootCmd.AddCommand(newFeedbackCmd(...))` to keep the build green.
+func TestInjectRootAST_SkipFeedback(t *testing.T) {
+ out, changed, err := injectRootAST([]byte(baseRootGo), injectOptions{
+ Skip: map[string]bool{"feedback": true},
+ })
+ require.NoError(t, err)
+ require.True(t, changed, "profile/deliver mutations should still fire")
+ src := string(out)
+
+ assert.NotContains(t, src, "newFeedbackCmd(&flags)",
+ "newFeedbackCmd must NOT be registered when feedback is skipped")
+ assert.Contains(t, src, "newProfileCmd(&flags)",
+ "newProfileCmd must still be registered")
+ assert.Contains(t, src, `"deliver"`, "--deliver flag must still be registered")
+ assert.Contains(t, src, `flags.deliverSpec != ""`,
+ "deliver-setup block must still be present")
+}
+
// 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) {
@@ -138,7 +158,7 @@ func Execute() error {
return nil
}
`
- out, changed, err := injectRootAST([]byte(src))
+ out, changed, err := injectRootAST([]byte(src), injectOptions{})
require.NoError(t, err)
// rootFlags struct is missing so no field mutation; no PersistentFlags
// call so no flag mutation; no AddCommand so no command mutation.
diff --git a/internal/patch/patch.go b/internal/patch/patch.go
index 15b5e52f..4b6db2ca 100644
--- a/internal/patch/patch.go
+++ b/internal/patch/patch.go
@@ -91,7 +91,15 @@ func Patch(opts Options) (*Report, error) {
}
report.Collisions = collisions
- patchedRoot, rootChanged, err := injectRootAST(rootSrc)
+ // When --force is used to proceed past a resource-level collision, also
+ // skip the AST mutations owned by the colliding feature so the resulting
+ // root.go doesn't reference a symbol the skipped drop-in would have
+ // provided.
+ skipFeatures := map[string]bool{}
+ for _, c := range fatal {
+ skipFeatures[c.Symbol] = true
+ }
+ patchedRoot, rootChanged, err := injectRootAST(rootSrc, injectOptions{Skip: skipFeatures})
if err != nil {
return nil, fmt.Errorf("AST injection: %w", err)
}
← c5ed4369 fix(cli): publish manifest must read spec.yaml alongside spe
·
back to Cli Printing Press
·
fix(cli): scope goimports to patched files only (#223) 8c54c4c8 →