claude-code: act on the review of the nox-mesh plugin

Refuse paths with empty, dot or parent segments and a file that is also a directory; take an item only when its key says what it is; write the view under its lock; expand nodes all and check node names; merge the plugin's entries into the operator's own; render every file past one that fails; in the home, never take over the person's file, never write through a symbolic link, keep a deleted file deleted, keep the kind's directory; and refuse settings that would deny the console or the marketplace.
2026-10-05 14:29:53 +02:00
parent 1d8f1ceff8
commit 92f78db970
6 changed files with 314 additions and 41 deletions
@@ -69,6 +69,14 @@ const SettingsName = "settings"

 var itemName = regexp.MustCompile(`^[a-z0-9][a-z0-9-]{0,63}$`)

+// nodeName is what a node may be called in a key.
+var nodeName = regexp.MustCompile(`^[a-z0-9][a-z0-9-]{0,62}$`)
+
+// meshOwnedKeys are the settings a registration may not set: the mesh's own, and those that would deny
+// the mesh's console or its marketplace by another way.
+var meshOwnedKeys = []string{"attribution", "allowAllClaudeAiMcps", "apiKeyHelper", "extraKnownMarketplaces", "enabledPlugins",
+	"allowedMcpServers", "deniedMcpServers", "allowManagedMcpServersOnly", "strictKnownMarketplaces", "blockedMarketplaces"}
+
 // hookEvents are the events a hook may be registered for.
 var hookEvents = map[string]bool{"PreToolUse": true, "PostToolUse": true, "UserPromptSubmit": true, "Notification": true,
 	"Stop": true, "SubagentStop": true, "SessionStart": true, "SessionEnd": true, "PreCompact": true}
