mirror of
https://github.com/tiennm99/goclaw.git
synced 2026-10-11 03:13:24 +00:00
fix(exec): surface real cwd error + auto-normalize stale workspace paths (#77)
* 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).
This commit is contained in:
1 parent
93461e9863
commit
d4463cde6f
7 files changed
+324
-6
No files matched your search
@@ -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
|
||||
|
||||
@@ -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: <errno-string>"
|
||||
// — 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
|
||||
|
||||
@@ -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
|
||||
}
|
||||
@@ -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: <errno-string>"
|
||||
// — 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.
|
||||
|
||||
@@ -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(), "/")
|
||||
}
|
||||
@@ -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")
|
||||
}
|
||||
}
|
||||
@@ -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)
|
||||
}
|
||||
Reference in new issue
Block a user