diff --git a/cmd/autosync.go b/cmd/autosync.go index 49087561..fd521096 100644 --- a/cmd/autosync.go +++ b/cmd/autosync.go @@ -34,44 +34,60 @@ var ( strandedWarnMu sync.Mutex ) -// 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. +// mutatingCommands lists commands that write data that syncs, so they should trigger auto-sync. +// A top-level name also covers its subcommands. A full path such as "board create" +// covers just that subcommand. var mutatingCommands = map[string]bool{ - "create": true, - "update": true, - "delete": true, - "restore": true, - "start": true, - "unstart": true, - "close": true, - "block": true, - "unblock": true, - "reopen": true, - "review": true, - "approve": true, - "reject": true, - "log": true, - "handoff": true, - "focus": true, - "unfocus": true, - "link": true, - "unlink": true, - "comment": true, - "undo": true, - "import": true, - "init": true, - "board": true, - "dep": true, - "blocked-by": true, - "depends-on": true, - "epic": true, - "task": true, - "ws": true, - "monitor": true, + "create": true, + "update": true, + "delete": true, + "restore": true, + "start": true, + "unstart": true, + "close": true, + "block": true, + "unblock": true, + "reopen": true, + "review": true, + "approve": true, + "reject": true, + "log": true, + "handoff": true, + "link": true, + "unlink": true, + "comment": true, + "undo": true, + "import": true, + "init": true, + "dep": true, + "defer": true, + "due": true, + "monitor": true, - // Not "comments": `td comments ` only lists. - "comments add": true, + // These parents also have subcommands that only read, so only the ones that + // write are listed. `comments` itself only lists (`td comments `). + "board create": true, + "board delete": true, + "board edit": true, + "board move": true, + "board unposition": true, + "comments add": true, + "epic create": true, + "note add": true, + "note archive": true, + "note delete": true, + "note edit": true, + "note pin": true, + "note restore": true, + "note unarchive": true, + "note unpin": true, + "task create": true, + "ws end": true, + "ws handoff": true, + "ws log": true, + "ws start": true, + "ws tag": true, + "ws untag": true, } // isMutatingCommand checks if the given command name triggers auto-sync. diff --git a/cmd/autosync_test.go b/cmd/autosync_test.go index c236d7cd..0b816b61 100644 --- a/cmd/autosync_test.go +++ b/cmd/autosync_test.go @@ -11,7 +11,8 @@ 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 add", "monitor"} + mutating := []string{"create", "update", "delete", "start", "close", "log", "handoff", "dep", "defer", "due", "monitor", + "board create", "comments add", "epic create", "note add", "task create", "ws start"} for _, name := range mutating { if !isMutatingCommand(name) { t.Errorf("expected %q to be mutating", name) @@ -19,7 +20,8 @@ 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", "comments"} + readOnly := []string{"list", "show", "search", "query", "sync", "auth", "status", "info", "version", "help", "doctor", "comments", + "blocked-by", "depends-on", "focus", "unfocus", "board", "epic", "note", "task", "ws"} for _, name := range readOnly { if isMutatingCommand(name) { t.Errorf("expected %q to NOT be mutating", name) @@ -35,7 +37,25 @@ func TestIsMutatingInvocation(t *testing.T) { {[]string{"comments", "td-1"}, false}, {[]string{"comments", "add", "td-1", "text"}, true}, {[]string{"comment", "td-1", "text"}, true}, - {[]string{"board", "list"}, true}, + {[]string{"board", "list"}, false}, + {[]string{"board", "show", "b"}, false}, + {[]string{"board", "create", "b"}, true}, + {[]string{"epic", "list"}, false}, + {[]string{"epic", "create", "e"}, true}, + {[]string{"task", "list"}, false}, + {[]string{"task", "create", "t"}, true}, + {[]string{"ws", "current"}, false}, + {[]string{"ws", "show", "w"}, false}, + {[]string{"ws", "start", "w"}, true}, + {[]string{"note", "list"}, false}, + {[]string{"note", "show", "n"}, false}, + {[]string{"note", "add", "n"}, true}, + {[]string{"blocked-by", "td-1"}, false}, + {[]string{"depends-on", "td-1"}, false}, + {[]string{"focus", "td-1"}, false}, + {[]string{"unfocus"}, false}, + {[]string{"defer", "td-1", "+1w"}, true}, + {[]string{"due", "td-1", "friday"}, true}, {[]string{"list"}, false}, } for _, tt := range tests { @@ -51,7 +71,43 @@ func TestIsMutatingInvocation(t *testing.T) { } } -func TestMutationHookSkipsCommentsListing(t *testing.T) { +// TestSubcommandsAreClassified fails when a parent whose writes are listed one +// subcommand at a time gains a subcommand that is neither listed as mutating +// nor known to be read-only, so a new write cannot silently skip autosync. +func TestSubcommandsAreClassified(t *testing.T) { + readOnly := map[string]bool{ + "board list": true, + "board show": true, + "epic list": true, + "note list": true, + "note show": true, + "task list": true, + "ws current": true, + "ws list": true, + "ws show": true, + } + for _, parent := range []string{"board", "comments", "epic", "note", "task", "ws"} { + cmd, _, err := rootCmd.Find([]string{parent}) + if err != nil { + t.Fatalf("find %q: %v", parent, err) + } + if !cmd.HasSubCommands() { + t.Fatalf("%q has no subcommands", parent) + } + for _, sub := range cmd.Commands() { + path := strings.TrimPrefix(sub.CommandPath(), rootCmd.CommandPath()+" ") + if mutating := isMutatingInvocation(sub); mutating == readOnly[path] { + t.Errorf("%q: isMutatingInvocation = %v; list it in mutatingCommands if it writes, or only in this test's read-only list if it only reads", path, mutating) + } + } + } +} + +func TestMutationHookRunsOnlyForMutatingCommands(t *testing.T) { + // The hook runs only while the autosync gate is open, so ignore any kill-switch + // in the developer's environment. + t.Setenv("TD_FEATURE_SYNC_AUTOSYNC", "") + t.Setenv("TD_SYNC_AUTO", "") t.Setenv("TD_AUTH_KEY", "test-key") dir := setupSyncStateDir(t, "proj-test", false) saveAndRestoreGlobals(t) @@ -70,6 +126,11 @@ func TestMutationHookSkipsCommentsListing(t *testing.T) { }{ {[]string{"comments", "td-1"}, 0}, {[]string{"comments", "add", "td-1", "text"}, 1}, + {[]string{"board", "list"}, 0}, + {[]string{"board", "create", "b"}, 1}, + {[]string{"blocked-by", "td-1"}, 0}, + {[]string{"due", "td-1", "friday"}, 1}, + {[]string{"note", "add", "n"}, 1}, } for _, tt := range tests { t.Run(strings.Join(tt.args, " "), func(t *testing.T) {