From 164b0b9f623dd519411ccffc3dcc1c38c349a1d8 Mon Sep 17 00:00:00 2001 From: Justin Bishop Date: Sat, 17 Oct 2026 10:48:42 -0700 Subject: [PATCH] fix(sync): stop autosyncing after td comments listings `td comments ` only reads, but it shared the `comments` entry that makes `td comments add` sync, so each listing pushed and pulled afterward. The mutating list now names `comments add`, and the post-run hook also matches a command's full path. Co-Authored-By: Claude Opus 5.5 --- cmd/autosync.go | 6 ++++- cmd/autosync_test.go | 64 ++++++++++++++++++++++++++++++++++++++++++-- cmd/feature_gate.go | 17 ++++++++++-- 3 files changed, 82 insertions(+), 5 deletions(-) diff --git a/cmd/autosync.go b/cmd/autosync.go index 9693c442..49087561 100644 --- a/cmd/autosync.go +++ b/cmd/autosync.go @@ -35,6 +35,8 @@ var ( ) // mutatingCommands lists commands that modify local data and should trigger auto-sync. +// A top-level name also covers its subcommands. A full path such as "comments add" +// covers just that subcommand, for a parent whose own run only reads. var mutatingCommands = map[string]bool{ "create": true, "update": true, @@ -56,7 +58,6 @@ var mutatingCommands = map[string]bool{ "link": true, "unlink": true, "comment": true, - "comments": true, "undo": true, "import": true, "init": true, @@ -68,6 +69,9 @@ var mutatingCommands = map[string]bool{ "task": true, "ws": true, "monitor": true, + + // Not "comments": `td comments ` only lists. + "comments add": true, } // isMutatingCommand checks if the given command name triggers auto-sync. diff --git a/cmd/autosync_test.go b/cmd/autosync_test.go index 436662f4..c236d7cd 100644 --- a/cmd/autosync_test.go +++ b/cmd/autosync_test.go @@ -1,6 +1,7 @@ package cmd import ( + "strings" "sync/atomic" "testing" "time" @@ -10,7 +11,7 @@ import ( func TestIsMutatingCommand(t *testing.T) { // Commands that should trigger auto-sync - mutating := []string{"create", "update", "delete", "start", "close", "log", "handoff", "board", "dep", "ws", "comments", "monitor"} + mutating := []string{"create", "update", "delete", "start", "close", "log", "handoff", "board", "dep", "ws", "comments add", "monitor"} for _, name := range mutating { if !isMutatingCommand(name) { t.Errorf("expected %q to be mutating", name) @@ -18,7 +19,7 @@ func TestIsMutatingCommand(t *testing.T) { } // Commands that should NOT trigger auto-sync - readOnly := []string{"list", "show", "search", "query", "sync", "auth", "status", "info", "version", "help", "doctor"} + readOnly := []string{"list", "show", "search", "query", "sync", "auth", "status", "info", "version", "help", "doctor", "comments"} for _, name := range readOnly { if isMutatingCommand(name) { t.Errorf("expected %q to NOT be mutating", name) @@ -26,6 +27,65 @@ func TestIsMutatingCommand(t *testing.T) { } } +func TestIsMutatingInvocation(t *testing.T) { + tests := []struct { + args []string + want bool + }{ + {[]string{"comments", "td-1"}, false}, + {[]string{"comments", "add", "td-1", "text"}, true}, + {[]string{"comment", "td-1", "text"}, true}, + {[]string{"board", "list"}, true}, + {[]string{"list"}, false}, + } + for _, tt := range tests { + t.Run(strings.Join(tt.args, " "), func(t *testing.T) { + cmd, _, err := rootCmd.Find(tt.args) + if err != nil { + t.Fatalf("find %v: %v", tt.args, err) + } + if got := isMutatingInvocation(cmd); got != tt.want { + t.Errorf("isMutatingInvocation(%q) = %v, want %v", cmd.CommandPath(), got, tt.want) + } + }) + } +} + +func TestMutationHookSkipsCommentsListing(t *testing.T) { + t.Setenv("TD_AUTH_KEY", "test-key") + dir := setupSyncStateDir(t, "proj-test", false) + saveAndRestoreGlobals(t) + baseDirOverride = &dir + + origHooks := syncFeatureHooks + t.Cleanup(func() { syncFeatureHooks = origHooks }) + calls := 0 + hooks := origHooks + hooks.OnAfterMutation = func() { calls++ } + syncFeatureHooks = hooks + + tests := []struct { + args []string + want int + }{ + {[]string{"comments", "td-1"}, 0}, + {[]string{"comments", "add", "td-1", "text"}, 1}, + } + for _, tt := range tests { + t.Run(strings.Join(tt.args, " "), func(t *testing.T) { + cmd, _, err := rootCmd.Find(tt.args) + if err != nil { + t.Fatalf("find %v: %v", tt.args, err) + } + calls = 0 + runGatedSyncMutationHook(cmd) + if calls != tt.want { + t.Errorf("post-mutation sync ran %d times, want %d", calls, tt.want) + } + }) + } +} + func TestAutoSyncEnabled_Default(t *testing.T) { // With no env var set, auto-sync should be enabled by default t.Setenv("TD_SYNC_AUTO", "") diff --git a/cmd/feature_gate.go b/cmd/feature_gate.go index 09c2fe20..a7b9bc95 100644 --- a/cmd/feature_gate.go +++ b/cmd/feature_gate.go @@ -1,6 +1,8 @@ package cmd import ( + "strings" + "github.com/marcus/td/internal/features" "github.com/spf13/cobra" ) @@ -60,8 +62,7 @@ func runGatedSyncMutationHook(cmd *cobra.Command) { // Skip everything for non-mutating (read-only) commands: the stranded warning // and the autosync hook both only concern commands that change local data. - commandName := resolveCommandName(cmd) - if syncFeatureHooks.IsMutatingCommand != nil && !syncFeatureHooks.IsMutatingCommand(commandName) { + if !isMutatingInvocation(cmd) { return } @@ -80,6 +81,18 @@ func runGatedSyncMutationHook(cmd *cobra.Command) { syncFeatureHooks.OnAfterMutation() } +// isMutatingInvocation reports whether cmd counts as mutating, checking both its +// resolved name (its parent's, for a subcommand) and its full path below the +// root, such as "comments add". Without a registered check, every command counts. +func isMutatingInvocation(cmd *cobra.Command) bool { + isMutating := syncFeatureHooks.IsMutatingCommand + if isMutating == nil { + return true + } + path := strings.TrimPrefix(cmd.CommandPath(), cmd.Root().CommandPath()+" ") + return isMutating(resolveCommandName(cmd)) || isMutating(path) +} + func resolveCommandName(cmd *cobra.Command) string { name := cmd.Name() if cmd.Parent() != nil && cmd.Parent().Name() != "td" {