@@ -139,9 +147,9 @@ func (it Item) Problem() string {
 		if len(it.Settings) == 0 {
 			return "no settings given"
 		}
-		for _, key := range []string{"attribution", "allowAllClaudeAiMcps", "apiKeyHelper", "extraKnownMarketplaces", "enabledPlugins"} {
+		for _, key := range meshOwnedKeys {
 			if _, ok := it.Settings[key]; ok {
-				return fmt.Sprintf("%q is one of the mesh's own keys; a registration cannot set it", key)
+				return fmt.Sprintf("%q is one of the mesh's own keys, or would turn off the mesh's console or plugin; a registration cannot set it", key)
 			}
 		}
 	} else if !itemName.MatchString(it.Name) {
@@ -159,8 +167,13 @@ func (it Item) Problem() string {
 		}
 	}
 	for rel := range it.Files {
-		if rel == "" || path.IsAbs(rel) || strings.Contains(rel, "\\") || path.Clean(rel) != rel || strings.HasPrefix(rel, "..") {
-			return fmt.Sprintf("%q is not a path inside the item", rel)
+		if problem := pathProblem(rel); problem != "" {
+			return problem
+		}
+		for other := range it.Files {
+			if strings.HasPrefix(other, rel+"/") {
+				return fmt.Sprintf("%q is a file and also the directory of %q", rel, other)
+			}
 		}
 	}
 	switch it.Kind {
@@ -179,6 +192,20 @@ func (it Item) Problem() string {
 	return ""
 }

+// pathProblem says why a file's path is not one inside the item, or "": relative, slash-separated, every
+// segment a real name — never empty, `.` or `..`.
+func pathProblem(rel string) string {
+	if rel == "" || path.IsAbs(rel) || strings.ContainsAny(rel, "\\\x00") {
+		return fmt.Sprintf("%q is not a path inside the item", rel)
+	}
+	for _, seg := range strings.Split(rel, "/") {
+		if seg == "" || seg == "." || seg == ".." {
+			return fmt.Sprintf("%q is not a path inside the item", rel)
+		}
+	}
+	return ""
+}
+
 func (it Item) size() int {
 	raw, _ := json.Marshal(it)
 	return len(raw)
@@ -213,14 +240,17 @@ func (v *ConfigView) Take(key, op string, item *Item) bool {
 	if !v.Applies(key) {
 		return false
 	}
+	_, node, _, _, _ := ParseKey(key)
 	v.mu.Lock()
-	if op == "put" && item != nil && item.Problem() == "" {
+	defer v.mu.Unlock()
+	// Only an item that is what its key says: a key decides which nodes take an item, the item where it
+	// lands, and the two must agree — a mesh key holding a home item would land in every home.
+	if op == "put" && item != nil && item.Problem() == "" && item.Key(node) == key {
 		v.items[key] = *item
 	} else {
 		delete(v.items, key)
 	}
-	v.mu.Unlock()
-	return v.writeThrough()
+	return v.writeThroughLocked()
 }

 // Prune drops what the view holds and the state no longer does: a registration removed while this node
@@ -231,13 +261,13 @@ func (v *ConfigView) Prune(present []string) bool {
 		keep[k] = true
 	}
 	v.mu.Lock()
+	defer v.mu.Unlock()
 	for k := range v.items {
 		if !keep[k] {
 			delete(v.items, k)
 		}
 	}
-	v.mu.Unlock()
-	return v.writeThrough()
+	return v.writeThroughLocked()
 }

 // Items is what applies here, by key.
@@ -251,8 +281,10 @@ func (v *ConfigView) Items() map[string]Item {
 	return out
 }

-func (v *ConfigView) writeThrough() bool {
-	now, _ := indented(v.Items())
+// writeThroughLocked writes the view to its file, with v.mu held: the snapshot and the write are one step, so
+// an older snapshot never lands after a newer one.
+func (v *ConfigView) writeThroughLocked() bool {
+	now, _ := indented(v.items)
 	before, _ := os.ReadFile(v.p.config())
 	if string(before) == string(now) {
 		return false
@@ -481,9 +513,15 @@ func digest(s string) string {
 	return hex.EncodeToString(sum[:])
 }

-// PlaceHome brings the home in line with what the home scope wants: writes what is wanted and absent or
-// its own, never a path the person made, and removes what it placed and is no longer wanted — unless the
-// person changed it since. Answers what it did and what it left alone, and why.
+// deletedByHand marks, in the record, a path the mesh placed and the person then deleted: their choice,
+// kept until the item is unregistered.
+const deletedByHand = "deleted-by-hand"
+
+// PlaceHome brings the home in line with what the home scope wants (ADR 0182): it writes what is wanted and
+// absent, or its own; it never writes a path the person made, nor through a directory that is a symbolic
+// link; it removes what it placed and is no longer wanted, unless the person changed it since; and a file it
+// placed that the person deleted stays deleted until the item is unregistered. Answers what it did and what
+// it left alone, and why.
 func PlaceHome(p Paths, want map[string]string) (done []string, left []string) {
 	dir := filepath.Join(p.Home, ".claude")
 	var placed Placed
@@ -498,22 +536,30 @@ func PlaceHome(p Paths, want map[string]string) (done []string, left []string) {
 	for _, rel := range paths {
 		full := filepath.Join(dir, filepath.FromSlash(rel))
 		content := want[rel]
+		if why := linkedParent(dir, rel); why != "" {
+			left = append(left, rel+": "+why+", left alone")
+			continue
+		}
 		current, err := os.ReadFile(full)
 		exists := err == nil
 		ours, wasPlaced := placed[rel]
 		switch {
+		case !exists && wasPlaced && ours == deletedByHand:
+			continue
+		case !exists && wasPlaced:
+			placed[rel] = deletedByHand
+			left = append(left, rel+": deleted by hand, left deleted until the item is unregistered")
+			continue
 		case exists && !wasPlaced:
-			if string(current) == content {
-				placed[rel] = digest(content) // the same bytes: taken over, nothing changes
-				continue
-			}
 			left = append(left, rel+": the person's own, left alone")
 			continue
-		case exists && wasPlaced && digest(string(current)) != ours && digest(string(current)) != digest(content):
-			left = append(left, rel+": changed by hand since it was placed, left alone")
+		case exists && ours == deletedByHand:
+			left = append(left, rel+": made again by hand after the mesh's was deleted, left alone")
 			continue
 		case exists && string(current) == content:
-			placed[rel] = digest(content)
+			continue
+		case exists && digest(string(current)) != ours:
+			left = append(left, rel+": changed by hand since it was placed, left alone")
 			continue
 		}
 		if err := os.MkdirAll(filepath.Dir(full), 0o755); err != nil {
@@ -536,22 +582,50 @@ func PlaceHome(p Paths, want map[string]string) (done []string, left []string) {
 			continue
 		}
 		full := filepath.Join(dir, filepath.FromSlash(rel))
+		if ours == deletedByHand {
+			delete(placed, rel)
+			continue
+		}
+		if why := linkedParent(dir, rel); why != "" {
+			left = append(left, rel+": no longer registered, but "+why+", left alone")
+			continue
+		}
 		current, err := os.ReadFile(full)
 		if err == nil && digest(string(current)) != ours {
 			left = append(left, rel+": no longer registered, but changed by hand, left alone")
 			delete(placed, rel)
 			continue
 		}
-		_ = os.Remove(full)
-		removeEmptyParents(dir, filepath.Dir(full))
+		if err := os.Remove(full); err != nil && !os.IsNotExist(err) {
+			left = append(left, rel+": no longer registered, and could not be removed: "+err.Error())
+			continue
+		}
+		// Up to the kind's own directory, never it: `skills/<name>` goes when empty, `skills` stays.
+		removeEmptyParents(filepath.Join(dir, strings.SplitN(rel, "/", 2)[0]), filepath.Dir(full))
 		delete(placed, rel)
 		done = append(done, rel+": removed")
 	}
 	raw, _ := indented(placed)
-	_ = os.WriteFile(p.placed(), raw, 0o600)
+	if err := os.WriteFile(p.placed(), raw, 0o600); err != nil {
+		left = append(left, "the record of what was placed could not be saved: "+err.Error())
+	}
 	return done, left
 }

+// linkedParent says, when a directory between the agent directory and a file is a symbolic link, which one:
+// writing through it would write wherever it points.
+func linkedParent(dir, rel string) string {
+	parts := strings.Split(rel, "/")
+	at := dir
+	for _, seg := range parts[:len(parts)-1] {
+		at = filepath.Join(at, seg)
+		if info, err := os.Lstat(at); err == nil && info.Mode()&os.ModeSymlink != 0 {
+			return at + " is a symbolic link"
+		}
+	}
+	return ""
+}
+
 // removeEmptyParents removes empty directories from dir up to, not including, root.
 func removeEmptyParents(root, dir string) {
 	for dir != root && strings.HasPrefix(dir, root+string(filepath.Separator)) {
@@ -714,6 +788,8 @@ func Register(p Paths, it Item, nodes []string, unregister bool, state ConfigSta
 		nodes = []string{""}
 	} else if len(nodes) == 0 {
 		nodes = []string{p.Node}
+	} else if problem := nodesProblem(nodes); problem != "" {
+		return map[string]any{verbOf(unregister): false, "reason": problem}, nil
 	}
 	if !unregister && it.Scope == ScopeHome {
 		for _, n := range nodes {
@@ -761,10 +837,10 @@ func Register(p Paths, it Item, nodes []string, unregister bool, state ConfigSta
 	}
 	if changed {
 		rendered, err := RenderNow(p, write)
-		if err != nil {
-			return nil, err
-		}
 		answer["rendered here"] = rendered
+		if err != nil {
+			answer["not written here"] = err.Error() // kept on the bus all the same; the next render tries again
+		}
 		answer["sessions"] = "a new session takes it; a running one at /reload-plugins"
 	} else if v.Applies(keys[0]) || len(keys) > 1 {
 		answer["here"] = "already so"
@@ -774,6 +850,35 @@ func Register(p Paths, it Item, nodes []string, unregister bool, state ConfigSta
 	return answer, nil
 }

+func verbOf(unregister bool) string {
+	return map[bool]string{false: "registered", true: "unregistered"}[unregister]
+}
+
+// nodesProblem says why a list of nodes cannot name keys, or "". "all" has been expanded before this.
+func nodesProblem(nodes []string) string {
+	for _, n := range nodes {
+		if !nodeName.MatchString(n) {
+			return fmt.Sprintf("%q is not a node's name", n)
+		}
+	}
+	return ""
+}
+
+// ExpandNodes turns `all` into every node running the module; anything else is kept.
+func ExpandNodes(nodes []string, running func() ([]string, error)) ([]string, error) {
+	if len(nodes) != 1 || nodes[0] != "all" {
+		return nodes, nil
+	}
+	all, err := running()
+	if err != nil {
+		return nil, fmt.Errorf("which nodes run claude-code: %w", err)
+	}
+	if len(all) == 0 {
+		return nil, fmt.Errorf("no node is known to run claude-code")
+	}
+	return all, nil
+}
+
 // List is every registration of a kind ("" for every kind) on the mesh, by key, read from the state.
 func List(state ConfigState, kind string) (map[string]any, error) {
 	keys, err := state.Keys()