From d4f57aff7aca3aa731188c5ec972727b90a07b2d Mon Sep 17 00:00:00 2001 From: ZeroIce <39822906+vicksiyi@users.noreply.github.com> Date: Fri, 3 Jul 2026 12:35:03 +0800 Subject: [PATCH] MUL-3944: Fix daemon agent discovery around hook wrappers (#4817) * Fix agent path discovery around hook wrappers Co-authored-by: multica-agent * Fix login shell hook wrapper discovery Co-authored-by: multica-agent --------- Co-authored-by: multica-agent --- CLI_AND_DAEMON.md | 2 + server/internal/daemon/config.go | 116 +++++++++++++++++++++++-- server/internal/daemon/config_test.go | 118 ++++++++++++++++++++++++++ 3 files changed, 231 insertions(+), 5 deletions(-) diff --git a/CLI_AND_DAEMON.md b/CLI_AND_DAEMON.md index 29924109f..7ff12a8ce 100644 --- a/CLI_AND_DAEMON.md +++ b/CLI_AND_DAEMON.md @@ -227,6 +227,8 @@ Agent-specific overrides: | `MULTICA_TRAECLI_PATH` | Custom path to the `traecli` binary | | `MULTICA_TRAECLI_MODEL` | Override the Trae model used (a model id from your logged-in traecli catalog, e.g. `Doubao-Seed-2.1-Pro`) | +If a previously generated `~/.multica/hooks` wrapper is first on `PATH` and calls the same command name again, the daemon skips that hooks directory during built-in agent discovery and records the real binary path behind it. If your interactive shell still recurses when you run `claude`, `codex`, or `hermes` manually, remove the hooks entry from your shell startup file or replace the wrapper body with an absolute `exec /path/to/real-binary "$@"`. + The daemon launches Qoder as `qodercli --yolo --acp`, matching Qoder’s ACP “bypass permissions” mode so tool runs do not block on interactive approval in headless runs. `MULTICA_CLAUDE_ARGS` and `MULTICA_CODEX_ARGS` are parsed with POSIX shellword quoting, so values such as `--model "gpt-5.1 codex" --sandbox read-only` are split like a shell command line. Agent arguments are applied in this order: hardcoded Multica defaults, daemon-wide env defaults, then per-agent `custom_args` from the task. diff --git a/server/internal/daemon/config.go b/server/internal/daemon/config.go index 06139c99c..7ac96c9eb 100644 --- a/server/internal/daemon/config.go +++ b/server/internal/daemon/config.go @@ -222,9 +222,9 @@ func LoadConfig(overrides Overrides) (Config, error) { } probe := func(envVar, defaultCmd, modelEnv string) (AgentEntry, bool) { cmd := envOrDefault(envVar, defaultCmd) - if _, err := exec.LookPath(cmd); err == nil { + if path, err := resolveAgentExecutablePath(cmd); err == nil { return AgentEntry{ - Path: cmd, + Path: path, Model: strings.TrimSpace(os.Getenv(modelEnv)), }, true } @@ -298,9 +298,9 @@ func LoadConfig(overrides Overrides) (Config, error) { agents["antigravity"] = e } qoderPath := envOrDefault("MULTICA_QODER_PATH", "qodercli") - if _, err := exec.LookPath(qoderPath); err == nil { + if path, err := resolveAgentExecutablePath(qoderPath); err == nil { agents["qoder"] = AgentEntry{ - Path: qoderPath, + Path: path, Model: strings.TrimSpace(os.Getenv("MULTICA_QODER_MODEL")), } } @@ -654,6 +654,98 @@ func shellArgsFromEnv(name string) ([]string, error) { return args, nil } +// resolveAgentExecutablePath returns the concrete executable path the daemon +// should keep for an agent command. Bare command names are pinned to the path +// resolved during startup so later PATH changes cannot redirect task launches. +// When ~/.multica/hooks shadows a real agent binary, skip that hooks directory: +// previously generated hook wrappers can execute the same command name and +// recurse forever if the daemon records or launches the wrapper. +func resolveAgentExecutablePath(cmd string) (string, error) { + resolved, err := exec.LookPath(cmd) + if err != nil { + return "", err + } + if strings.ContainsAny(cmd, "/\\") { + return resolved, nil + } + if isInMulticaHooksDir(resolved) { + if unshadowed, err := lookPathExcludingMulticaHooks(cmd); err == nil { + return unshadowed, nil + } + } + return canonicalExecutablePath(resolved), nil +} + +func lookPathExcludingMulticaHooks(cmd string) (string, error) { + for _, dir := range filepath.SplitList(os.Getenv("PATH")) { + if dir == "" { + dir = "." + } + if isMulticaHooksDir(dir) { + continue + } + candidate := filepath.Join(dir, cmd) + if isExecutableFile(candidate) { + return canonicalExecutablePath(candidate), nil + } + } + return "", exec.ErrNotFound +} + +func isInMulticaHooksDir(path string) bool { + if path == "" { + return false + } + return isMulticaHooksDir(filepath.Dir(path)) +} + +func isMulticaHooksDir(dir string) bool { + home, err := os.UserHomeDir() + if err != nil || home == "" { + return false + } + return samePathDir(dir, filepath.Join(home, ".multica", "hooks")) +} + +func samePathDir(a, b string) bool { + absA, err := filepath.Abs(a) + if err != nil { + return false + } + absB, err := filepath.Abs(b) + if err != nil { + return false + } + absA = filepath.Clean(absA) + absB = filepath.Clean(absB) + if realA, err := filepath.EvalSymlinks(absA); err == nil { + absA = realA + } + if realB, err := filepath.EvalSymlinks(absB); err == nil { + absB = realB + } + return absA == absB +} + +func canonicalExecutablePath(path string) string { + abs, err := filepath.Abs(path) + if err != nil { + return path + } + if real, err := filepath.EvalSymlinks(abs); err == nil { + return real + } + return abs +} + +func isExecutableFile(path string) bool { + info, err := os.Stat(path) + if err != nil || info.IsDir() { + return false + } + return info.Mode()&0o111 != 0 +} + // defaultAgentCommandNames lists the command names the agent probe loop tries // before any MULTICA_*_PATH override is applied. Kept in sync with the // `probe(...)` calls in LoadConfig — the shell-fallback resolver uses this @@ -806,7 +898,9 @@ func resolveAgentsViaLoginShell(names []string) map[string]string { // than hand back garbage), // 5. canonicalises the directory via `cd ... && pwd -P` so symlinked prefix // dirs (fnm/nvm/volta) collapse to stable paths, -// 6. prints `\t` one entry per line for the caller. +// 6. if the resolved path lives in ~/.multica/hooks, searches the same +// shell-expanded PATH for the first executable outside that hooks dir, +// 7. prints `\t` one entry per line for the caller. // // Why steps 2 is important — and why this PR's first revision missed #2512: // the motivating case has `alias claude=...` in ~/.zshrc *and* fnm's real @@ -835,6 +929,18 @@ func buildLoginShellResolveScript(names []string) string { b.WriteString(" [ -n \"$p\" ] || continue\n") b.WriteString(" case \"$p\" in /*) ;; *) continue ;; esac\n") b.WriteString(" d=$(dirname \"$p\") && f=$(basename \"$p\") && c=$(cd \"$d\" 2>/dev/null && pwd -P) || continue\n") + b.WriteString(" hc=\"\"\n") + b.WriteString(" if [ -n \"${HOME:-}\" ]; then hd=\"$HOME/.multica/hooks\"; hc=$(cd \"$hd\" 2>/dev/null && pwd -P) || hc=\"\"; fi\n") + b.WriteString(" if [ -n \"$hc\" ] && [ \"$c\" = \"$hc\" ]; then\n") + b.WriteString(" oldIFS=$IFS; IFS=:\n") + b.WriteString(" for d2 in $PATH; do\n") + b.WriteString(" [ -n \"$d2\" ] || d2=.\n") + b.WriteString(" c2=$(cd \"$d2\" 2>/dev/null && pwd -P) || continue\n") + b.WriteString(" [ \"$c2\" = \"$hc\" ] && continue\n") + b.WriteString(" if [ -f \"$c2/$n\" ] && [ -x \"$c2/$n\" ]; then c=\"$c2\"; f=\"$n\"; break; fi\n") + b.WriteString(" done\n") + b.WriteString(" IFS=$oldIFS\n") + b.WriteString(" fi\n") b.WriteString(" printf '%s\\t%s\\n' \"$n\" \"$c/$f\"\n") b.WriteString("done\n") return b.String() diff --git a/server/internal/daemon/config_test.go b/server/internal/daemon/config_test.go index c2e9c900c..9be8967bf 100644 --- a/server/internal/daemon/config_test.go +++ b/server/internal/daemon/config_test.go @@ -243,6 +243,124 @@ func stageFakeAgent(t *testing.T) string { return binDir } +func TestLoadConfig_SkipsMulticaHooksShadowingAgentBinaries(t *testing.T) { + if runtime.GOOS == "windows" { + t.Skip("POSIX shell not available on Windows") + } + + home := t.TempDir() + t.Setenv("HOME", home) + hooksDir := filepath.Join(home, ".multica", "hooks") + if err := os.MkdirAll(hooksDir, 0o755); err != nil { + t.Fatalf("create hooks dir: %v", err) + } + realBinDir := t.TempDir() + + for _, name := range []string{"claude", "codex", "hermes"} { + hookPath := filepath.Join(hooksDir, name) + hookBody := "#!/bin/sh\nexec " + name + " \"$@\"\n" + if err := os.WriteFile(hookPath, []byte(hookBody), 0o755); err != nil { + t.Fatalf("write hook wrapper %s: %v", name, err) + } + realPath := filepath.Join(realBinDir, name) + if err := os.WriteFile(realPath, []byte("#!/bin/sh\nexit 0\n"), 0o755); err != nil { + t.Fatalf("write real binary %s: %v", name, err) + } + } + + t.Setenv("PATH", hooksDir+string(os.PathListSeparator)+realBinDir) + t.Setenv("SHELL", filepath.Join(t.TempDir(), "fish")) + t.Setenv("MULTICA_DAEMON_ID", "11111111-1111-1111-1111-111111111111") + + cfg, err := LoadConfig(Overrides{ + ServerURL: "http://localhost:0", + WorkspacesRoot: t.TempDir(), + }) + if err != nil { + t.Fatalf("LoadConfig: %v", err) + } + + for provider, binary := range map[string]string{ + "claude": "claude", + "codex": "codex", + "hermes": "hermes", + } { + got, ok := cfg.Agents[provider] + if !ok { + t.Fatalf("expected %s agent in config, got %#v", provider, cfg.Agents) + } + want := canonicalExecutablePath(filepath.Join(realBinDir, binary)) + if got.Path != want { + t.Errorf("%s path = %q, want unshadowed real binary %q", provider, got.Path, want) + } + if strings.HasPrefix(got.Path, hooksDir) { + t.Errorf("%s path still points into hooks dir: %q", provider, got.Path) + } + } +} + +func TestLoadConfig_SkipsMulticaHooksFromLoginShellFallback(t *testing.T) { + if runtime.GOOS == "windows" { + t.Skip("POSIX shell not available on Windows") + } + sh := "/bin/sh" + if _, err := os.Stat(sh); err != nil { + t.Skipf("no /bin/sh available: %v", err) + } + + home := t.TempDir() + t.Setenv("HOME", home) + hooksDir := filepath.Join(home, ".multica", "hooks") + if err := os.MkdirAll(hooksDir, 0o755); err != nil { + t.Fatalf("create hooks dir: %v", err) + } + realBinDir := t.TempDir() + hookPath := filepath.Join(hooksDir, "codex") + if err := os.WriteFile(hookPath, []byte("#!/bin/sh\nexec codex \"$@\"\n"), 0o755); err != nil { + t.Fatalf("write hook wrapper: %v", err) + } + realPath := filepath.Join(realBinDir, "codex") + if err := os.WriteFile(realPath, []byte("#!/bin/sh\nexit 0\n"), 0o755); err != nil { + t.Fatalf("write real codex: %v", err) + } + + t.Setenv("PATH", "/usr/bin:/bin") + if _, err := exec.LookPath("codex"); err == nil { + t.Skip("PATH leak - codex already visible to daemon without shell fallback") + } + rc := filepath.Join(t.TempDir(), "sh.rc") + rcBody := "export PATH=\"" + hooksDir + string(os.PathListSeparator) + realBinDir + ":$PATH\"\n" + if err := os.WriteFile(rc, []byte(rcBody), 0o644); err != nil { + t.Fatalf("write shell rc: %v", err) + } + t.Setenv("SHELL", sh) + t.Setenv("ENV", rc) + t.Setenv("MULTICA_DAEMON_ID", "11111111-1111-1111-1111-111111111111") + pinNonCodexAgentsToMissingPaths(t) + oldBundlePaths := codexDesktopAppBundlePaths + codexDesktopAppBundlePaths = func() []string { return nil } + t.Cleanup(func() { codexDesktopAppBundlePaths = oldBundlePaths }) + + cfg, err := LoadConfig(Overrides{ + ServerURL: "http://localhost:0", + WorkspacesRoot: t.TempDir(), + }) + if err != nil { + t.Fatalf("LoadConfig: %v", err) + } + got, ok := cfg.Agents["codex"] + if !ok { + t.Fatalf("expected codex from login-shell fallback, got %#v", cfg.Agents) + } + want := canonicalExecutablePath(realPath) + if got.Path != want { + t.Fatalf("codex path = %q, want unshadowed real binary %q", got.Path, want) + } + if strings.HasPrefix(got.Path, hooksDir) { + t.Fatalf("codex path still points into hooks dir: %q", got.Path) + } +} + // TestLoadConfig_AutoUpdateDefault_SelfHostOff is the regression guard for // MUL-2381: a daemon pointed at any non-cloud server URL must default // AutoUpdateEnabled to false, because self-host operators frequently run a