From 249e760fbde0be29fb02f6ac9cbd82cd69364bcb Mon Sep 17 00:00:00 2001 From: claude-ceo-assistant Date: Fri, 8 May 2026 08:26:05 -0700 Subject: [PATCH] =?UTF-8?q?feat(plugins):=20hot-reload=20classifier=20?= =?UTF-8?q?=E2=80=94=20skip=20restart=20on=20SKILL-content-only=20updates?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Closes molecule-core#112. Composes with #114 (atomic install). Before issuing restartFunc, classify the diff between staged and live: - skill-content-only: only **/SKILL.md content changed → skip restart (Claude Code re-reads SKILL.md on each Skill invocation; no in-memory cache) - cold: anything else → restartFunc as before (hooks/settings load at session start; plugin.yaml is structural; added/removed files require a fresh load) DETECTION - Hash every regular file in staged tree (host filesystem, sha256) - Hash every regular file in live tree (in-container via docker exec sh -c 'cd && find . -type f -print0 | xargs -0 sha256sum') - .complete marker dropped from comparison (mtime varies install-to- install; including it would force-cold every reinstall) - File added/removed → cold - File content differs but isn't SKILL.md → cold - All differences are SKILL.md basenames → skill-content-only DEFAULTS COLD - First install (no live tree) → cold - Live tree read failure → cold (conservative; never hot-reload speculatively) - Symlinks skipped during hash (same posture as tar walker) PHASE 4 SELF-REVIEW Correctness: No finding — all error paths default to cold; never falsely classify as skill-content-only. The .complete drop is a deliberate exception (the marker is bookkeeping, not content). Readability: No finding — single-purpose helpers (hashLocalTree, hashContainerTree, isSkillMarkdown, shQuote) each do one thing. The classifier itself reads as 'compare set, then walk diff with isSkillMarkdown gate.' Architecture: No finding — composes existing execAsRoot primitive; new helpers in plugins_classifier.go don't touch any other handler. Old behavior unchanged when live read fails. Security: No finding — shQuote single-quotes any non-trivial path, pluginName comes from validatePluginName-validated source, and the docker exec command takes the path as a single arg (xargs -0 handles binary-safe path delimiting). Symlinks skipped. Performance: No finding — adds two tree walks (host + container) per install. Container walk is one docker exec call returning sha256 lines; for typical plugins (~10-50 files) round-trip is ~100ms. Versus the saved ~5-10s of restart on a hot-reloadable update, this is a clear win. TESTS (4 new, all green; full handler suite green) TestIsSkillMarkdown — basename match, case-sensitive TestHashLocalTree_StableHash — re-hash same dir = same map TestHashLocalTree_SymlinkSkipped — hostile link doesn't poison classifier TestShQuote — quoting boundary for shell injection safety REFS molecule-core#112 — this issue molecule-core#114 — atomic install (.complete marker added there) Reno-Stars iteration safety (Hongming 2026-05-08) Co-Authored-By: Claude Opus 4.7 (1M context) --- .../internal/handlers/plugins_classifier.go | 214 ++++++++++++++++++ .../handlers/plugins_classifier_test.go | 121 ++++++++++ .../handlers/plugins_install_pipeline.go | 15 +- 3 files changed, 349 insertions(+), 1 deletion(-) create mode 100644 workspace-server/internal/handlers/plugins_classifier.go create mode 100644 workspace-server/internal/handlers/plugins_classifier_test.go diff --git a/workspace-server/internal/handlers/plugins_classifier.go b/workspace-server/internal/handlers/plugins_classifier.go new file mode 100644 index 00000000..14595ed6 --- /dev/null +++ b/workspace-server/internal/handlers/plugins_classifier.go @@ -0,0 +1,214 @@ +package handlers + +// plugins_classifier.go — diff classifier for plugin updates. +// +// Closes molecule-core#112. Composes with #114 (atomic install) so the +// platform can decide *before* triggering restartFunc whether the +// update is content-only (SKILL.md text changed; agent re-reads at next +// Skill invocation) or structural (hooks/settings/plugin.yaml/file added +// or removed; agent must restart to pick up the new state). +// +// SKILL.md content is hot-reloadable because Claude Code reads the file +// on each Skill invocation — no in-memory cache. Hooks and settings.json +// are loaded at session start and need a session restart. plugin.yaml +// changes are structural by definition (manifest controls everything +// else). +// +// CLASSIFICATION RULE +// classify(staged, live) → "skill-content-only" if and only if +// every file present in either tree is one of: +// - identical between staged and live, OR +// - a **/SKILL.md file with content change (text body modified) +// AND no files were added or removed. +// Anything else → "cold" (the safe default). +// +// The classifier reads live-tree files from inside the container via +// `docker exec cat`. Comparison is by SHA-256 over file content, not +// mtime — mtime changes on every install regardless of content. + +import ( + "context" + "crypto/sha256" + "encoding/hex" + "fmt" + "io/fs" + "os" + "path/filepath" + "strings" +) + +const ( + // classifyKindSkillContentOnly: install can skip restartFunc; the + // only changes are SKILL.md body text. + classifyKindSkillContentOnly = "skill-content-only" + // classifyKindCold: must restart the workspace container; structural + // or hook/settings change. + classifyKindCold = "cold" +) + +// classifyInstallChanges compares the staged plugin tree (host filesystem) +// against the currently-live plugin tree inside the container. Returns +// classifyKindSkillContentOnly when the only diff is SKILL.md content +// changes, classifyKindCold otherwise (added/removed files, hooks/ +// settings.json edits, plugin.yaml edits, anything else). +// +// `noLive` is the sentinel returned when /configs/plugins/ doesn't +// exist (first install for this plugin). Treated as cold — no live state +// to hot-reload into. +func (h *PluginsHandler) classifyInstallChanges( + ctx context.Context, containerName, hostStagedDir, pluginName string, +) (string, error) { + livePath := "/configs/plugins/" + pluginName + + // Probe: does live exist? If not, this is a first install — cold. + if _, err := h.execAsRoot(ctx, containerName, []string{ + "test", "-d", livePath, + }); err != nil { + return classifyKindCold, nil + } + + // Build hash maps for both trees. + stagedHashes, err := hashLocalTree(hostStagedDir) + if err != nil { + return classifyKindCold, fmt.Errorf("classifier: hash staged: %w", err) + } + liveHashes, err := h.hashContainerTree(ctx, containerName, livePath) + if err != nil { + // Live tree read failure: be conservative, cold-restart. + return classifyKindCold, nil + } + + // Drop the .complete marker from comparison — its mtime/atime can + // vary across installs but content is empty/trivial; including it + // would force-cold every reinstall. + delete(stagedHashes, ".complete") + delete(liveHashes, ".complete") + + // Set difference: any file in one but not the other → cold. + for rel := range stagedHashes { + if _, ok := liveHashes[rel]; !ok { + return classifyKindCold, nil // file added + } + } + for rel := range liveHashes { + if _, ok := stagedHashes[rel]; !ok { + return classifyKindCold, nil // file removed + } + } + + // Same set of files. Walk the diff. + for rel, stagedHash := range stagedHashes { + liveHash := liveHashes[rel] + if stagedHash == liveHash { + continue + } + // Content differs. Allow if and only if it's a SKILL.md. + if !isSkillMarkdown(rel) { + return classifyKindCold, nil + } + } + return classifyKindSkillContentOnly, nil +} + +// isSkillMarkdown returns true for any path whose basename is SKILL.md +// (case-sensitive, matches Claude Code's skill discovery rule). +func isSkillMarkdown(rel string) bool { + return filepath.Base(rel) == "SKILL.md" +} + +// hashLocalTree walks a host directory and returns rel-path → sha256-hex. +// Symlinks are skipped (same posture as the tar walker). +func hashLocalTree(root string) (map[string]string, error) { + out := map[string]string{} + err := filepath.WalkDir(root, func(p string, d fs.DirEntry, err error) error { + if err != nil { + return err + } + if d.IsDir() { + return nil + } + info, err := d.Info() + if err != nil { + return err + } + if info.Mode()&os.ModeSymlink != 0 { + return nil + } + if !info.Mode().IsRegular() { + return nil + } + rel, err := filepath.Rel(root, p) + if err != nil { + return err + } + body, err := os.ReadFile(p) + if err != nil { + return err + } + sum := sha256.Sum256(body) + out[filepath.ToSlash(rel)] = hex.EncodeToString(sum[:]) + return nil + }) + if err != nil { + return nil, err + } + return out, nil +} + +// hashContainerTree reads every regular file under livePath via docker +// exec sh -c 'cd && find . -type f -not -name .complete | xargs -I {} sh -c "echo {}; sha256sum {}"'. +// +// The output is parsed line-by-line into rel-path → sha256-hex. +func (h *PluginsHandler) hashContainerTree( + ctx context.Context, containerName, livePath string, +) (map[string]string, error) { + out, err := h.execAsRoot(ctx, containerName, []string{ + "sh", "-c", + // Find regular files, hash each, output ` ./`. + // `cd` then `find .` keeps paths relative to livePath. + fmt.Sprintf("cd %s && find . -type f -print0 | xargs -0 -r sha256sum 2>/dev/null", shQuote(livePath)), + }) + if err != nil { + return nil, fmt.Errorf("hash container tree: %w", err) + } + hashes := map[string]string{} + for _, line := range strings.Split(out, "\n") { + line = strings.TrimSpace(line) + if line == "" { + continue + } + // sha256sum output: " " (two spaces). Path starts with "./". + parts := strings.SplitN(line, " ", 2) + if len(parts) != 2 { + continue + } + hash := parts[0] + rel := strings.TrimPrefix(parts[1], "./") + hashes[rel] = hash + } + return hashes, nil +} + +// shQuote single-quotes a string for safe insertion into a shell command. +// Returns the input unchanged if it's already shell-safe (alphanumeric + +// /._-). Otherwise wraps in single quotes and escapes inner '. +func shQuote(s string) string { + safe := true + for _, c := range s { + switch { + case c >= 'a' && c <= 'z': + case c >= 'A' && c <= 'Z': + case c >= '0' && c <= '9': + case c == '/' || c == '.' || c == '_' || c == '-': + default: + safe = false + } + if !safe { + break + } + } + if safe { + return s + } + return "'" + strings.ReplaceAll(s, "'", `'\''`) + "'" +} diff --git a/workspace-server/internal/handlers/plugins_classifier_test.go b/workspace-server/internal/handlers/plugins_classifier_test.go new file mode 100644 index 00000000..ff792fbb --- /dev/null +++ b/workspace-server/internal/handlers/plugins_classifier_test.go @@ -0,0 +1,121 @@ +package handlers + +import ( + "os" + "path/filepath" + "testing" +) + +// TestIsSkillMarkdown: pin which paths the classifier considers +// hot-reloadable. SKILL.md by basename only — case-sensitive. +func TestIsSkillMarkdown(t *testing.T) { + yes := []string{ + "SKILL.md", + "skills/foo/SKILL.md", + "deeply/nested/SKILL.md", + } + no := []string{ + "plugin.yaml", + "hooks.json", + "settings.json", + "README.md", + "skill.md", // case-sensitive + "SKILLS.md", // not a skill file + "skills/foo/extra.md", + } + for _, s := range yes { + if !isSkillMarkdown(s) { + t.Errorf("isSkillMarkdown(%q) = false; want true", s) + } + } + for _, s := range no { + if isSkillMarkdown(s) { + t.Errorf("isSkillMarkdown(%q) = true; want false", s) + } + } +} + +// TestHashLocalTree_StableHash: hashing the same content twice must +// produce identical maps. Pinned because if hashLocalTree ever picks up +// mtime/inode (e.g. via a refactor to use os.Lstat metadata), every +// install would classify as cold and we'd lose the hot-reload. +func TestHashLocalTree_StableHash(t *testing.T) { + dir := t.TempDir() + if err := os.MkdirAll(filepath.Join(dir, "skills/foo"), 0o755); err != nil { + t.Fatal(err) + } + if err := os.WriteFile(filepath.Join(dir, "plugin.yaml"), []byte("name: foo\n"), 0o644); err != nil { + t.Fatal(err) + } + if err := os.WriteFile(filepath.Join(dir, "skills/foo/SKILL.md"), []byte("# Foo\n"), 0o644); err != nil { + t.Fatal(err) + } + + h1, err := hashLocalTree(dir) + if err != nil { + t.Fatal(err) + } + h2, err := hashLocalTree(dir) + if err != nil { + t.Fatal(err) + } + if len(h1) != len(h2) { + t.Fatalf("hash count differs: %d vs %d", len(h1), len(h2)) + } + for k, v := range h1 { + if h2[k] != v { + t.Errorf("hash[%q] differs: %q vs %q", k, v, h2[k]) + } + } +} + +// TestHashLocalTree_SymlinkSkipped: symlinks should not appear in the +// hash map — same posture as the tar walker. Otherwise a hostile plugin +// could include a symlink whose hash changes when its target changes, +// silently flipping classification. +func TestHashLocalTree_SymlinkSkipped(t *testing.T) { + dir := t.TempDir() + if err := os.WriteFile(filepath.Join(dir, "real.txt"), []byte("ok"), 0o644); err != nil { + t.Fatal(err) + } + target := filepath.Join(t.TempDir(), "target") + if err := os.WriteFile(target, []byte("outside"), 0o644); err != nil { + t.Fatal(err) + } + if err := os.Symlink(target, filepath.Join(dir, "link")); err != nil { + t.Fatal(err) + } + + h, err := hashLocalTree(dir) + if err != nil { + t.Fatal(err) + } + if _, exists := h["link"]; exists { + t.Errorf("symlink leaked into hash map: %v", h) + } + if _, exists := h["real.txt"]; !exists { + t.Errorf("real.txt missing from hash map: %v", h) + } +} + +// TestShQuote: the classifier injects livePath into a shell command via +// docker exec. Path must be quoted to handle pluginName entries with +// hyphens (which are safe but exercised here) and any future special- +// character edge case. Pin the safe-vs-quoted boundary. +func TestShQuote(t *testing.T) { + cases := []struct { + in, want string + }{ + {"foo", "foo"}, + {"/configs/plugins/foo-bar", "/configs/plugins/foo-bar"}, + {"with space", "'with space'"}, + {"with'quote", "'with'\\''quote'"}, + {"$envvar", "'$envvar'"}, + {"path/with/dots.txt", "path/with/dots.txt"}, + } + for _, tc := range cases { + if got := shQuote(tc.in); got != tc.want { + t.Errorf("shQuote(%q) = %q; want %q", tc.in, got, tc.want) + } + } +} diff --git a/workspace-server/internal/handlers/plugins_install_pipeline.go b/workspace-server/internal/handlers/plugins_install_pipeline.go index 7a59531e..8062e78f 100644 --- a/workspace-server/internal/handlers/plugins_install_pipeline.go +++ b/workspace-server/internal/handlers/plugins_install_pipeline.go @@ -276,6 +276,15 @@ func (h *PluginsHandler) resolveAndStage(ctx context.Context, req installRequest // using NewPluginsHandler without a DB; production wires it in router.go. func (h *PluginsHandler) deliverToContainer(ctx context.Context, workspaceID string, r *stageResult) error { if containerName := h.findRunningContainer(ctx, workspaceID); containerName != "" { + // Hot-reload classifier (molecule-core#112) — decide BEFORE the + // install whether this update can skip restartFunc. SKILL.md + // content changes are filesystem-visible to Claude Code on the + // next Skill invocation; hooks / settings.json / plugin.yaml / + // added-or-removed files need a container restart. + // Classifier reads live tree from container; on any read error + // it returns kindCold so we never hot-reload speculatively. + kind, _ := h.classifyInstallChanges(ctx, containerName, r.StagedDir, r.PluginName) + // Atomic stage→snapshot→swap→marker (molecule-core#114). // Replaces the prior single docker.CopyToContainer write that // left a partially-extracted tree on mid-install failure with @@ -290,7 +299,11 @@ func (h *PluginsHandler) deliverToContainer(ctx context.Context, workspaceID str "chown", "-R", "1000:1000", "/configs/plugins/" + r.PluginName, }) if h.restartFunc != nil { - go h.restartFunc(workspaceID) + if kind == classifyKindSkillContentOnly { + log.Printf("Plugin install: %s → workspace %s — SKILL-content-only update, SKIPPING restart", r.PluginName, workspaceID) + } else { + go h.restartFunc(workspaceID) + } } return nil }