diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index e1ee5b4729..33111a1609 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -483,7 +483,7 @@ jobs: run: | go test ./internal/adapter/managedtemp -count=1 go test ./internal/adapter/osfs -run '^TestADR_0281_.*' -count=1 - go test ./internal/app -run '^TestADR_0281_ManagedModeUnixAdmission$' -count=1 + go test ./internal/app -run '^Test(ADR_0281_ManagedModeUnixAdmission$|ManagedTempRecovery|LocalPlacementPropagatesRunnerErrors)' -count=1 GOOS=freebsd GOARCH=amd64 go test -c -o "$RUNNER_TEMP/osfs-freebsd.test" ./internal/adapter/osfs GOOS=freebsd GOARCH=amd64 go test -c -o "$RUNNER_TEMP/app-freebsd.test" ./internal/app diff --git a/docs/adr/0027-cloud-native.md b/docs/adr/0027-cloud-native.md index 5a107f16e6..1be04bbdd8 100644 --- a/docs/adr/0027-cloud-native.md +++ b/docs/adr/0027-cloud-native.md @@ -860,7 +860,7 @@ durable artifact survives and is reloaded), or **lost** (gone, possibly leaking) | 67 | Embedded mecatui perf admin listener (ADR 0254) | `cmd/mecatui/embed.Server` | process instance, only with `--perf` | ordinary perf binds owner-private `admin.sock` beside the private gRPC socket; explicit `--perf-addr` and addressless `--perf-mcp` bind loopback TCP (the latter ephemeral). `Server.Close` shuts down the HTTP server and removes the shared private runtime directory. Each instance has its own directory, so defaults do not collide | **reconstructible/reset-by-design**: a restart creates a fresh private directory/socket or ephemeral MCP URL; metrics/profile state is live process data, not session state. No List 2 row | `cmd/mecatui/embed/embed.go` (`setupPerf`, `listenPrivateUnix`, `Close`) | | 68 | mecak8s plaintext drain-only listener | `cmd/mecak8s` (`serve`) | process | bound before serving; `boundedShutdown` shuts down its `http.Server`, and deferred listener close releases it on every later startup failure | **reconstructible/reset-by-design**: listener state is process-local and recreated from `--drain-addr` (default `0.0.0.0:8082`); no session state exists | `cmd/mecak8s/serve.go` (`drainHTTPMux`, `serve`, `boundedShutdown`); ADR 0290 | | 69 | Worktree selector HMAC key (ADR 0291) | `app.Build` | process lifetime | no goroutine, registry, map, cache, or external handle; the random key dies with Build/process and needs no explicit cleanup | **reset-by-design**: the key and selectors are not persisted. Restart invalidates every selector, clients relist worktrees, and the server re-enumerates current choices for constant-time matching. There is no selector registry/map to reattach | `internal/app/build.go` (`Build`); `internal/adapter/server/worktree_selector.go` (`WorktreeSelectorIssuer`) | -| 70 | Managed temporary command/job reaper worker, root GC lock/completion record, and per-lease locks (ADR 0281) | `app.Build` / `managedtemp.Namespace` | process worker; durable private managed root | `Built.Close` cancels the worker and waits only `shutdown_reap_timeout`; each startup/periodic sweep is bounded by `reap_timeout`. The root lock is non-blocking, lease locks are acquired non-blockingly per candidate, and only validated unlocked leases past TTL are removed. System or unsupported modes construct no worker and make no mutation | **mixed**: the ticker/worker reset on restart; owner-only completion record and lease manifests persist. A crashed holder releases advisory locks, so a later interval-gated Build sweep can reclaim eligible residue; incomplete/cancelled scans publish no completion record. No List 2 row: this is adapter-owned cleanup coordination, not session/run state | `internal/app/managed_temp_worker.go` (`startManagedTempWorker`); `internal/adapter/managedtemp/reaper_unix.go` (`Sweep`) | +| 70 | Managed temporary namespace/workspace handles, command/job reaper worker, root GC lock/completion record, and per-lease locks (ADR 0281) | `app.Build` / `managedtemp.Namespace` | process handles and worker; durable private managed root | `Built.Close` cancels the worker and waits only `shutdown_reap_timeout`; each startup/periodic sweep is bounded by `reap_timeout`. Namespace recovery and operation-handle pinning share a mutex: reopening the exact configured private root retires the stale cached handle, while per-operation pins close on return and independently owned workspace/lease handles are never closed or rehomed beneath active users. Runner construction opens and immediately closes a validation workspace; each command/job allocation reopens through namespace recovery and its lease owns the workspace until `Lease.Close` (including failed removal). There is no Build-lifetime workspace registry. Writable child provider guards borrow the existing Config/provider without new handles or session state, revalidating configured shell construction before model invocation; fork builders propagate construction errors before driving a child. The root GC lock is non-blocking, lease locks are acquired non-blockingly per candidate, and only validated unlocked leases past TTL are removed. System or unsupported modes construct no worker and make no mutation | **mixed**: the ticker/worker reset on restart; owner-only completion record and lease manifests persist. Missing/replaced namespaces are revalidated and safely reopened or recreated on later opens/sweeps; unsafe replacements fail closed, and configured local Bash placement cannot silently become shell-less. Old allocations whose visible workspace was replaced fail rather than being adopted into the new namespace. A crashed holder releases advisory locks, so a later interval-gated Build sweep can reclaim eligible residue; incomplete/cancelled scans publish no completion record. No List 2 row: this is adapter-owned cleanup coordination, not session/run state | `internal/app/managed_temp_worker.go` (`startManagedTempWorker`); `internal/app/temporary_storage.go` (`managedTemporaryStorage`); `internal/adapter/managedtemp/namespace_unix.go` (`openRoot`, `refreshLocked`, `Close`); `internal/adapter/managedtemp/reaper_unix.go` (`Sweep`) | **Remote-login resource re-audit (List 1 / List 2 — ADR 0277).** Remote mecatui adds rows 64–66: the durable public registry plus per-registry/per-target flock domains, the encrypted credential source plus root-scoped keyring key/root flock, and the connected-target refresh/validator transport. These are adapter-owned credential or connection resources, not session/run state, so List 2 gains no row. All flocks are held per operation and process death releases them; their sentinel files persist. There is no cross-store journal, so the documented partial-state crash residual is not rehydratable session state. Registry and credential records are reattached from disk/keyring; validator and access-token state are derived again on the next connect. `mecatui` closes the refresh source, validator, and store before a target-switch restart completes. diff --git a/internal/adapter/forker/forker.go b/internal/adapter/forker/forker.go index f288ca3690..9bb5c00386 100644 --- a/internal/adapter/forker/forker.go +++ b/internal/adapter/forker/forker.go @@ -136,7 +136,7 @@ type Forker struct { // or nil when the child namespace has no shell. nil (the field) means the // forker was constructed without a runner builder, so EVERY forked child is // shell-less (the composition root wires the builder only when Bash is on). - newRunner childRunner + newRunner func(string) (tool.CommandRunner, error) // tmpBase is the parent directory under which child directories are created. // Empty means os.MkdirTemp's default (os.TempDir()). tmpBase string @@ -224,6 +224,17 @@ func WithDirtyOverlay() Option { // called once per Fork with the isolated child directory; it returns nil when the // child namespace should have no shell (e.g. the trust gate withheld it). func WithRunner(r childRunner) Option { + return func(f *Forker) { + f.newRunner = nil + if r != nil { + f.newRunner = func(root string) (tool.CommandRunner, error) { return r(root), nil } + } + } +} + +// WithRunnerError preserves configured-shell construction failures as fork errors. +// A nil runner with no error still means intentionally shell-less. +func WithRunnerError(r func(string) (tool.CommandRunner, error)) Option { return func(f *Forker) { f.newRunner = r } } @@ -349,7 +360,11 @@ func (f *Forker) childEnv(base tool.Environment, ws tool.Workspace) (tool.Enviro root := ws.Root() var runner tool.CommandRunner if f.newRunner != nil { - runner = f.newRunner(root) + var err error + runner, err = f.newRunner(root) + if err != nil { + return tool.Environment{}, fmt.Errorf("forker: configured child command runner: %w", err) + } } ref := session.EnvironmentRef{Kind: base.Ref().Kind, ID: root, Revision: base.Ref().Revision} return tool.NewEnvironment(ref, ws, memledger.New(), runner) diff --git a/internal/adapter/managedtemp/namespace_unix.go b/internal/adapter/managedtemp/namespace_unix.go index 56d8ffe184..6917b43d1e 100644 --- a/internal/adapter/managedtemp/namespace_unix.go +++ b/internal/adapter/managedtemp/namespace_unix.go @@ -17,6 +17,7 @@ import ( "os" "path/filepath" "strings" + "sync" "syscall" ) @@ -29,6 +30,7 @@ const ( // Namespace is a private managed root. Every mutation is relative to root. type Namespace struct { + mu sync.Mutex // serializes root recovery, handle pinning, and Close path string root *os.Root } @@ -54,10 +56,20 @@ func Open(path string) (*Namespace, error) { if err := ensurePrivateDir(parent, name); err != nil { return nil, fmt.Errorf("managedtemp: managed root: %w", err) } + info, err := parent.Lstat(name) + if err != nil { + return nil, err + } root, err := parent.OpenRoot(name) if err != nil { return nil, fmt.Errorf("managedtemp: open managed root: %w", err) } + opened, statErr := root.Stat(".") + visible, visibleErr := parent.Lstat(name) + if statErr != nil || visibleErr != nil || !os.SameFile(info, opened) || !os.SameFile(opened, visible) { + _ = root.Close() + return nil, errors.New("managedtemp: managed root replaced while opening") + } if err := validatePrivateDir(root, "."); err != nil { _ = root.Close() return nil, fmt.Errorf("managedtemp: validate managed root: %w", err) @@ -71,15 +83,61 @@ func Open(path string) (*Namespace, error) { } func (n *Namespace) initialize() error { + // Validate existing protocol objects before creating anything in an adopted root. + if err := validatePrivateDir(n.root, "workspaces"); err != nil && !errors.Is(err, fs.ErrNotExist) { + return err + } + if err := validatePrivateFile(n.root, "gc.lock"); err != nil && !errors.Is(err, fs.ErrNotExist) { + return err + } if err := ensurePrivateDir(n.root, "workspaces"); err != nil { return err } return ensurePrivateFile(n.root, "gc.lock") } +// openRoot pins an independent handle for one operation. Recovery is serialized, +// but a long sweep must not block workspace opens. Existing workspace and lease +// handles remain independently owned, never closed or rehomed by recovery. +func (n *Namespace) openRoot() (*os.Root, error) { + if n == nil { + return nil, errors.New("managedtemp: namespace is closed") + } + n.mu.Lock() + defer n.mu.Unlock() + if err := n.refreshLocked(); err != nil { + return nil, err + } + return n.root.OpenRoot(".") +} + +// refreshLocked reopens the exact configured path using the same checks as Open. +func (n *Namespace) refreshLocked() error { + if n.root == nil { + return errors.New("managedtemp: namespace is closed") + } + current, err := Open(n.path) + if err != nil { + return fmt.Errorf("managedtemp: reopen namespace: %w", err) + } + oldInfo, oldErr := n.root.Stat(".") + newInfo, newErr := current.root.Stat(".") + if oldErr == nil && newErr == nil && os.SameFile(oldInfo, newInfo) { + return current.Close() + } + old := n.root + n.root = current.root + return old.Close() +} + // Close releases the root handle. It does not delete managed data. func (n *Namespace) Close() error { - if n == nil || n.root == nil { + if n == nil { + return nil + } + n.mu.Lock() + defer n.mu.Unlock() + if n.root == nil { return nil } err := n.root.Close() @@ -94,9 +152,6 @@ func (n *Namespace) Path() string { return n.path } // identity. The digest key, never the raw identity, is used in the path. canonicalPath // is retained only in the owner-readable manifest for operator diagnosis. func (n *Namespace) OpenWorkspace(backend, identity, canonicalPath string) (*Workspace, error) { - if n == nil || n.root == nil { - return nil, errors.New("managedtemp: namespace is closed") - } if backend == "" || identity == "" { return nil, errors.New("managedtemp: workspace backend and identity are required") } @@ -104,10 +159,15 @@ func (n *Namespace) OpenWorkspace(backend, identity, canonicalPath string) (*Wor return nil, errors.New("managedtemp: canonical workspace path is required") } key := workspaceKey(backend, identity) - if err := validatePrivateDir(n.root, "workspaces"); err != nil { + namespaceRoot, err := n.openRoot() + if err != nil { + return nil, err + } + defer func() { _ = namespaceRoot.Close() }() + if err := validatePrivateDir(namespaceRoot, "workspaces"); err != nil { return nil, fmt.Errorf("managedtemp: workspaces: %w", err) } - workspaces, err := n.root.OpenRoot("workspaces") + workspaces, err := namespaceRoot.OpenRoot("workspaces") if err != nil { return nil, fmt.Errorf("managedtemp: open workspaces: %w", err) } @@ -148,7 +208,7 @@ func (n *Namespace) OpenWorkspace(backend, identity, canonicalPath string) (*Wor _ = root.Close() return nil, err } - if err := validateWorkspaceManifest(root, key); err != nil { + if err := validateWorkspace(w); err != nil { _ = root.Close() return nil, err } @@ -192,10 +252,12 @@ func (w *Workspace) WritePrivateFile(name string, data []byte) error { // WritePrivateFile creates a private root-level protocol record atomically. func (n *Namespace) WritePrivateFile(name string, data []byte) error { - if n == nil || n.root == nil { - return errors.New("managedtemp: namespace is closed") + root, err := n.openRoot() + if err != nil { + return err } - return writePrivateFile(n.root, name, data) + defer func() { _ = root.Close() }() + return writePrivateFile(root, name, data) } func workspaceManifestJSON(key, canonicalPath string) ([]byte, error) { @@ -203,6 +265,9 @@ func workspaceManifestJSON(key, canonicalPath string) ([]byte, error) { } func (w *Workspace) writeWorkspaceManifest(manifest []byte, canonicalPath string) error { + if err := validateVisibleWorkspace(w); err != nil { + return err + } info, err := w.root.Lstat("workspace.manifest") if errors.Is(err, fs.ErrNotExist) { return writePrivateFile(w.root, "workspace.manifest", manifest) diff --git a/internal/adapter/managedtemp/protocol_other.go b/internal/adapter/managedtemp/protocol_other.go index e1d43bd6b5..d5a8eb60f7 100644 --- a/internal/adapter/managedtemp/protocol_other.go +++ b/internal/adapter/managedtemp/protocol_other.go @@ -19,6 +19,9 @@ func (*Namespace) Close() error { return nil } func (*Namespace) OpenWorkspace(string, string, string) (*Workspace, error) { return nil, errUnsupported } +func (*Namespace) AllocateWorkspace(string, string, string, string) (*Lease, error) { + return nil, errUnsupported +} func (*Workspace) Close() error { return nil } func (*Workspace) Allocate(string) (*Lease, error) { return nil, errUnsupported } func (*Lease) Path() string { return "" } diff --git a/internal/adapter/managedtemp/protocol_unix.go b/internal/adapter/managedtemp/protocol_unix.go index a0b8b7c947..14c7993716 100644 --- a/internal/adapter/managedtemp/protocol_unix.go +++ b/internal/adapter/managedtemp/protocol_unix.go @@ -48,13 +48,14 @@ type sweepCompletion struct { // Lease is one locked command or job allocation. Its path is adapter-internal. type Lease struct { - path string - name string - id string - kind string - root *os.Root - lock *os.File - parent *Workspace + path string + name string + id string + kind string + root *os.Root + lock *os.File + parent *Workspace + ownsParent bool } // ID returns the opaque 96-bit random allocation ID. @@ -167,9 +168,28 @@ func (l *Lease) Close() error { err = closeErr } } + if l.ownsParent { + err = errors.Join(err, l.parent.Close()) + } return err } +// AllocateWorkspace recovers the namespace at allocation time. The lease owns +// its independent workspace handle and releases it on Close, not at process exit. +func (n *Namespace) AllocateWorkspace(backend, identity, canonicalPath, kind string) (*Lease, error) { + w, err := n.OpenWorkspace(backend, identity, canonicalPath) + if err != nil { + return nil, err + } + lease, err := w.Allocate(kind) + if err != nil { + _ = w.Close() + return nil, err + } + lease.ownsParent = true + return lease, nil +} + // Allocate creates and locks a private cmd or job lease before publishing its manifest. func (w *Workspace) Allocate(kind string) (*Lease, error) { if w == nil || w.root == nil { @@ -249,10 +269,16 @@ func (l *Lease) Remove() error { // ReadSweepCompletion reads the durable sweep record without changing it. func (n *Namespace) ReadSweepCompletion() (time.Time, error) { - if n == nil || n.root == nil { - return time.Time{}, errors.New("managedtemp: namespace is closed") + root, err := n.openRoot() + if err != nil { + return time.Time{}, err } - data, err := readPrivateFile(n.root, "last-successful-sweep.json") + defer func() { _ = root.Close() }() + return readSweepCompletion(root) +} + +func readSweepCompletion(root *os.Root) (time.Time, error) { + data, err := readPrivateFile(root, "last-successful-sweep.json") if err != nil { return time.Time{}, err } @@ -305,12 +331,43 @@ func replacePrivateFile(root *os.Root, name string, data []byte) error { } func validateWorkspace(w *Workspace) error { + if err := validateVisibleWorkspace(w); err != nil { + return err + } if err := validatePrivateDir(w.root, "."); err != nil { return err } return validateWorkspaceManifest(w.root, filepath.Base(w.path)) } +func validateVisibleWorkspace(w *Workspace) error { + // Reject a replaced namespace even for runners holding an older workspace. + // These are exactly the three private components: root/workspaces/key. + path := w.path + for range 3 { + info, err := os.Lstat(path) + if err != nil { + return err + } + if err := validatePrivateDirInfo(info); err != nil { + return err + } + path = filepath.Dir(path) + } + visible, err := os.Lstat(w.path) + if err != nil { + return err + } + opened, err := w.root.Stat(".") + if err != nil { + return err + } + if !os.SameFile(visible, opened) { + return errors.New("managedtemp: workspace replaced") + } + return nil +} + func validateLease(l *Lease) error { if err := validateWorkspace(l.parent); err != nil { return err diff --git a/internal/adapter/managedtemp/reaper_unix.go b/internal/adapter/managedtemp/reaper_unix.go index 89a7164aaa..1587d97e32 100644 --- a/internal/adapter/managedtemp/reaper_unix.go +++ b/internal/adapter/managedtemp/reaper_unix.go @@ -32,16 +32,18 @@ type SweepResult struct { // Sweep performs one non-blocking, root-coordinated recovery pass. It never // waits for an active command's lease lock and only removes validated leases. func (n *Namespace) Sweep(ctx context.Context, opts SweepOptions) (SweepResult, error) { - if n == nil || n.root == nil { - return SweepResult{}, errors.New("managedtemp: namespace is closed") - } if err := ctx.Err(); err != nil { return SweepResult{}, err } if opts.Now.IsZero() || opts.Interval <= 0 || opts.CommandReapAfter <= 0 { return SweepResult{}, errors.New("managedtemp: invalid sweep options") } - lock, err := n.root.OpenFile("gc.lock", os.O_RDWR, 0) + root, err := n.openRoot() + if err != nil { + return SweepResult{}, err + } + defer func() { _ = root.Close() }() + lock, err := root.OpenFile("gc.lock", os.O_RDWR, 0) if err != nil { return SweepResult{}, err } @@ -54,14 +56,14 @@ func (n *Namespace) Sweep(ctx context.Context, opts SweepOptions) (SweepResult, } defer func() { _ = unlockClose(lock) }() - completed, err := n.ReadSweepCompletion() + completed, err := readSweepCompletion(root) if err == nil && opts.Now.Sub(completed) < opts.Interval { return SweepResult{}, nil } if err != nil && !errors.Is(err, fs.ErrNotExist) { return SweepResult{}, err } - deleted, err := n.sweepWorkspaces(ctx, opts) + deleted, err := sweepWorkspaces(ctx, root, opts) if err != nil { return SweepResult{Scanned: true, Deleted: deleted}, err } @@ -73,10 +75,10 @@ func (n *Namespace) Sweep(ctx context.Context, opts SweepOptions) (SweepResult, return SweepResult{Scanned: true, Deleted: deleted}, err } var completionErr error - if _, statErr := n.root.Lstat("last-successful-sweep.json"); errors.Is(statErr, fs.ErrNotExist) { - completionErr = writePrivateFile(n.root, "last-successful-sweep.json", data) + if _, statErr := root.Lstat("last-successful-sweep.json"); errors.Is(statErr, fs.ErrNotExist) { + completionErr = writePrivateFile(root, "last-successful-sweep.json", data) } else if statErr == nil { - completionErr = replacePrivateFile(n.root, "last-successful-sweep.json", data) + completionErr = replacePrivateFile(root, "last-successful-sweep.json", data) } else { completionErr = statErr } @@ -86,11 +88,11 @@ func (n *Namespace) Sweep(ctx context.Context, opts SweepOptions) (SweepResult, return SweepResult{Scanned: true, Deleted: deleted}, nil } -func (n *Namespace) sweepWorkspaces(ctx context.Context, opts SweepOptions) (int, error) { - if err := validatePrivateDir(n.root, "workspaces"); err != nil { +func sweepWorkspaces(ctx context.Context, root *os.Root, opts SweepOptions) (int, error) { + if err := validatePrivateDir(root, "workspaces"); err != nil { return 0, err } - workspaces, err := n.root.OpenRoot("workspaces") + workspaces, err := root.OpenRoot("workspaces") if err != nil { return 0, err } diff --git a/internal/adapter/managedtemp/recovery_unix_test.go b/internal/adapter/managedtemp/recovery_unix_test.go new file mode 100644 index 0000000000..8e5e0375de --- /dev/null +++ b/internal/adapter/managedtemp/recovery_unix_test.go @@ -0,0 +1,396 @@ +//go:build linux || darwin + +package managedtemp + +import ( + "context" + "errors" + "io/fs" + "os" + "path/filepath" + "sync" + "sync/atomic" + "syscall" + "testing" + "time" +) + +func TestNamespaceRecoversAfterRemoval(t *testing.T) { + for _, replacement := range []string{"missing", "recreated", "renamed", "workspaces missing"} { + t.Run(replacement, func(t *testing.T) { + ns, workspace := recoveryNamespace(t) + oldRoot := ns.root + pinned, err := ns.openRoot() + if err != nil { + t.Fatal(err) + } + defer func() { _ = pinned.Close() }() + oldInfo, err := oldRoot.Stat(".") + if err != nil { + t.Fatal(err) + } + switch replacement { + case "renamed": + err = os.Rename(ns.Path(), ns.Path()+"-retired") + case "workspaces missing": + err = os.RemoveAll(filepath.Join(ns.Path(), "workspaces")) + default: + err = os.RemoveAll(ns.Path()) + } + if err != nil { + t.Fatal(err) + } + if replacement == "recreated" { + if err := os.Mkdir(ns.Path(), 0o700); err != nil { + t.Fatal(err) + } + } + fresh, err := ns.OpenWorkspace("osfs", "identity", filepath.Dir(ns.Path())) + if err != nil { + t.Fatalf("recover: %v", err) + } + defer func() { _ = fresh.Close() }() + visible, err := os.Stat(ns.Path()) + if err != nil { + t.Fatal(err) + } + current, err := ns.root.Stat(".") + if err != nil || !os.SameFile(current, visible) { + t.Fatalf("recovered handle does not match visible namespace: %v", err) + } + if replacement != "workspaces missing" { + if os.SameFile(oldInfo, current) { + t.Fatal("namespace inode did not change") + } + if _, err := oldRoot.Stat("."); !errors.Is(err, fs.ErrClosed) { + t.Fatalf("stale namespace handle not retired: %v", err) + } + } + // A sweep already in progress owns a pin, not the replaceable cached root. + if info, err := pinned.Stat("."); err != nil || !os.SameFile(info, oldInfo) { + t.Fatalf("recovery invalidated an in-flight operation: %v", err) + } + // Recovery must not close a handle an earlier runner/lease still owns. + if _, err := workspace.root.Stat("."); err != nil { + t.Fatalf("recovery closed an independently owned workspace: %v", err) + } + if lease, err := workspace.Allocate("cmd"); err == nil { + _ = lease.Close() + t.Fatal("stale workspace allocated against a replaced visible path") + } + lease, err := fresh.Allocate("cmd") + if err != nil { + t.Fatalf("allocate after recovery: %v", err) + } + if err := lease.Close(); err != nil { + t.Fatal(err) + } + }) + } +} + +func TestNamespaceRecoveryRejectsUnsafeReplacement(t *testing.T) { + for _, replacement := range []string{"symlink", "file", "mode", "owner", "workspaces link", "workspaces mode", "lock link", "missing parent"} { + t.Run(replacement, func(t *testing.T) { + ns, _ := recoveryNamespace(t) + if err := os.RemoveAll(ns.Path()); err != nil { + t.Fatal(err) + } + outside := t.TempDir() + var err error + switch replacement { + case "symlink": + err = os.Symlink(outside, ns.Path()) + case "file": + err = os.WriteFile(ns.Path(), []byte("do not change"), 0o600) + case "missing parent": + err = os.Remove(filepath.Dir(ns.Path())) + default: + err = os.Mkdir(ns.Path(), 0o700) + if err != nil { + t.Fatal(err) + } + switch replacement { + case "mode": + err = os.Chmod(ns.Path(), 0o755) + case "owner": + if os.Geteuid() != 0 { + t.Skip("changing directory owner requires root") + } + err = os.Chown(ns.Path(), 1, -1) + case "workspaces link": + err = os.Symlink(outside, filepath.Join(ns.Path(), "workspaces")) + case "workspaces mode": + err = os.Mkdir(filepath.Join(ns.Path(), "workspaces"), 0o755) + case "lock link": + err = os.Symlink(outside, filepath.Join(ns.Path(), "gc.lock")) + } + } + if err != nil { + t.Fatal(err) + } + before, _ := os.Lstat(ns.Path()) + if w, err := ns.OpenWorkspace("osfs", "identity", outside); err == nil { + _ = w.Close() + t.Fatal("unsafe namespace accepted") + } + after, _ := os.Lstat(ns.Path()) + if before != nil && (after == nil || !os.SameFile(before, after) || before.Mode() != after.Mode()) { + t.Fatal("unsafe replacement was changed") + } + if before != nil && before.IsDir() { + entries, err := os.ReadDir(ns.Path()) + if err != nil { + t.Fatal(err) + } + want := 0 + if replacement == "workspaces link" || replacement == "workspaces mode" || replacement == "lock link" { + want = 1 + } + if len(entries) != want { + t.Fatalf("recovery mutated unsafe replacement: %v", entries) + } + } + entries, err := os.ReadDir(outside) + if err != nil || len(entries) != 0 { + t.Fatalf("recovery escaped namespace: %v, %v", entries, err) + } + }) + } +} + +func TestNamespaceConcurrentRecovery(t *testing.T) { + ns, stale := recoveryNamespace(t) + old := ns.root + // Pause a real sweep after it pins the old root and acquires its GC lock. + ctx := &sweepBarrierContext{Context: context.Background(), entered: make(chan struct{}), release: make(chan struct{})} + defer ctx.unblock() + swept := make(chan error, 1) + go func() { + _, err := ns.Sweep(ctx, SweepOptions{Now: time.Now(), Interval: time.Hour, CommandReapAfter: time.Hour}) + swept <- err + }() + select { + case <-ctx.entered: + case <-time.After(10 * time.Second): + t.Fatal("sweep did not reach pinned-root barrier") + } + if err := os.Rename(ns.Path(), ns.Path()+"-retired"); err != nil { + t.Fatal(err) + } + const workers = 12 + start := make(chan struct{}) + allocated := make(chan *Lease, workers) + release := make(chan struct{}) + var wg sync.WaitGroup + for range workers { + wg.Go(func() { + <-start + lease, err := ns.AllocateWorkspace("osfs", "identity", filepath.Dir(ns.Path()), "cmd") + if err != nil { + t.Errorf("concurrent recovery/allocation: %v", err) + } + allocated <- lease + <-release + _ = lease.Close() + }) + } + close(start) + var leases []*Lease + for range workers { + select { + case lease := <-allocated: + if lease != nil { + leases = append(leases, lease) + } + case <-time.After(10 * time.Second): + close(release) + t.Fatal("allocation blocked behind in-flight sweep/recovery") + } + } + // All allocations remain locked while a second sweep visits the recovered + // namespace. The first sweep is still pinned to the retired namespace. + result, err := ns.Sweep(context.Background(), SweepOptions{Now: time.Now().Add(2 * time.Hour), Interval: time.Hour, CommandReapAfter: time.Nanosecond}) + if err != nil || !result.Scanned || result.Deleted != 0 { + t.Errorf("sweep of overlapping live allocations: %+v, %v", result, err) + } + for _, lease := range leases { + if _, err := os.Stat(lease.TempDir()); err != nil { + t.Errorf("sweep removed live allocation: %v", err) + } + } + close(release) + wg.Wait() + ctx.unblock() + if err := <-swept; err != nil { + t.Fatalf("recovery invalidated pinned sweep: %v", err) + } + if lease, err := stale.Allocate("cmd"); err == nil { + _ = lease.Close() + t.Fatal("stale allocation succeeded") + } + if _, err := old.Stat("."); !errors.Is(err, fs.ErrClosed) { + t.Fatalf("old root was not retired: %v", err) + } + if err := ns.Close(); err != nil { + t.Fatal(err) + } + if w, err := ns.OpenWorkspace("osfs", "identity", filepath.Dir(ns.Path())); err == nil { + _ = w.Close() + t.Fatal("recovery resurrected a closed namespace") + } +} + +type sweepBarrierContext struct { + context.Context + calls atomic.Int32 + entered, release chan struct{} + once sync.Once +} + +func (c *sweepBarrierContext) Err() error { + if c.calls.Add(1) == 2 { + close(c.entered) + <-c.release + } + return c.Context.Err() +} + +func (c *sweepBarrierContext) unblock() { c.once.Do(func() { close(c.release) }) } + +func TestNamespaceRejectsSymlinkToRetainedRoot(t *testing.T) { + ns, stale := recoveryNamespace(t) + retired := ns.Path() + "-retired" + if err := os.Rename(ns.Path(), retired); err != nil { + t.Fatal(err) + } + if err := os.Symlink(retired, ns.Path()); err != nil { + t.Fatal(err) + } + if lease, err := stale.Allocate("cmd"); err == nil { + _ = lease.Close() + t.Fatal("old runner followed a replacement namespace symlink") + } + if w, err := ns.OpenWorkspace("osfs", "identity", filepath.Dir(ns.Path())); err == nil { + _ = w.Close() + t.Fatal("recovery followed a replacement namespace symlink") + } + if _, err := os.Stat(filepath.Join(stale.Path(), "commands")); !errors.Is(err, fs.ErrNotExist) { + t.Fatalf("unsafe allocation mutated retained root: %v", err) + } +} + +func TestNamespaceReadSweepCompletionRecovers(t *testing.T) { + ns, _ := recoveryNamespace(t) + opts := SweepOptions{Now: time.Now(), Interval: time.Hour, CommandReapAfter: time.Hour} + if _, err := ns.Sweep(context.Background(), opts); err != nil { + t.Fatal(err) + } + if err := os.Rename(ns.Path(), ns.Path()+"-retired"); err != nil { + t.Fatal(err) + } + if _, err := ns.ReadSweepCompletion(); !errors.Is(err, fs.ErrNotExist) { + t.Fatalf("read returned retired namespace's completion instead of recovering: %v", err) + } + if _, err := os.Stat(ns.Path()); err != nil { + t.Fatalf("read did not recover configured root: %v", err) + } +} + +func TestWorkspaceManifestChecksVisibleIdentityBeforeMutation(t *testing.T) { + for _, existing := range []bool{false, true} { + t.Run(map[bool]string{false: "create", true: "replace"}[existing], func(t *testing.T) { + _, w := recoveryNamespace(t) + before, err := w.root.ReadFile("workspace.manifest") + if err != nil { + t.Fatal(err) + } + if !existing { + if err := w.root.Remove("workspace.manifest"); err != nil { + t.Fatal(err) + } + } + if err := os.Rename(w.Path(), w.Path()+"-retired"); err != nil { + t.Fatal(err) + } + if err := os.Mkdir(w.Path(), 0o700); err != nil { + t.Fatal(err) + } + manifest, err := workspaceManifestJSON(filepath.Base(w.Path()), "/changed") + if err != nil { + t.Fatal(err) + } + if err := w.writeWorkspaceManifest(manifest, "/changed"); err == nil { + t.Fatal("manifest mutated through a retained handle after replacement") + } + after, err := w.root.ReadFile("workspace.manifest") + if existing && (err != nil || string(after) != string(before)) { + t.Fatalf("retired manifest changed: %q, %v", after, err) + } + if !existing && !errors.Is(err, fs.ErrNotExist) { + t.Fatalf("manifest created in retired workspace: %q, %v", after, err) + } + entries, err := os.ReadDir(w.Path()) + if err != nil || len(entries) != 0 { + t.Fatalf("replacement workspace changed: %v, %v", entries, err) + } + }) + } +} + +type ownerFileInfo struct { + fs.FileInfo + stat syscall.Stat_t +} + +func (i ownerFileInfo) Sys() any { return &i.stat } + +func TestRecoveryRejectsWrongOwnerWithoutPrivileges(t *testing.T) { + ns, _ := recoveryNamespace(t) + info, err := os.Lstat(ns.Path()) + if err != nil { + t.Fatal(err) + } + if err := validatePrivateDirInfo(info); err != nil { + t.Fatalf("valid owner fixture: %v", err) + } + stat := *info.Sys().(*syscall.Stat_t) + stat.Uid ^= 1 + if err := validatePrivateDirInfo(ownerFileInfo{FileInfo: info, stat: stat}); err == nil { + t.Fatal("recovery's private-directory validator accepted a different UID") + } +} + +func TestAllocateWorkspaceOwnsAndClosesHandles(t *testing.T) { + ns, _ := recoveryNamespace(t) + for range 32 { + lease, err := ns.AllocateWorkspace("osfs", "identity", filepath.Dir(ns.Path()), "cmd") + if err != nil { + t.Fatal(err) + } + parent, root := lease.parent.root, lease.root + if err := lease.Remove(); err != nil { + t.Fatal(err) + } + for _, handle := range []*os.Root{parent, root} { + if _, err := handle.Stat("."); !errors.Is(err, fs.ErrClosed) { + t.Fatalf("per-allocation handle leaked: %v", err) + } + } + } +} + +func recoveryNamespace(t *testing.T) (*Namespace, *Workspace) { + t.Helper() + ns, err := Open(filepath.Join(t.TempDir(), "managed")) + if err != nil { + t.Fatal(err) + } + t.Cleanup(func() { _ = ns.Close() }) + w, err := ns.OpenWorkspace("osfs", "identity", filepath.Dir(ns.Path())) + if err != nil { + t.Fatal(err) + } + t.Cleanup(func() { _ = w.Close() }) + return ns, w +} diff --git a/internal/adapter/osfs/osfs.go b/internal/adapter/osfs/osfs.go index 6ec1403e2b..f7d944e302 100644 --- a/internal/adapter/osfs/osfs.go +++ b/internal/adapter/osfs/osfs.go @@ -1297,10 +1297,9 @@ type CommandRunner struct { // inherited git danger is REMOVED, not merely overridden. osfs stays free of // git-specific knowledge — it just sets whatever complete env it is handed. env []string - // managedWorkspace owns foreground command and background-job leases. It is - // nil for system temporary storage and for runners that cannot make the managed - // guarantee. - managedWorkspace *managedtemp.Workspace + // allocateManagedLease supplies foreground command and background-job leases. + // It is nil only when managed temporary storage is not configured. + allocateManagedLease func(string) (*managedtemp.Lease, error) // systemTempDir is the configured/inherited host temporary directory applied // only when the trusted caller selects the system scope. systemTempDir string @@ -1338,7 +1337,16 @@ func WithCommandEnvList(env []string) CommandRunnerOption { // after the runner's already-scrubbed base environment; callers cannot supply // lease paths through shell text or tool arguments. func WithManagedTemporaryWorkspace(workspace *managedtemp.Workspace) CommandRunnerOption { - return func(r *CommandRunner) { r.managedWorkspace = workspace } + if workspace == nil { + return WithManagedTemporaryAllocator(nil) + } + return WithManagedTemporaryAllocator(workspace.Allocate) +} + +// WithManagedTemporaryAllocator resolves a fresh managed lease for each command +// or background job. The allocator owns recovery and the lease owns its handles. +func WithManagedTemporaryAllocator(allocate func(string) (*managedtemp.Lease, error)) CommandRunnerOption { + return func(r *CommandRunner) { r.allocateManagedLease = allocate } } // WithSystemTemporaryDirectory sets the configured/inherited system temporary @@ -1419,7 +1427,7 @@ func (r *CommandRunner) Run(ctx context.Context, command string) (tool.CommandRe func (r *CommandRunner) RunWithTemporaryScope(ctx context.Context, command string, scope tool.TemporaryScope) (tool.CommandResult, error) { overlay := tool.CommandEnvironmentOverlay{} managed := scope == tool.TemporaryScopeManaged - if scope == tool.TemporaryScopeSystem || r.managedWorkspace == nil { + if scope == tool.TemporaryScopeSystem || r.allocateManagedLease == nil { managed = false if r.systemTempDir != "" { overlay.TempDir = r.systemTempDir @@ -1466,7 +1474,7 @@ func (r *CommandRunner) RunStreaming(ctx context.Context, command string, out io func (r *CommandRunner) RunStreamingWithTemporaryScope(ctx context.Context, command string, scope tool.TemporaryScope, out io.Writer) (int, error) { overlay := tool.CommandEnvironmentOverlay{} managed := scope == tool.TemporaryScopeManaged - if scope == tool.TemporaryScopeSystem || r.managedWorkspace == nil { + if scope == tool.TemporaryScopeSystem || r.allocateManagedLease == nil { managed = false if r.systemTempDir != "" { overlay.TempDir = r.systemTempDir @@ -1505,6 +1513,7 @@ func (r *CommandRunner) run(ctx context.Context, command string, overlay tool.Co if leaseErr != nil { return 0, leaseErr } + defer func() { _ = lease.Close() }() cmd := exec.CommandContext(ctx, r.shell, "-c", command) cmd.Dir = r.root @@ -1577,10 +1586,10 @@ func (r *CommandRunner) run(ctx context.Context, command string, overlay tool.Co } func (r *CommandRunner) managedLease(managed bool, kind string, overlay tool.CommandEnvironmentOverlay) (*managedtemp.Lease, tool.CommandEnvironmentOverlay, error) { - if !managed || r.managedWorkspace == nil { + if !managed || r.allocateManagedLease == nil { return nil, overlay, nil } - lease, err := r.managedWorkspace.Allocate(kind) + lease, err := r.allocateManagedLease(kind) if err != nil { return nil, overlay, fmt.Errorf("osfs: allocate managed command lease: %w", err) } diff --git a/internal/adapter/server/placement.go b/internal/adapter/server/placement.go index cc8ccb81ba..9c4ba9474f 100644 --- a/internal/adapter/server/placement.go +++ b/internal/adapter/server/placement.go @@ -22,6 +22,8 @@ var ( ErrPlacementStale = errors.New("server: placement selector is stale") // ErrPlacementUnavailable reports a known, authorized placement whose backend is unavailable. ErrPlacementUnavailable = errors.New("server: placement unavailable") + // ErrManagedTemporaryStorageUnavailable is a path-free, actionable placement cause. + ErrManagedTemporaryStorageUnavailable = fmt.Errorf("%w: managed temporary storage unavailable; retry after the operator restores a real, owner-only managed root and its parent directory; do not use a symlink or relax permissions", ErrPlacementUnavailable) // ErrPlacementChanged reports an inventory revision race during Bind. ErrPlacementChanged = errors.New("server: placement changed during bind") // ErrInvalidPlacementBinding reports unsafe or internally inconsistent provider output. @@ -183,7 +185,7 @@ func sanitizePlacementProviderError(err error) error { public := ErrPlacementUnavailable for _, candidate := range []error{ ErrInvalidPlacementSelection, ErrPlacementNotFound, ErrPlacementStale, - ErrPlacementUnavailable, ErrPlacementChanged, ErrInvalidPlacementBinding, + ErrManagedTemporaryStorageUnavailable, ErrPlacementUnavailable, ErrPlacementChanged, ErrInvalidPlacementBinding, } { if errors.Is(err, candidate) { public = candidate diff --git a/internal/adapter/server/placement_internal_test.go b/internal/adapter/server/placement_internal_test.go index f84aafd60d..81f3902e68 100644 --- a/internal/adapter/server/placement_internal_test.go +++ b/internal/adapter/server/placement_internal_test.go @@ -11,6 +11,18 @@ import ( "github.com/stacklok/mecatl/engine/tool" ) +func TestManagedTemporaryPlacementErrorRemainsActionableAndRedacted(t *testing.T) { + private := errors.New("private root /srv/secret with token=do-not-disclose") + err := sanitizePlacementProviderError(errors.Join(ErrManagedTemporaryStorageUnavailable, private)) + if err.Error() != ErrManagedTemporaryStorageUnavailable.Error() || !errors.Is(err, ErrPlacementUnavailable) { + t.Fatalf("managed storage cause lost or private detail exposed: %v", err) + } + generic := sanitizePlacementProviderError(private) + if generic.Error() != ErrPlacementUnavailable.Error() { + t.Fatalf("unclassified provider error escaped redaction: %v", generic) + } +} + type exactPlacementProvider struct { binding PlacementBinding err error diff --git a/internal/app/build.go b/internal/app/build.go index dba1dc85be..9990d3e981 100644 --- a/internal/app/build.go +++ b/internal/app/build.go @@ -2100,8 +2100,8 @@ func Build(ctx context.Context, cfg Config) (*Built, error) { } placementProvider = &localPlacementProvider{ scope: placementScope, root: cfg.Workspace, workspace: workspaceFactory, - runnerForRoot: func(root string) tool.CommandRunner { - return buildCommandRunnerForRoot(cfg, root) + runnerForRoot: func(root string) (tool.CommandRunner, error) { + return commandRunnerForRoot(cfg, root) }, worktrees: worktreeLister, selectors: selectorIssuer, } @@ -5230,7 +5230,9 @@ func registerCoreTools(cfg Config, cat *tool.Catalog, log, noFS bool, searchProv cat.MustRegister(t) } cat.MustRegister(tools.NewWebSearchTool(searchProvider)) - if runner := buildCommandRunner(cfg); runner != nil { + if !cfg.NoBash && cfg.Shell != "" { + // Catalog availability follows configuration; placement constructs the runner + // and fails explicitly if it cannot supply the configured shell. // The AGENT-loop Bash tool (not the fstools one): foreground byte-identical, // plus the `background: true` detach over the run's child registry. Its // companion BashStatus — the SOLE status/collect/cancel channel for those @@ -6137,16 +6139,23 @@ func buildCommandRunner(cfg Config) tool.CommandRunner { return buildCommandRunnerForRoot(cfg, cfg.Workspace) } -// buildCommandRunnerForRoot is the ONE implementation of the no-bash/shell -// gate + envscrub.Scrub(os.Environ()) + osfs constructor, parameterised by the -// root the runner is bound to. The main runner and the placement provider's -// private environment construction both route through here, so secret scrubbing -// cannot drift between default-root and alternate-root paths (security review -// "Finding B"). It returns nil when command execution is disabled -// (NoBash or an empty Shell); on a construction error it WARNs and returns nil. +// buildCommandRunnerForRoot delegates to the same gated, secret-scrubbed +// constructor as placement. Disabled execution returns nil; failed construction +// retains an error marker so catalog assembly cannot silently remove Bash. +// The actual placement/fork bind propagates the construction error. func buildCommandRunnerForRoot(cfg Config, root string) tool.CommandRunner { + runner, err := commandRunnerForRoot(cfg, root) + if err != nil { + return failedCommandRunner{err: err} + } + return runner +} + +// commandRunnerForRoot keeps disabled shells distinct from failed construction. +// Placement must propagate the latter rather than silently attenuating Bash. +func commandRunnerForRoot(cfg Config, root string) (tool.CommandRunner, error) { if cfg.NoBash || cfg.Shell == "" { - return nil + return nil, nil } // SECRET SCRUB (security review "Finding B"): the main-session Bash child must // NOT see the harness's provider/auth credentials, or under posture auto/yolo a @@ -6158,28 +6167,26 @@ func buildCommandRunnerForRoot(cfg Config, root string) tool.CommandRunner { // (gitenv) — the operator's own hooks/pager are honoured here, only the secrets // are removed. env := envscrub.Scrub(os.Environ()) - return newCommandRunnerForRoot(cfg, root, env, "could not build command runner; Bash tool disabled") + return newCommandRunnerForRoot(cfg, root, env) } -func newCommandRunnerForRoot(cfg Config, root string, env []string, failure string) tool.CommandRunner { +func newCommandRunnerForRoot(cfg Config, root string, env []string) (tool.CommandRunner, error) { opts := []osfs.CommandRunnerOption{ osfs.WithCommandEnvList(env), osfs.WithSystemTemporaryDirectory(cfg.temporaryStorage.SystemTempDir), } if cfg.managedTemp != nil { - workspace, err := cfg.managedTemp.workspace(root) + allocate, err := cfg.managedTemp.allocator(root) if err != nil { - cfg.diag().Log(context.Background(), port.LevelWarn, failure, "workspace", root, "err", err) - return nil + return nil, server.ErrManagedTemporaryStorageUnavailable } - opts = append(opts, osfs.WithManagedTemporaryWorkspace(workspace)) + opts = append(opts, osfs.WithManagedTemporaryAllocator(allocate)) } runner, err := osfs.NewCommandRunnerShell(root, cfg.Shell, opts...) if err != nil { - cfg.diag().Log(context.Background(), port.LevelWarn, failure, "workspace", root, "err", err) - return nil + return nil, fmt.Errorf("configured command runner unavailable; ask the operator to restore the workspace and shell, then retry") } - return runner + return runner, nil } // buildSandboxedCommandRunner builds the command runner team MEMBERS' Bash executes @@ -6301,7 +6308,11 @@ func buildForceCopyRunner(cfg Config) tool.CommandRunner { // hardening rationale). It assumes the caller already applied the NoBash/empty-shell // (and, where applicable, trust) gates. The runner is bound to cfg.Workspace. func newHardenedCommandRunner(cfg Config) tool.CommandRunner { - return newHardenedRunnerForRoot(cfg, cfg.Workspace) + runner, err := newHardenedRunnerForRoot(cfg, cfg.Workspace) + if err != nil { + return failedCommandRunner{err: err} + } + return runner } // newHardenedRunnerForRoot constructs an env-scrubbed runner bound to root (an @@ -6309,21 +6320,15 @@ func newHardenedCommandRunner(cfg Config) tool.CommandRunner { // hardening as newHardenedCommandRunner. It is the forker's bound-runner // builder (issue #462): a forked child's Bash observes the SAME child namespace // its Read/Write do, never the parent base. It assumes the caller already -// applied the NoBash/empty-shell (and, where applicable, trust) gates; on a -// construction error it returns nil (the child degrades to shell-less, matching -// newHardenedCommandRunner's WARN-then-nil shape, but a per-child builder has no -// session-correlated diagnostics handle, so it returns nil silently — the -// member/subagent catalog already gated Bash registration on the parent runner -// being non-nil, so a nil here only ever reaches a child whose catalog has Bash -// but whose isolated namespace could not open a shell, a rare FS-permission -// case the child's Bash surfaces as ErrNoShell). -func newHardenedRunnerForRoot(cfg Config, root string) tool.CommandRunner { +// applied the NoBash/empty-shell (and, where applicable, trust) gates. Construction +// failures propagate through the forker; they never attenuate a configured shell. +func newHardenedRunnerForRoot(cfg Config, root string) (tool.CommandRunner, error) { // SECRET SCRUB then GIT NEUTRALISE: drop the harness credentials first // (envscrub — "Finding B"; gitenv only ever removed GIT_*/PAGER, never secrets), // then layer the git-neutralising env on the secret-free base so a sandboxed // child sees neither the operator's secrets nor an untrusted repo's git hooks. env := gitenv.Scrub(envscrub.Scrub(os.Environ())) - return newCommandRunnerForRoot(cfg, root, env, "could not build sandboxed member command runner; team-member Bash disabled") + return newCommandRunnerForRoot(cfg, root, env) } // subagentShellUntrustedReason returns the model/operator-facing reason the @@ -6780,7 +6785,7 @@ func buildWritableSubagentChildEngine(cfg Config, provReg *providerRegistry, pro // with childWindowFor for the factory — the buildParallelEngineFactory discipline). func writableExplorerDeps(cfg Config, provider port.LLMProvider, model, role string, windowFn func() int, runner tool.CommandRunner) agent.Deps { childCat := writableExplorerCatalog(runner, "writable subagent tool catalog") - return childEngineDepsForProvider(cfg, role, provider, model, windowFn, + return childEngineDepsForProvider(cfg, role, guardWritableRunner(cfg, provider), model, windowFn, childCat, promptConfig(modelCfgFor(cfg, model), cfg.gitStatus), nil) } @@ -7134,9 +7139,9 @@ func buildSubagentTool(ctx context.Context, cfg Config, provReg *providerRegistr // gate passed at build time; the per-child builder re-checks it so a future // per-session trust change cannot hand a shell to an untrusted child). taskForker := forker.New(newForkWorkspace(), forker.WithDirtyOverlay(), - forker.WithRunner(func(childRoot string) tool.CommandRunner { + forker.WithRunnerError(func(childRoot string) (tool.CommandRunner, error) { if !sandboxedShellAvailable(cfg) { - return nil + return nil, nil } return newHardenedRunnerForRoot(cfg, childRoot) })) @@ -7428,7 +7433,7 @@ func buildAgentWritableEngineFactories(ctx context.Context, cfg Config, provReg role += ":model=" + model } - eng, mcpClose, names, _, skillCount := buildAgentDefEngine(ctx, cfg, def, role, reg.Detail(def.Name), childProvider, model, windowFn, + eng, mcpClose, names, _, skillCount := buildAgentDefEngine(ctx, cfg, def, role, reg.Detail(def.Name), guardWritableRunner(cfg, childProvider), model, windowFn, baseSubagentTools(cfg), true /*allowMutating*/, mainRunner != nil /*allowShell*/, skillIdx, defaultHooks, mainRunner, mainMgr) if mcpClose != nil { if err := mcpClose(); err != nil { @@ -7546,20 +7551,20 @@ func buildTeamWiring(_ context.Context, cfg Config, provReg *providerRegistry, p // when the gate withholds the shell (Bash disabled / untrusted workspace for // roFk), so the child Environment is shell-less and its Bash surfaces ErrNoShell // honestly — matching the historical shell-less degrade. - roRunnerBuilder := func(childRoot string) tool.CommandRunner { + roRunnerBuilder := func(childRoot string) (tool.CommandRunner, error) { if !sandboxedShellAvailable(cfg) { - return nil + return nil, nil } return newHardenedRunnerForRoot(cfg, childRoot) } - fkRunnerBuilder := func(childRoot string) tool.CommandRunner { + fkRunnerBuilder := func(childRoot string) (tool.CommandRunner, error) { if !forceCopyShellAvailable(cfg) { - return nil + return nil, nil } return newHardenedRunnerForRoot(cfg, childRoot) } - fk := forker.New(newForkWorkspace(), forker.WithForceCopy(), forker.WithRunner(fkRunnerBuilder)) - roFk := forker.New(newForkWorkspace(), forker.WithDirtyOverlay(), forker.WithRunner(roRunnerBuilder)) + fk := forker.New(newForkWorkspace(), forker.WithForceCopy(), forker.WithRunnerError(fkRunnerBuilder)) + roFk := forker.New(newForkWorkspace(), forker.WithDirtyOverlay(), forker.WithRunnerError(roRunnerBuilder)) memberRunner := buildSandboxedCommandRunner(cfg) mutatingRunner := buildForceCopyRunner(cfg) roIsolationAvailable := memberRunner != nil && roFk != nil diff --git a/internal/app/catalog.go b/internal/app/catalog.go index e0601f12b0..7de910a621 100644 --- a/internal/app/catalog.go +++ b/internal/app/catalog.go @@ -408,9 +408,9 @@ func registerParallelTool(ctx context.Context, cfg Config, cat *tool.Catalog, re // buildForceCopyRunner does (force-copy forks do no fork-time git, so the // trust gate does not apply — see the comment above). fk := forker.New(newForkWorkspace(), forker.WithForceCopy(), - forker.WithRunner(func(childRoot string) tool.CommandRunner { + forker.WithRunnerError(func(childRoot string) (tool.CommandRunner, error) { if !forceCopyShellAvailable(cfg) { - return nil + return nil, nil } return newHardenedRunnerForRoot(cfg, childRoot) })) diff --git a/internal/app/command_runner_failure.go b/internal/app/command_runner_failure.go new file mode 100644 index 0000000000..9a2e279bcc --- /dev/null +++ b/internal/app/command_runner_failure.go @@ -0,0 +1,41 @@ +package app + +import ( + "context" + "iter" + + "github.com/stacklok/mecatl/engine/port" + "github.com/stacklok/mecatl/engine/tool" +) + +// A configured runner is never represented as an intentional shell-less mount. +// Fork builders propagate errors directly; catalog construction retains this +// marker until the real environment is bound. +type failedCommandRunner struct{ err error } + +func (r failedCommandRunner) Run(context.Context, string) (tool.CommandResult, error) { + return tool.CommandResult{}, r.err +} + +// Writable factories have a capability-only bool result in the engine API. +// Validate at execution instead: unsupported remains false, while unavailable +// becomes a normal failed child with an actionable cause, before any model call. +// Rechecking also permits recovery without rebuilding a cached child engine. +type writableRunnerProvider struct { + port.LLMProvider + cfg Config +} + +func guardWritableRunner(cfg Config, provider port.LLMProvider) port.LLMProvider { + if cfg.NoBash || cfg.Shell == "" { + return provider + } + return writableRunnerProvider{LLMProvider: provider, cfg: cfg} +} + +func (p writableRunnerProvider) Stream(ctx context.Context, req port.LLMRequest) (iter.Seq2[port.Chunk, error], error) { + if _, err := commandRunnerForRoot(p.cfg, p.cfg.Workspace); err != nil { + return nil, err + } + return p.LLMProvider.Stream(ctx, req) +} diff --git a/internal/app/managed_temp_recovery_test.go b/internal/app/managed_temp_recovery_test.go new file mode 100644 index 0000000000..a550a00293 --- /dev/null +++ b/internal/app/managed_temp_recovery_test.go @@ -0,0 +1,363 @@ +//go:build linux || darwin + +package app + +import ( + "context" + "errors" + "os" + "path/filepath" + "slices" + "strings" + "sync/atomic" + "testing" + + "github.com/stacklok/mecatl/engine/adapter/mockllm" + "github.com/stacklok/mecatl/engine/port" + "github.com/stacklok/mecatl/engine/session" + "github.com/stacklok/mecatl/engine/tool" + "github.com/stacklok/mecatl/internal/adapter/agents" + "github.com/stacklok/mecatl/internal/adapter/hookexec" + "github.com/stacklok/mecatl/internal/adapter/server" +) + +func TestManagedTempRecoveryContinuedSessionAndWritableSubagent(t *testing.T) { + for _, recreate := range []bool{false, true} { + t.Run(map[bool]string{false: "deleted", true: "recreated"}[recreate], func(t *testing.T) { + var requests atomic.Int32 + provider := mockllm.NewWith([]mockllm.Option{ + mockllm.WithRequestObserver(func(req port.LLMRequest) { + requests.Add(1) + if !slices.ContainsFunc(req.Tools, func(spec tool.ToolSpec) bool { return spec.Name == "Bash" }) { + t.Error("main or direct-write child request lost Bash") + } + }), + }, + mockllm.ToolCallTurn(session.NewToolCall("before", "Bash", []byte(`{"command":"printf before-recovery"}`))), + mockllm.TextTurn("first run complete"), + mockllm.ToolCallTurn(session.NewToolCall("after", "Bash", []byte(`{"command":"printf after-recovery"}`))), + mockllm.ToolCallTurn(session.NewToolCall("delegate", "Subagent", []byte(`{"prompt":"write the proof using Bash","mode":"read-write"}`))), + mockllm.ToolCallTurn(session.NewToolCall("child-bash", "Bash", []byte(`{"command":"printf child-recovered > recovered-child.txt"}`))), + mockllm.TextTurn("child complete"), + mockllm.TextTurn("continued run complete"), + ) + cfg := recoveryBuildConfig(t, provider) + built, err := Build(context.Background(), cfg) + if err != nil { + t.Fatal(err) + } + defer built.Close() + sess, err := built.Service.CreateSession(context.Background(), session.ModeDefault, session.Limits{MaxTurns: 10}) + if err != nil { + t.Fatal(err) + } + assertRecoveryRun(t, built.Service, sess.ID, "before-recovery") + if err := os.RemoveAll(cfg.temporaryStorage.ManagedRoot); err != nil { + t.Fatal(err) + } + if recreate { + if err := os.Mkdir(cfg.temporaryStorage.ManagedRoot, 0o700); err != nil { + t.Fatal(err) + } + } + assertRecoveryRun(t, built.Service, sess.ID, "after-recovery") + data, err := os.ReadFile(filepath.Join(cfg.Workspace, "recovered-child.txt")) + if err != nil || string(data) != "child-recovered" { + t.Fatalf("direct-write child did not execute Bash in parent workspace: %q, %v", data, err) + } + if got := requests.Load(); got != 7 { + t.Fatalf("provider requests = %d, want 7 including child", got) + } + }) + } +} + +func TestManagedTempRecoveryWritableChildConstruction(t *testing.T) { + cfg := managedLeaseConfig(t, t.TempDir(), filepath.Join(t.TempDir(), "managed")) + cfg.Model = "mock" + cfg.AllowAllTools = true + if runner, err := commandRunnerForRoot(cfg, cfg.Workspace); err != nil || runner == nil { + t.Fatalf("initial runner: %v, %v", runner, err) + } + if err := os.RemoveAll(cfg.temporaryStorage.ManagedRoot); err != nil { + t.Fatal(err) + } + var removed atomic.Bool + provider := mockllm.NewWith([]mockllm.Option{mockllm.WithRequestObserver(func(port.LLMRequest) { + if removed.CompareAndSwap(false, true) { + // The child engine and parent runner already exist, and the writable + // preflight just succeeded. Allocation must recover independently. + if err := os.Rename(cfg.temporaryStorage.ManagedRoot, cfg.temporaryStorage.ManagedRoot+"-after-construction"); err != nil { + t.Error(err) + } + } + })}, + mockllm.ToolCallTurn(session.NewToolCall("proof", "Bash", []byte(`{"command":"printf constructed > constructed-child.txt"}`))), + mockllm.TextTurn("done"), + ) + child, closeFn := buildSubagentTool(context.Background(), + cfg, regForTest(provider, providerMock, cfg.Model), provider, providerMock, cfg.Model, + hookexec.New(nil), agents.NewRegistry(nil), nil, nil, nil, catalogAssets{}, false) + if closeFn != nil { + defer func() { _ = closeFn() }() + } + runner, err := commandRunnerForRoot(cfg, cfg.Workspace) + if err != nil { + t.Fatal(err) + } + result, err := child.Execute(context.Background(), + session.NewToolCall("delegate", "Subagent", []byte(`{"prompt":"write the proof","mode":"read-write"}`)), + osfsEnvironment(t, cfg.Workspace, runner)) + if err != nil || result.IsError { + t.Fatalf("direct-write child after namespace recovery: %v, %+v", err, result) + } + data, err := os.ReadFile(filepath.Join(cfg.Workspace, "constructed-child.txt")) + if err != nil || string(data) != "constructed" { + t.Fatalf("child constructed after deletion lost Bash: %q, %v", data, err) + } +} + +func TestManagedTempRecoveryFailureRejectsContinuedSession(t *testing.T) { + var requests atomic.Int32 + provider := mockllm.NewWith([]mockllm.Option{ + mockllm.WithRequestObserver(func(port.LLMRequest) { requests.Add(1) }), + }, mockllm.TextTurn("ready"), + mockllm.ToolCallTurn(session.NewToolCall("must-not-start", "Subagent", []byte(`{"prompt":"requires Bash","mode":"read-write"}`))), + ) + cfg := recoveryBuildConfig(t, provider) + built, err := Build(context.Background(), cfg) + if err != nil { + t.Fatal(err) + } + defer built.Close() + sess, err := built.Service.CreateSession(context.Background(), session.ModeDefault, session.Limits{}) + if err != nil { + t.Fatal(err) + } + run, err := built.Service.StartRunContent(context.Background(), sess.ID, "first", nil) + if err != nil { + t.Fatal(err) + } + for range run.Events() { + } + if err := os.RemoveAll(cfg.temporaryStorage.ManagedRoot); err != nil { + t.Fatal(err) + } + outside := t.TempDir() + if err := os.Symlink(outside, cfg.temporaryStorage.ManagedRoot); err != nil { + t.Fatal(err) + } + run, err = built.Service.StartRunContent(context.Background(), sess.ID, "delegate shell-required work", nil) + if err == nil { + run.Cancel() + for range run.Events() { + } + t.Fatal("unsafe managed root became a valid shell-less run") + } + if !errors.Is(err, server.ErrPlacementUnavailable) || !strings.Contains(err.Error(), "managed temporary storage") || !strings.Contains(err.Error(), "retry") || strings.Contains(err.Error(), cfg.temporaryStorage.ManagedRoot) || strings.Contains(err.Error(), outside) { + t.Fatalf("continuation must have a redacted actionable placement error: %v", err) + } + if requests.Load() != 1 { + t.Fatal("main/child model started despite failed placement reattachment") + } + entries, err := os.ReadDir(outside) + if err != nil || len(entries) != 0 { + t.Fatalf("unsafe replacement target changed: %v, %v", entries, err) + } +} + +func TestManagedTempRecoveryFailureDeclinesWritableChild(t *testing.T) { + cfg := managedLeaseConfig(t, t.TempDir(), filepath.Join(t.TempDir(), "managed")) + cfg.TrustProject = true + cfg.AllowAllTools = true + cfg.Model = "mock" + runner, err := commandRunnerForRoot(cfg, cfg.Workspace) + if err != nil { + t.Fatal(err) + } + parent := osfsEnvironment(t, cfg.Workspace, runner) + if err := os.RemoveAll(cfg.temporaryStorage.ManagedRoot); err != nil { + t.Fatal(err) + } + if err := os.WriteFile(cfg.temporaryStorage.ManagedRoot, []byte("unsafe replacement"), 0o600); err != nil { + t.Fatal(err) + } + var requests atomic.Int32 + provider := mockllm.NewWith([]mockllm.Option{ + mockllm.WithRequestObserver(func(port.LLMRequest) { requests.Add(1) }), + }, mockllm.ToolCallTurn(session.NewToolCall("retry-bash", "Bash", []byte(`{"command":"printf retried > retry-proof.txt"}`))), mockllm.TextTurn("recovered")) + reg := regForTest(provider, providerMock, cfg.Model) + child, closeFn := buildSubagentTool(context.Background(), + cfg, reg, provider, providerMock, cfg.Model, + hookexec.New(nil), agents.NewRegistry(nil), nil, nil, nil, catalogAssets{}, false) + if closeFn != nil { + defer func() { _ = closeFn() }() + } + for name, args := range map[string]string{ + "read-only": `{"prompt":"requires Bash"}`, + "writable": `{"prompt":"requires Bash","mode":"read-write"}`, + "writable-model": `{"prompt":"requires Bash","mode":"read-write","model":"override"}`, + } { + t.Run(name, func(t *testing.T) { + result, err := child.Execute(context.Background(), session.NewToolCall(session.ToolCallID(name), "Subagent", []byte(args)), parent) + if err == nil && !result.IsError { + t.Fatal("failed child runner construction became a clean shell-less child") + } + if !strings.Contains(result.Content, "managed temporary storage") || !strings.Contains(result.Content, "retry") || strings.Contains(result.Content, cfg.temporaryStorage.ManagedRoot) { + t.Fatalf("runner failure disguised as unsupported capability or leaked path: %s", result.Content) + } + }) + } + factory := buildWritableSubagentEngineFactory(cfg, reg, provider, providerMock, cfg.Model) + if engine, ok := factory("override"); !ok || engine == nil { + t.Fatal("runner failure must not masquerade as unsupported model capability") + } + if requests.Load() != 0 { + t.Fatal("child model started despite runner construction failure") + } + if err := os.Rename(cfg.temporaryStorage.ManagedRoot, cfg.temporaryStorage.ManagedRoot+"-unsafe"); err != nil { + t.Fatal(err) + } + result, err := child.Execute(context.Background(), session.NewToolCall("retry", "Subagent", []byte(`{"prompt":"try again","mode":"read-write"}`)), parent) + if err != nil || result.IsError { + t.Fatalf("cached writable engine did not recover on retry: %+v, %v", result, err) + } + data, err := os.ReadFile(filepath.Join(cfg.Workspace, "retry-proof.txt")) + if err != nil || string(data) != "retried" || requests.Load() != 2 { + t.Fatalf("retry did not execute Bash: %q, %v, requests=%d", data, err, requests.Load()) + } +} + +func TestManagedTempRecoveryPreservesDisabledShell(t *testing.T) { + cfg := managedLeaseConfig(t, t.TempDir(), filepath.Join(t.TempDir(), "managed")) + if err := os.RemoveAll(cfg.temporaryStorage.ManagedRoot); err != nil { + t.Fatal(err) + } + if err := os.WriteFile(cfg.temporaryStorage.ManagedRoot, []byte("unsafe replacement"), 0o600); err != nil { + t.Fatal(err) + } + for _, noBash := range []bool{false, true} { + cfg.NoBash = noBash + cfg.Shell = "" + if noBash { + cfg.Shell = "/bin/sh" + } + runner, err := commandRunnerForRoot(cfg, cfg.Workspace) + if err != nil || runner != nil { + t.Fatalf("intentional shell-less construction = %v, %v", runner, err) + } + } + cfg.NoBash = false + cfg.Shell = "/bin/sh" + if runner, err := commandRunnerForRoot(cfg, cfg.Workspace); runner != nil || err == nil || !strings.Contains(err.Error(), "managed temporary storage") { + t.Fatalf("configured shell failure must be explicit: %v, %v", runner, err) + } +} + +func TestManagedTempRecoveryDoesNotGateShelllessSessions(t *testing.T) { + for _, profile := range []server.SessionProfile{server.ProfileDefault, server.ProfileNoFS} { + t.Run(string(profile), func(t *testing.T) { + var requests atomic.Int32 + provider := mockllm.NewWith([]mockllm.Option{mockllm.WithRequestObserver(func(req port.LLMRequest) { + requests.Add(1) + if slices.ContainsFunc(req.Tools, func(spec tool.ToolSpec) bool { return spec.Name == "Bash" }) { + t.Error("intentional shell-less session advertised Bash") + } + })}, mockllm.TextTurn("ready"), mockllm.TextTurn("still ready")) + cfg := recoveryBuildConfig(t, provider) + cfg.NoBash = profile == server.ProfileDefault + built, err := Build(context.Background(), cfg) + if err != nil { + t.Fatal(err) + } + defer built.Close() + // Damage the managed root after Build. Neither Bind nor Reattach for + // these intentional shell-less sessions should consult it. + if err := os.Rename(cfg.temporaryStorage.ManagedRoot, cfg.temporaryStorage.ManagedRoot+"-retired"); err != nil { + t.Fatal(err) + } + if err := os.WriteFile(cfg.temporaryStorage.ManagedRoot, []byte("unsafe"), 0o600); err != nil { + t.Fatal(err) + } + sess, err := built.Service.CreateSessionWithProfile(context.Background(), session.ModeDefault, session.Limits{}, server.ProviderSelector{}, profile) + if err != nil { + t.Fatal(err) + } + for range 2 { + run, err := built.Service.StartRunContent(context.Background(), sess.ID, "continue", nil) + if err != nil { + t.Fatal(err) + } + var completed bool + for ev := range run.Events() { + if ev.Result != nil { + completed = ev.Result.Stop == session.StopEndTurn + } + } + if !completed { + t.Fatal("shell-less session did not complete") + } + } + if requests.Load() != 2 { + t.Fatalf("requests = %d, want 2", requests.Load()) + } + }) + } +} + +func recoveryBuildConfig(t *testing.T, provider port.LLMProvider) Config { + t.Helper() + return Config{ + Workspace: t.TempDir(), Model: "mock", MockProvider: provider, + Shell: "/bin/sh", Headless: true, Posture: PostureAuto, + NoSoul: true, StoreDir: t.TempDir(), MemoryDir: t.TempDir(), + Diagnostics: port.NopDiagnostics{}, + temporaryStorage: temporaryStorageConfig{ + Mode: temporaryStorageManaged, ManagedRoot: filepath.Join(t.TempDir(), "managed"), SystemTempDir: t.TempDir(), + }, + } +} + +func assertRecoveryRun(t *testing.T, svc *server.Service, id session.SessionID, output string) { + t.Helper() + run, err := svc.StartRunContent(context.Background(), id, "execute the work", nil) + if err != nil { + t.Fatalf("StartRunContent: %v", err) + } + var manifests, bashResults int + var completed bool + for ev := range run.Events() { + if ev.RequestManifest != nil { + manifests++ + if !slices.Contains(ev.RequestManifest.ToolNames, "Bash") { + t.Error("continued request manifest omitted Bash") + } + var advertised bool + for _, decision := range ev.RequestManifest.ToolDecisions { + if decision.Name == "Bash" { + advertised = advertised || decision.Decision == session.RequestToolAdvertised + if decision.Decision == session.RequestToolMountUnavailable { + t.Error("recovered Bash reported mount_unavailable") + } + } + } + if !advertised { + t.Error("request manifest lacks Bash advertised decision") + } + } + if ev.ToolResult != nil { + if ev.ToolResult.IsError { + t.Errorf("tool failed: %s", ev.ToolResult.Content) + } + if strings.Contains(ev.ToolResult.Content, output) { + bashResults++ + } + } + if ev.Result != nil { + completed = ev.Result.Stop == session.StopEndTurn + } + } + if manifests == 0 || bashResults == 0 || !completed { + t.Fatalf("missing successful Bash evidence: manifests=%d output=%d completed=%v", manifests, bashResults, completed) + } +} diff --git a/internal/app/placement.go b/internal/app/placement.go index fbecac9187..e6b1d2ebb3 100644 --- a/internal/app/placement.go +++ b/internal/app/placement.go @@ -2,6 +2,7 @@ package app import ( "context" + "fmt" "github.com/stacklok/mecatl/engine/adapter/memledger" "github.com/stacklok/mecatl/engine/adapter/nofs" @@ -25,7 +26,7 @@ type localPlacementProvider struct { scope server.PlacementScope root string workspace server.WorkspaceFactory - runnerForRoot func(string) tool.CommandRunner + runnerForRoot func(string) (tool.CommandRunner, error) worktrees server.WorktreeLister selectors *server.WorktreeSelectorIssuer } @@ -154,7 +155,11 @@ func (p *localPlacementProvider) bindWorktree(choice server.Worktree) (server.Pl } var runner tool.CommandRunner if p.runnerForRoot != nil { - runner = p.runnerForRoot(ws.Root()) + var err error + runner, err = p.runnerForRoot(ws.Root()) + if err != nil { + return server.PlacementBinding{}, fmt.Errorf("%w: %w", server.ErrPlacementUnavailable, err) + } } env, err := tool.NewEnvironment(ref, ws, memledger.New(), runner) if err != nil { @@ -182,7 +187,11 @@ func (p *localPlacementProvider) bindLocal() (server.PlacementBinding, error) { ref := configuredLocalPlacementRef(p.root) var runner tool.CommandRunner if p.runnerForRoot != nil { - runner = p.runnerForRoot(ws.Root()) + var err error + runner, err = p.runnerForRoot(ws.Root()) + if err != nil { + return server.PlacementBinding{}, fmt.Errorf("%w: %w", server.ErrPlacementUnavailable, err) + } } env, err := tool.NewEnvironment(ref, ws, memledger.New(), runner) if err != nil { diff --git a/internal/app/placement_test.go b/internal/app/placement_test.go index eff6773e1d..6a51136838 100644 --- a/internal/app/placement_test.go +++ b/internal/app/placement_test.go @@ -52,6 +52,49 @@ func (l *scriptedWorktreeLister) List(context.Context, string) ([]server.Worktre return out, nil } +func TestLocalPlacementPropagatesRunnerErrors(t *testing.T) { + failure := errors.New("configured runner failed") + root := t.TempDir() + choice := server.Worktree{Path: t.TempDir(), Branch: "feature", Head: "head"} + issuer, err := server.NewWorktreeSelectorIssuer(make([]byte, 32)) + if err != nil { + t.Fatal(err) + } + provider := &localPlacementProvider{ + scope: "test", root: root, selectors: issuer, + workspace: func(root string) tool.Workspace { return memfs.NewWorkspace(root) }, + worktrees: &scriptedWorktreeLister{responses: [][]server.Worktree{{choice}}}, + runnerForRoot: func(string) (tool.CommandRunner, error) { return nil, failure }, + } + ctx := context.Background() + sourceRef := configuredLocalPlacementRef(root) + choices, err := provider.ListWorktrees(ctx, server.PlacementDiscoveryRequest{Source: "source", SourceRef: sourceRef, Scope: "test"}) + if err != nil || len(choices) != 1 { + t.Fatalf("worktree choices: %v, %v", choices, err) + } + for name, bind := range map[string]func() (server.PlacementBinding, error){ + "Bind default": func() (server.PlacementBinding, error) { + return provider.Bind(ctx, server.PlacementBindRequest{Selector: server.DefaultPlacement(), Scope: "test"}) + }, + "Reattach default": func() (server.PlacementBinding, error) { + return provider.Reattach(ctx, server.PlacementReattachRequest{Ref: sourceRef, Scope: "test"}) + }, + "Bind worktree": func() (server.PlacementBinding, error) { + return provider.Bind(ctx, server.PlacementBindRequest{Selector: server.SelectWorktree("source", sourceRef, choices[0].Selector), Scope: "test"}) + }, + "Reattach worktree": func() (server.PlacementBinding, error) { + return provider.Reattach(ctx, server.PlacementReattachRequest{Ref: session.EnvironmentRef{Kind: session.EnvKindLocal, ID: choice.Path, Revision: choice.Head}, Scope: "test"}) + }, + } { + t.Run(name, func(t *testing.T) { + binding, err := bind() + if !errors.Is(err, failure) || !errors.Is(err, server.ErrPlacementUnavailable) || binding.Ref != (session.EnvironmentRef{}) { + t.Fatalf("runner error became a binding or lost its cause: %+v, %v", binding, err) + } + }) + } +} + func TestInvariant_local_placement_identity_and_atomic_worktree_binding(t *testing.T) { rootA, rootB := t.TempDir(), t.TempDir() providerA := &localPlacementProvider{scope: "test", root: rootA, workspace: func(root string) tool.Workspace { return memfs.NewWorkspace(root) }} diff --git a/internal/app/scheduletool_test.go b/internal/app/scheduletool_test.go index 2f14d9b74c..c7bbf49b8b 100644 --- a/internal/app/scheduletool_test.go +++ b/internal/app/scheduletool_test.go @@ -164,7 +164,7 @@ func newScheduleTestService(t *testing.T, store port.SessionStore, llm *mockllm. PlacementProvider: &localPlacementProvider{ scope: defaultPlacementScope, root: workspace, workspace: func(root string) tool.Workspace { return memfs.NewWorkspace(root) }, - runnerForRoot: func(string) tool.CommandRunner { return nil }, + runnerForRoot: func(string) (tool.CommandRunner, error) { return nil, nil }, }, PlacementScope: defaultPlacementScope, }) diff --git a/internal/app/subagent_perm_config_test.go b/internal/app/subagent_perm_config_test.go index bedda0aeff..c07b83f650 100644 --- a/internal/app/subagent_perm_config_test.go +++ b/internal/app/subagent_perm_config_test.go @@ -60,9 +60,9 @@ func runSubagentBash(t *testing.T, cfg Config, command string) session.ToolResul rf := &recordingForker{inner: forker.New(func(root string) (tool.Workspace, error) { return osfs.NewWorkspace(root) - }, forker.WithRunner(func(childRoot string) tool.CommandRunner { + }, forker.WithRunnerError(func(childRoot string) (tool.CommandRunner, error) { if cfg.NoBash || cfg.Shell == "" || !cfg.TrustProject { - return nil + return nil, nil } return newHardenedRunnerForRoot(cfg, childRoot) }))} diff --git a/internal/app/task_shell_test.go b/internal/app/task_shell_test.go index d94a7076ec..ee551ee07f 100644 --- a/internal/app/task_shell_test.go +++ b/internal/app/task_shell_test.go @@ -524,9 +524,9 @@ func newSubagentToolForTestOpts(t *testing.T, cfg Config, childProvider *mockllm roFk := forker.New(func(root string) (tool.Workspace, error) { return osfs.NewWorkspace(root) }, - append(opts, forker.WithRunner(func(childRoot string) tool.CommandRunner { + append(opts, forker.WithRunnerError(func(childRoot string) (tool.CommandRunner, error) { if cfg.NoBash || cfg.Shell == "" || !cfg.TrustProject { - return nil + return nil, nil } return newHardenedRunnerForRoot(cfg, childRoot) }))...) diff --git a/internal/app/teams_test.go b/internal/app/teams_test.go index f6ab845394..cf530dea59 100644 --- a/internal/app/teams_test.go +++ b/internal/app/teams_test.go @@ -290,22 +290,22 @@ func TestReadOnlyMemberRunsGitInWorktreeEndToEnd(t *testing.T) { // builder applies the SAME hardening buildSandboxedCommandRunner/ // buildForceCopyRunner do — trust-gated for the read-only worktree forker, // ungated for the force-copy mutating forker. - sandboxedRunnerBuilder := func(childRoot string) tool.CommandRunner { + sandboxedRunnerBuilder := func(childRoot string) (tool.CommandRunner, error) { if cfg.NoBash || cfg.Shell == "" || !cfg.TrustProject { - return nil + return nil, nil } return newHardenedRunnerForRoot(cfg, childRoot) } - forceCopyRunnerBuilder := func(childRoot string) tool.CommandRunner { + forceCopyRunnerBuilder := func(childRoot string) (tool.CommandRunner, error) { if cfg.NoBash || cfg.Shell == "" { - return nil + return nil, nil } return newHardenedRunnerForRoot(cfg, childRoot) } roFk := forker.New(func(root string) (tool.Workspace, error) { return osfs.NewWorkspace(root) }, - forker.WithTempBase(worktreeBase), forker.WithRunner(sandboxedRunnerBuilder)) + forker.WithTempBase(worktreeBase), forker.WithRunnerError(sandboxedRunnerBuilder)) mutatingFk := forker.New(func(root string) (tool.Workspace, error) { return osfs.NewWorkspace(root) }, - forker.WithForceCopy(), forker.WithRunner(forceCopyRunnerBuilder)) + forker.WithForceCopy(), forker.WithRunnerError(forceCopyRunnerBuilder)) runner := buildSandboxedCommandRunner(cfg) if runner == nil { t.Fatal("precondition: expected a non-nil sandboxed runner") diff --git a/internal/app/temporary_storage.go b/internal/app/temporary_storage.go index bc7c520d20..2377525912 100644 --- a/internal/app/temporary_storage.go +++ b/internal/app/temporary_storage.go @@ -5,11 +5,11 @@ import ( "os" "path/filepath" "runtime" - "sync" "time" "github.com/stacklok/mecatl/internal/adapter/managedtemp" "github.com/stacklok/mecatl/internal/adapter/permconfig" + "github.com/stacklok/mecatl/internal/adapter/server" ) type temporaryStorageMode string @@ -32,9 +32,7 @@ type temporaryStorageConfig struct { } type managedTemporaryStorage struct { - namespace *managedtemp.Namespace - mu sync.Mutex - workspaces []*managedtemp.Workspace + namespace *managedtemp.Namespace } func openManagedTemporaryStorage(cfg temporaryStorageConfig) (*managedTemporaryStorage, error) { @@ -48,10 +46,7 @@ func openManagedTemporaryStorage(cfg temporaryStorageConfig) (*managedTemporaryS return &managedTemporaryStorage{namespace: namespace}, nil } -func (s *managedTemporaryStorage) workspace(root string) (*managedtemp.Workspace, error) { - if s == nil { - return nil, nil - } +func (s *managedTemporaryStorage) allocator(root string) (func(string) (*managedtemp.Lease, error), error) { identity, err := managedWorkspaceIdentity(root) if err != nil { return nil, err @@ -60,22 +55,22 @@ func (s *managedTemporaryStorage) workspace(root string) (*managedtemp.Workspace if err != nil { return nil, err } - s.mu.Lock() - s.workspaces = append(s.workspaces, workspace) - s.mu.Unlock() - return workspace, nil + if err := workspace.Close(); err != nil { + return nil, err + } + return func(kind string) (*managedtemp.Lease, error) { + lease, err := s.namespace.AllocateWorkspace("osfs", identity, identity, kind) + if err != nil { + return nil, server.ErrManagedTemporaryStorageUnavailable + } + return lease, nil + }, nil } func (s *managedTemporaryStorage) close() { if s == nil { return } - s.mu.Lock() - for _, workspace := range s.workspaces { - _ = workspace.Close() - } - s.workspaces = nil - s.mu.Unlock() _ = s.namespace.Close() } diff --git a/user-docs/building/what-you-get/core-tools.md b/user-docs/building/what-you-get/core-tools.md index dd6140e20b..19a91d28e0 100644 --- a/user-docs/building/what-you-get/core-tools.md +++ b/user-docs/building/what-you-get/core-tools.md @@ -66,6 +66,24 @@ TTL. It never sweeps arbitrary system temporary files and never blocks command allocation. The lifecycle is available on Linux and macOS; other platforms must use system mode. +If the managed root disappears while the harness is running, the next session +attachment, command allocation, or maintenance pass automatically recreates it. +An existing replacement is used only if it is a real directory owned by the +harness user with private permissions and a valid managed layout. Recovery does +not restore deleted files or move temporary leases for commands already running; +a command interrupted by removal can still fail. Retry that command or continue +the session after recovery. + +An unsafe root is never repaired by weakening its security checks, and a session +configured for Bash must not silently continue without its shell. If you see +**managed temporary storage unavailable**, inspect the operator-configured +`managed_root` and its parent: restore the missing parent, resolve any unexpected +file or symlink, and ensure the managed root is owned by the harness user with +mode `0700` (protocol files use `0600`). Inspect unexpected contents before making +changes; do not point the root at another user's directory or make it broadly +writable. Then retry the session or delegation. Intentional `--no-bash` and +`no-fs` sessions do not require managed-shell recovery. + An operator can set `temporary_storage.mode: system` in user-global `~/.config/mecatl/settings.yaml` to restore system temporary storage. This rollback mode creates no new managed leases, runs no reaper, and leaves existing managed