This is an automated email from the ASF dual-hosted git repository. hsluoyz pushed a commit to branch master in repository https://gitbox.apache.org/repos/asf/casbin-gateway.git
commit 78ee914c0a13cc962085bf8a47f1adef904b70a3 Author: Yang Luo <[email protected]> AuthorDate: Sat Aug 15 14:02:43 2026 +0800 fix(agentpatch): never wedge unpatch when a config changed after patch verifyPatchOwnership refused the whole revert when any patched file no longer hashed to what Patch wrote. Claude Desktop rewrites claude_desktop_config.json whenever the user edits their MCP servers, so that condition is reached in normal use, and because Apply reverts first the installation could then neither be unpatched nor patched again, with no force option and no guidance. rollback now restores only the files the patch still owns, leaves externally changed files exactly as they are, and always clears the tracked state, so a target can never get stuck. Skipped files are reported as PartialRevertError so the operator is told what to clean up by hand. The Claude Desktop patcher no longer backs up the whole config either. It owns one mcpServers key and adds or deletes exactly that, which is reliable no matter what else rewrote the file, and it discards manifests left by the old scheme without restoring them over the current configuration. --- agentpatch/claude_desktop.go | 94 ++++++++++++++++++++++----------- agentpatch/state.go | 120 ++++++++++++++++++++++++++++--------------- 2 files changed, 143 insertions(+), 71 deletions(-) diff --git a/agentpatch/claude_desktop.go b/agentpatch/claude_desktop.go index c60326e..c53a284 100644 --- a/agentpatch/claude_desktop.go +++ b/agentpatch/claude_desktop.go @@ -39,6 +39,10 @@ func (claudeDesktopPatcher) AgentId() string { return "claude-desktop" } func (claudeDesktopPatcher) Supported() bool { return true } +// Patch registers exactly one MCP server entry and leaves the rest of +// claude_desktop_config.json alone. Claude Desktop rewrites that file whenever +// the user edits their servers, so a whole-file backup could never be restored +// safely; owning a single key keeps Patch and Unpatch reliable regardless. func (p claudeDesktopPatcher) Patch(target Target) error { configPath, err := p.configPath(target) if err != nil { @@ -48,52 +52,85 @@ func (p claudeDesktopPatcher) Patch(target Target) error { if err != nil { return err } - if err := Apply(target, func(changes *ChangeSet) error { - if err := changes.MkdirAll(filepath.Dir(configPath)); err != nil { - return err - } - config, err := readJSONObject(changes, configPath) - if err != nil { - return err - } - servers, exists := config["mcpServers"] - if !exists { - servers = map[string]any{} - config["mcpServers"] = servers - } - object, ok := objectAt(servers) - if !ok { - return errors.New("mcpServers must be a JSON object") - } - object[claudeDesktopServerName] = entry - updated, err := json.MarshalIndent(config, "", " ") - if err != nil { - return err - } - return changes.WriteFile(configPath, append(updated, '\n'), 0o600) - }); err != nil { + + stateMutex.Lock() + err = updateClaudeDesktopServers(configPath, func(servers map[string]any) bool { + servers[claudeDesktopServerName] = entry + return true + }) + stateMutex.Unlock() + if err != nil { return err } + if runtime.GOOS != "windows" { return nil } if err := agentmonitor.EnableCoworkMonitor(target.Path, target.Owner); err != nil { - _ = Revert(target) + _ = p.Unpatch(target) return err } return nil } -func (claudeDesktopPatcher) Unpatch(target Target) error { - if err := Revert(target); err != nil { +func (p claudeDesktopPatcher) Unpatch(target Target) error { + configPath, err := p.configPath(target) + if err != nil { + return err + } + + stateMutex.Lock() + err = updateClaudeDesktopServers(configPath, func(servers map[string]any) bool { + if _, found := servers[claudeDesktopServerName]; !found { + return false + } + delete(servers, claudeDesktopServerName) + return true + }) + // Discard any manifest written by an earlier release that backed up the whole + // file, without restoring it over the user's current configuration. + discardStateLocked(target) + stateMutex.Unlock() + if err != nil { return err } + if runtime.GOOS == "windows" { return agentmonitor.DisableCoworkMonitor(target.Path) } return nil } +// updateClaudeDesktopServers applies mutate to the mcpServers object and writes +// the file back only when mutate reports a change. +func updateClaudeDesktopServers(configPath string, mutate func(map[string]any) bool) error { + config, mode, exists, err := readJSONConfig(configPath) + if err != nil { + return err + } + + servers, found := config["mcpServers"] + if !found { + servers = map[string]any{} + } + object, ok := objectAt(servers) + if !ok { + return errors.New("mcpServers must be a JSON object") + } + if !mutate(object) { + return nil + } + if len(object) == 0 { + delete(config, "mcpServers") + } else { + config["mcpServers"] = object + } + if !exists { + mode = 0o600 + } + return writeJSONConfig(configPath, config, mode) +} + func (p claudeDesktopPatcher) Status(target Target) (Status, error) { configPath, err := p.configPath(target) if err != nil { @@ -106,9 +143,6 @@ func (p claudeDesktopPatcher) Status(target Target) (Status, error) { if !exists || !hasClaudeDesktopServer(config) { return Status{Detail: "MCP recorder is not registered"}, nil } - if !IsApplied(target) { - return Status{Patched: true, Detail: "MCP recorder is registered outside Gateway"}, nil - } if runtime.GOOS != "windows" { return Status{Patched: true, Detail: "MCP recorder registered; restart Claude Desktop to apply it"}, nil } diff --git a/agentpatch/state.go b/agentpatch/state.go index acf9891..1ffabab 100644 --- a/agentpatch/state.go +++ b/agentpatch/state.go @@ -18,15 +18,21 @@ import ( "crypto/sha256" "encoding/hex" "encoding/json" + "errors" "fmt" "os" "path/filepath" + "strings" "sync" "time" "github.com/apache/casbin-gateway/conf" ) +// gatewayEntryName is the identifier Gateway writes into every agent +// configuration it touches, so operators can find and remove it by hand. +const gatewayEntryName = "casbin-gateway-agent-monitor" + type changeKind string const ( @@ -130,7 +136,9 @@ func Apply(target Target, apply func(*ChangeSet) error) error { stateMutex.Lock() defer stateMutex.Unlock() - if err := revertLocked(target); err != nil { + // A previous patch whose files were edited externally must not block a fresh + // one: revertLocked has already cleaned up the tracked state by that point. + if err := revertLocked(target); err != nil && !errors.As(err, new(*PartialRevertError)) { return err } changes := &ChangeSet{ @@ -158,18 +166,32 @@ func Revert(target Target) error { return revertLocked(target) } +// revertLocked restores what it can and always clears the tracked state, so an +// installation can never end up unable to unpatch and unable to re-patch. func revertLocked(target Target) error { saved, err := loadManifest(target) if err != nil || saved == nil { return err } - if err := rollback(saved, backupDir(target)); err != nil { - return err + rollbackErr := rollback(saved, backupDir(target)) + if rollbackErr != nil && !errors.As(rollbackErr, new(*PartialRevertError)) { + return rollbackErr } if err := os.Remove(manifestPath(target)); err != nil && !os.IsNotExist(err) { return err } - return os.RemoveAll(backupDir(target)) + if err := os.RemoveAll(backupDir(target)); err != nil { + return err + } + return rollbackErr +} + +// discardStateLocked drops a target's manifest and backups without restoring +// them. Patchers that own a precise entry rather than a whole file use it to +// clear state written by an earlier, backup-based release. +func discardStateLocked(target Target) { + _ = os.Remove(manifestPath(target)) + _ = os.RemoveAll(backupDir(target)) } // IsApplied reports whether Gateway has the manifest needed to restore target. @@ -180,58 +202,74 @@ func IsApplied(target Target) bool { return err == nil && saved != nil } +// rollback restores the files this patch still owns. A file that changed after +// Patch is left exactly as the operator or the agent left it: refusing the whole +// unpatch instead would strand the installation, because Apply reverts first and +// so a stale manifest would block re-patching too. Skipped files are reported so +// the caller can tell the operator what to clean up by hand. func rollback(saved *manifest, backups string) error { - if err := verifyPatchOwnership(saved); err != nil { - return err - } - var first error + var skipped []string for index := len(saved.Changes) - 1; index >= 0; index-- { item := saved.Changes[index] - var err error - switch item.Kind { - case changeDir: + if item.Kind == changeDir { _ = os.Remove(item.Path) - case changeFile: - if item.Backup == "" { - err = os.Remove(item.Path) - if os.IsNotExist(err) { - err = nil - } - } else if content, readErr := os.ReadFile(filepath.Join(backups, item.Backup)); readErr != nil { - err = readErr - } else { - err = os.WriteFile(item.Path, content, item.Mode) - if err == nil { - err = os.Chmod(item.Path, item.Mode) - } + continue + } + if !patchStillOwns(item) { + skipped = append(skipped, item.Path) + continue + } + + var err error + if item.Backup == "" { + err = os.Remove(item.Path) + if os.IsNotExist(err) { + err = nil + } + } else if content, readErr := os.ReadFile(filepath.Join(backups, item.Backup)); readErr != nil { + err = readErr + } else { + err = os.WriteFile(item.Path, content, item.Mode) + if err == nil { + err = os.Chmod(item.Path, item.Mode) } } if err != nil && first == nil { first = fmt.Errorf("restore %s: %w", item.Path, err) } } + if first == nil && len(skipped) != 0 { + return &PartialRevertError{Paths: skipped} + } return first } -func verifyPatchOwnership(saved *manifest) error { - for _, item := range saved.Changes { - if item.Kind != changeFile { - continue - } - content, err := os.ReadFile(item.Path) - if err != nil { - return fmt.Errorf("cannot safely unpatch %s: %w", item.Path, err) - } - info, err := os.Stat(item.Path) - if err != nil { - return fmt.Errorf("cannot safely unpatch %s: %w", item.Path, err) - } - if item.PatchedHash == "" || item.PatchedHash != contentHash(content) || item.PatchedMode != info.Mode().Perm() { - return fmt.Errorf("cannot safely unpatch %s because it changed after Patch", item.Path) - } +// PartialRevertError reports files that Unpatch deliberately left untouched +// because they changed after Patch. The patch state is still cleaned up, so the +// installation can be patched again. +type PartialRevertError struct { + Paths []string +} + +func (e *PartialRevertError) Error() string { + return fmt.Sprintf("monitoring was disabled, but %s changed after Patch and was left unmodified; remove any remaining %q entry by hand", + strings.Join(e.Paths, ", "), gatewayEntryName) +} + +// patchStillOwns reports whether a file is byte-for-byte what Patch wrote. +func patchStillOwns(item change) bool { + content, err := os.ReadFile(item.Path) + if err != nil { + // A file the patch created and something else deleted is already in the + // desired post-unpatch state; a file we cannot read is not ours to touch. + return os.IsNotExist(err) && item.Backup == "" } - return nil + info, err := os.Stat(item.Path) + if err != nil { + return false + } + return item.PatchedHash != "" && item.PatchedHash == contentHash(content) && item.PatchedMode == info.Mode().Perm() } func stateDir() string { --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
