From d4463cde6f9764da034d94d493df6cf4c9b9dc42 Mon Sep 17 00:00:00 2001 From: Duy /zuey/ Date: Wed, 27 May 2026 11:52:25 +0700 Subject: [PATCH] fix(exec): surface real cwd error + auto-normalize stale workspace paths (#77) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * fix(exec): surface chdir error instead of misleading fork/exec Linux Go forkAndExecInChild conflates chdir + execve failures into one PathError naming only argv0. When agent workspace points to a stale directory, exec reports "fork/exec /usr/bin/gh: no such file" even though the binary exists — the real failure is chdir on cmd.Dir. Two fixes: - loop_context.go: fall back to l.workspace when per-user MkdirAll fails (3 sites: user workspace, dispatched team, auto-resolved team) - shell.go + credentialed_exec.go: preflight cwd via validateExecCwd before exec.Command, so the error names the directory, not the binary Regression test TestValidateExecCwd covers empty / existing / missing / file-instead-of-dir. * feat(upgrade): auto-normalize stale agent workspace paths Detects non-portable workspace values left by deployment migrations (Docker → bare-metal, host path drift) and rewrites them to the current configured base. Registered as data hook 072_normalize_agent_workspaces; runs automatically after `migrate up`. Stale patterns: - /app/workspace/* (Docker container path persisted on bare-metal) - ~ prefix (Go doesn't expand tildes; MkdirAll creates literal ~ dir) Base resolved with gateway precedence: GOCLAW_WORKSPACE env > config.Agents.Defaults.Workspace > default ~/.goclaw/workspace. Idempotent (skips rows already at proposed value). Conservative (leaves absolute non-stale paths untouched as intentional custom config). --- internal/agent/loop_context.go | 30 ++++- internal/tools/credentialed_exec.go | 38 ++++++ internal/tools/credentialed_exec_test.go | 61 +++++++++ internal/tools/shell.go | 8 ++ internal/upgrade/hook_workspace_normalize.go | 126 ++++++++++++++++++ .../upgrade/hook_workspace_normalize_test.go | 66 +++++++++ internal/upgrade/hooks.go | 1 + 7 files changed, 324 insertions(+), 6 deletions(-) create mode 100644 internal/upgrade/hook_workspace_normalize.go create mode 100644 internal/upgrade/hook_workspace_normalize_test.go diff --git a/internal/agent/loop_context.go b/internal/agent/loop_context.go index 03bade8e..80d166cd 100644 --- a/internal/agent/loop_context.go +++ b/internal/agent/loop_context.go @@ -177,7 +177,15 @@ func (l *Loop) injectContext(ctx context.Context, req *RunRequest) (contextSetup ctx = store.WithSharedSessions(ctx) } if err := os.MkdirAll(effectiveWorkspace, 0755); err != nil { - slog.Warn("failed to create user workspace directory", "workspace", effectiveWorkspace, "user", req.UserID, "error", err) + // Stale stored workspace (e.g. Docker-era /app/workspace/ on bare-metal host) + // would propagate as cmd.Dir into exec tools, where Linux's clone+chdir+execve + // failure surfaces as a misleading "fork/exec PATH: no such file or directory" + // — same message users would see for a missing binary. Fall back to the system + // default workspace (already created at startup) so tools keep working while + // the warning surfaces the data drift for operators. + slog.Warn("failed to create user workspace directory; falling back to system default", + "workspace", effectiveWorkspace, "fallback", l.workspace, "user", req.UserID, "error", err) + effectiveWorkspace = l.workspace } ctx = tools.WithToolWorkspace(ctx, effectiveWorkspace) } else if l.workspace != "" { @@ -187,10 +195,15 @@ func (l *Loop) injectContext(ctx context.Context, req *RunRequest) (contextSetup // Team workspace: dispatched task overrides default workspace. if req.TeamWorkspace != "" { if err := os.MkdirAll(req.TeamWorkspace, 0755); err != nil { - slog.Warn("failed to create team workspace directory", "workspace", req.TeamWorkspace, "error", err) + // See note above on loop_context user workspace fallback. A broken + // req.TeamWorkspace would otherwise become cmd.Dir and surface as + // "fork/exec PATH: no such file or directory" from any tool exec. + slog.Warn("failed to create team workspace directory; keeping previous workspace", + "workspace", req.TeamWorkspace, "error", err) + } else { + ctx = tools.WithToolTeamWorkspace(ctx, req.TeamWorkspace) + ctx = tools.WithToolWorkspace(ctx, req.TeamWorkspace) } - ctx = tools.WithToolTeamWorkspace(ctx, req.TeamWorkspace) - ctx = tools.WithToolWorkspace(ctx, req.TeamWorkspace) } if req.TeamID != "" { ctx = tools.WithToolTeamID(ctx, req.TeamID) @@ -235,9 +248,14 @@ func (l *Loop) injectContext(ctx context.Context, req *RunRequest) (contextSetup tools.UserChatLayer(wsChat, shared), ) if err := os.MkdirAll(wsDir, 0750); err != nil { - slog.Warn("failed to create team workspace directory", "workspace", wsDir, "error", err) + // See note above on loop_context user workspace fallback. Skip the + // team workspace context-set so downstream tools don't inherit a + // path that would fail with a misleading "fork/exec PATH: ENOENT". + slog.Warn("failed to create team workspace directory; skipping team workspace ctx", + "workspace", wsDir, "error", err) + } else { + ctx = tools.WithToolTeamWorkspace(ctx, wsDir) } - ctx = tools.WithToolTeamWorkspace(ctx, wsDir) // Team root (no UserChatLayer): lets any team agent — leader or member — // read files produced by peers under different chat/user scopes within // the same team. Writes still default to wsDir above; team root only diff --git a/internal/tools/credentialed_exec.go b/internal/tools/credentialed_exec.go index 44efd721..7a6a0435 100644 --- a/internal/tools/credentialed_exec.go +++ b/internal/tools/credentialed_exec.go @@ -452,6 +452,36 @@ func mergeCredentialedEnv(cred *store.SecureCLIBinary) (map[string]string, error return envMap, nil } +// validateExecCwd checks that cmd.Dir exists and is a directory before +// cmd.Start(). Empty cwd is allowed (Go interprets as "inherit parent's dir"). +// +// Why: on Linux, Go's syscall.forkAndExecInChild reports any child-side error +// (chdir, execve, missing PT_INTERP, etc.) as `&PathError{Op: "fork/exec", +// Path: argv0, Err: errno}`. The label `fork/exec PATH:` always names the +// binary even when chdir was the actual failure. A stale stored workspace +// (e.g. /app/workspace/clax persisted from a Docker-era deployment but used +// on a bare-metal host) then surfaces as `fork/exec /usr/bin/gh: no such file +// or directory` — sending operators down the wrong investigation path. +// +// Returns nil when cwd is empty, exists and is a directory; otherwise an +// error naming the actual problem with the working directory. +func validateExecCwd(cwd string) error { + if cwd == "" { + return nil + } + info, err := os.Stat(cwd) + if err != nil { + if os.IsNotExist(err) { + return fmt.Errorf("working directory does not exist: %q", cwd) + } + return fmt.Errorf("working directory inaccessible: %q: %w", cwd, err) + } + if !info.IsDir() { + return fmt.Errorf("working directory is not a directory: %q", cwd) + } + return nil +} + // executeCredentialedHost runs a credentialed command directly on the host. // Uses exec.Command (no shell) with credentials as env vars. // ctx cancellation triggers SIGTERM → 3s grace → SIGKILL via process-group helpers. @@ -461,6 +491,14 @@ func (t *ExecTool) executeCredentialedHost(ctx context.Context, absPath string, ctx, cancel := context.WithTimeout(ctx, timeout) defer cancel() + // Pre-flight cmd.Dir validation. On Linux, Go's clone+chdir+execve failure + // path collapses every child-side error into "fork/exec PATH: " + // — so a missing cmd.Dir surfaces as if absPath itself were missing. Catch + // this case explicitly so the error names the real culprit. + if err := validateExecCwd(cwd); err != nil { + return ErrorResult(fmt.Sprintf("credentialed exec: %v (binary %s does exist)", err, absPath)) + } + // Plain exec.Command (not CommandContext) so we own the kill sequence. cmd := exec.Command(absPath, args...) cmd.Dir = cwd diff --git a/internal/tools/credentialed_exec_test.go b/internal/tools/credentialed_exec_test.go index 1b2886b0..9275d575 100644 --- a/internal/tools/credentialed_exec_test.go +++ b/internal/tools/credentialed_exec_test.go @@ -427,3 +427,64 @@ func TestResolveAndMatchBinaryFindsGoogleWorkspaceRuntimeBinary(t *testing.T) { t.Fatalf("path = %q, want %q", got, binaryPath) } } + +// TestValidateExecCwd guards against the misleading "fork/exec PATH: no such +// file or directory" that Linux Go surfaces when cmd.Dir is the actual +// culprit (chdir failure inside the cloned child). Pre-flighting cmd.Dir +// surfaces the real cause so operators don't chase missing-binary ghosts. +func TestValidateExecCwd(t *testing.T) { + t.Run("empty cwd is allowed", func(t *testing.T) { + if err := validateExecCwd(""); err != nil { + t.Fatalf("empty cwd: want nil, got %v", err) + } + }) + + t.Run("existing directory is allowed", func(t *testing.T) { + dir := t.TempDir() + if err := validateExecCwd(dir); err != nil { + t.Fatalf("existing dir: want nil, got %v", err) + } + }) + + t.Run("missing directory names the cwd, not the binary", func(t *testing.T) { + missing := filepath.Join(t.TempDir(), "does-not-exist") + err := validateExecCwd(missing) + if err == nil { + t.Fatal("missing cwd: want error, got nil") + } + // The error must mention "working directory" — that's the whole point. + // Without this guard, exec would surface "fork/exec /usr/bin/gh: no such file or directory" + // even when the binary is fine. + if got := err.Error(); !contains(got, "working directory") || !contains(got, missing) { + t.Fatalf("missing cwd: error = %q, want it to mention %q and 'working directory'", got, missing) + } + }) + + t.Run("file instead of directory is rejected", func(t *testing.T) { + f, err := os.CreateTemp(t.TempDir(), "notadir") + if err != nil { + t.Fatal(err) + } + f.Close() + err = validateExecCwd(f.Name()) + if err == nil { + t.Fatal("file as cwd: want error, got nil") + } + if got := err.Error(); !contains(got, "not a directory") { + t.Fatalf("file as cwd: error = %q, want 'not a directory'", got) + } + }) +} + +func contains(s, sub string) bool { + return len(sub) == 0 || (len(s) >= len(sub) && (s == sub || stringIndex(s, sub) >= 0)) +} + +func stringIndex(s, sub string) int { + for i := 0; i+len(sub) <= len(s); i++ { + if s[i:i+len(sub)] == sub { + return i + } + } + return -1 +} diff --git a/internal/tools/shell.go b/internal/tools/shell.go index 2674679c..16b5e70f 100644 --- a/internal/tools/shell.go +++ b/internal/tools/shell.go @@ -539,6 +539,14 @@ func (t *ExecTool) executeOnHost(ctx context.Context, command, cwd string) *Resu ctx, cancel := context.WithTimeout(ctx, timeout) defer cancel() + // Pre-flight cmd.Dir validation. On Linux, Go's clone+chdir+execve failure + // path collapses every child-side error into "fork/exec PATH: " + // — so a missing cmd.Dir surfaces as if the shell binary itself were missing. + // Catch this case explicitly so the error names the real culprit. + if err := validateExecCwd(cwd); err != nil { + return ErrorResult(fmt.Sprintf("exec: %v", err)) + } + // Use plain exec.Command (not CommandContext) so we control the kill sequence. // CommandContext would SIGKILL only the direct child, leaving forked grandchildren alive. // Route through the platform shell: cmd.exe on Windows, sh on POSIX. diff --git a/internal/upgrade/hook_workspace_normalize.go b/internal/upgrade/hook_workspace_normalize.go new file mode 100644 index 00000000..ba87b1a3 --- /dev/null +++ b/internal/upgrade/hook_workspace_normalize.go @@ -0,0 +1,126 @@ +package upgrade + +import ( + "context" + "database/sql" + "fmt" + "log/slog" + "os" + "path/filepath" + "strings" + + "github.com/nextlevelbuilder/goclaw/internal/config" +) + +const workspaceNormalizeHookName = "072_normalize_agent_workspaces" + +// normalizeAgentWorkspaces rewrites stale `agents.workspace` values that +// won't resolve on the current host. Triggered automatically when the +// deployment backend changes shape (Docker → bare-metal, host path drift). +// +// A workspace is considered stale when it is non-portable on the new host: +// - prefixed with `/app/workspace/` (Docker container path persisted across +// a Docker → bare-metal migration; `/app` does not exist on the host) +// - prefixed with `~` (Go does not expand tildes — Mkdir would create a +// literal `~` directory in cwd) +// +// Stale rows are rewritten to `{configured_base}/{agent_key}`, where the +// base is read with the same precedence used by the gateway: +// `GOCLAW_WORKSPACE` env > config file `Agents.Defaults.Workspace` > +// default `~/.goclaw/workspace` (with tilde expansion). +// +// Idempotent: rows already at the proposed value are skipped. +// Conservative: absolute, non-stale paths are left untouched so operators +// who intentionally placed an agent in a custom directory keep their config. +func normalizeAgentWorkspaces(ctx context.Context, db *sql.DB) error { + base := resolveWorkspaceBase() + if base == "" { + slog.Warn("workspace normalization: no base configured, skipping") + return nil + } + + rows, err := db.QueryContext(ctx, + `SELECT id, agent_key, workspace FROM agents WHERE deleted_at IS NULL`) + if err != nil { + return fmt.Errorf("query agents: %w", err) + } + defer rows.Close() + + type pending struct { + id, key, current, proposed string + } + var todo []pending + for rows.Next() { + var id, key, current string + if err := rows.Scan(&id, &key, ¤t); err != nil { + return fmt.Errorf("scan agent row: %w", err) + } + if !isStaleWorkspace(current) { + continue + } + proposed := filepath.Join(base, key) + if current == proposed { + continue + } + todo = append(todo, pending{id, key, current, proposed}) + } + if err := rows.Err(); err != nil { + return fmt.Errorf("iterate agent rows: %w", err) + } + + for _, p := range todo { + if _, err := db.ExecContext(ctx, + `UPDATE agents SET workspace = $1, updated_at = NOW() WHERE id = $2`, + p.proposed, p.id, + ); err != nil { + return fmt.Errorf("update agent %q: %w", p.key, err) + } + slog.Info("workspace normalized", + "agent_key", p.key, + "from", p.current, + "to", p.proposed, + ) + } + + slog.Info("workspace normalization complete", + "rewritten", len(todo), + "base", base, + ) + return nil +} + +// isStaleWorkspace reports whether the stored path is non-portable on the +// current host. See normalizeAgentWorkspaces for the full set of patterns. +func isStaleWorkspace(ws string) bool { + ws = strings.TrimSpace(ws) + if ws == "" { + return false + } + if ws == "/app/workspace" || strings.HasPrefix(ws, "/app/workspace/") { + return true + } + if strings.HasPrefix(ws, "~") { + return true + } + return false +} + +// resolveWorkspaceBase returns the configured workspace base directory. +// Matches the gateway's precedence so the hook normalizes to whatever the +// next startup will use. +func resolveWorkspaceBase() string { + if v := strings.TrimSpace(os.Getenv("GOCLAW_WORKSPACE")); v != "" { + return strings.TrimRight(config.ExpandHome(v), "/") + } + cfgPath := os.Getenv("GOCLAW_CONFIG") + if cfgPath == "" { + cfgPath = "config.json" + } + cfg, err := config.Load(cfgPath) + if err != nil { + slog.Warn("workspace normalization: load config failed, using default", + "path", cfgPath, "error", err) + return strings.TrimRight(config.ExpandHome("~/.goclaw/workspace"), "/") + } + return strings.TrimRight(cfg.WorkspacePath(), "/") +} diff --git a/internal/upgrade/hook_workspace_normalize_test.go b/internal/upgrade/hook_workspace_normalize_test.go new file mode 100644 index 00000000..69f4c1e5 --- /dev/null +++ b/internal/upgrade/hook_workspace_normalize_test.go @@ -0,0 +1,66 @@ +package upgrade + +import ( + "os" + "testing" +) + +func TestIsStaleWorkspace(t *testing.T) { + cases := []struct { + name string + in string + want bool + }{ + {"docker era app workspace", "/app/workspace/clax", true}, + {"docker era root", "/app/workspace", true}, + {"docker era trailing slash", "/app/workspace/", true}, + {"tilde literal", "~/.goclaw/x-workspace", true}, + {"tilde only", "~", true}, + {"absolute host path", "/var/lib/goclaw/workspace/clax", false}, + {"current dot", ".", false}, + {"empty", "", false}, + {"whitespace", " ", false}, + {"custom absolute (preserve)", "/srv/agents/clax", false}, + {"relative non-tilde", "data/workspace", false}, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + if got := isStaleWorkspace(tc.in); got != tc.want { + t.Fatalf("isStaleWorkspace(%q) = %v, want %v", tc.in, got, tc.want) + } + }) + } +} + +func TestResolveWorkspaceBase_EnvWins(t *testing.T) { + t.Setenv("GOCLAW_WORKSPACE", "/var/lib/goclaw/workspace") + if got := resolveWorkspaceBase(); got != "/var/lib/goclaw/workspace" { + t.Fatalf("resolveWorkspaceBase() = %q, want /var/lib/goclaw/workspace", got) + } +} + +func TestResolveWorkspaceBase_EnvTilde(t *testing.T) { + t.Setenv("GOCLAW_WORKSPACE", "~/custom/ws") + home, _ := os.UserHomeDir() + want := home + "/custom/ws" + if got := resolveWorkspaceBase(); got != want { + t.Fatalf("resolveWorkspaceBase() = %q, want %q", got, want) + } +} + +func TestResolveWorkspaceBase_StripsTrailingSlash(t *testing.T) { + t.Setenv("GOCLAW_WORKSPACE", "/var/lib/goclaw/workspace/") + if got := resolveWorkspaceBase(); got != "/var/lib/goclaw/workspace" { + t.Fatalf("resolveWorkspaceBase() = %q, want trimmed", got) + } +} + +func TestResolveWorkspaceBase_FallbackOnMissingConfig(t *testing.T) { + t.Setenv("GOCLAW_WORKSPACE", "") + t.Setenv("GOCLAW_CONFIG", "/nonexistent/path/config.json") + // Should not return empty even if config file missing — falls back through + // config.Load's IsNotExist branch (which returns defaults) or to default. + if got := resolveWorkspaceBase(); got == "" { + t.Fatal("resolveWorkspaceBase() returned empty for missing config; expected fallback") + } +} diff --git a/internal/upgrade/hooks.go b/internal/upgrade/hooks.go index 6f9b36c3..7e74040c 100644 --- a/internal/upgrade/hooks.go +++ b/internal/upgrade/hooks.go @@ -21,4 +21,5 @@ func init() { RegisterDataHook(55, webSearchMigrateHookName, func(ctx context.Context, db *sql.DB) error { return migrateWebSearchInlineKeys(ctx, db) }) + RegisterDataHook(72, workspaceNormalizeHookName, normalizeAgentWorkspaces) }