diff --git a/daemon/AGENTS.md b/daemon/AGENTS.md index 63f3a18..0b186c9 100644 --- a/daemon/AGENTS.md +++ b/daemon/AGENTS.md @@ -11,6 +11,9 @@ databases, command parsing, and shutdown policy belong to the caller. - Never infer live daemon state from a runtime record alone. Probe the endpoint before claiming a process is reachable. +- With PID-aware discovery, retain a live record's probe failure until another + reachable record wins. Absence is a definite result, not a fallback for an + endpoint that could not be probed. - Treat process-creation identity as opaque and exact-match only. An unknown identity never authorizes destructive action against a process or record. - Never send a bearer credential to a runtime-record endpoint before it proves diff --git a/daemon/manager_test.go b/daemon/manager_test.go index e9567c2..93b5e17 100644 --- a/daemon/manager_test.go +++ b/daemon/manager_test.go @@ -84,6 +84,40 @@ func TestManagerFindSkipsIncompatibleDaemon(t *testing.T) { assert.False(t, ok) } +func TestManagerEnsureDoesNotStartWhenDiscoveryIsUnreachable(t *testing.T) { + assert := assert.New(t) + require := require.New(t) + + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + http.Error(w, "not ready", http.StatusServiceUnavailable) + })) + defer server.Close() + store := daemon.RuntimeStore{Dir: t.TempDir()} + _, err := store.Write(daemon.NewRuntimeRecord("tool", "v1", daemon.Endpoint{ + Network: daemon.NetworkTCP, + Address: listenerAddr(t, server), + })) + require.NoError(err) + + started := false + manager := daemon.Manager{ + Store: store, + Discover: daemon.DiscoverOptions{ + Probe: daemon.ProbeOptions{ExpectedService: "tool"}, + RequirePIDAlive: true, + }, + Start: func(context.Context) error { + started = true + return nil + }, + } + + _, _, err = manager.Ensure(context.Background(), time.Second) + require.Error(err) + require.ErrorIs(err, daemon.ErrDaemonUnreachable) + assert.False(started) +} + func TestManagerFindScansPastIncompatibleDaemon(t *testing.T) { assert := assert.New(t) require := require.New(t) diff --git a/daemon/probe.go b/daemon/probe.go index ea36af0..33aad6a 100644 --- a/daemon/probe.go +++ b/daemon/probe.go @@ -133,11 +133,43 @@ func probeHTTP( type DiscoverOptions struct { Probe ProbeOptions // Proof enables proof-of-possession probing for each runtime record. - Proof *Proof + Proof *Proof + // RequirePIDAlive rejects dead processes and definite process-identity + // mismatches. It also makes failed probes indeterminate: Discover returns an + // UnreachableError when no later record succeeds. The false zero value keeps + // the earlier compatibility behavior, where failed probes are skipped and + // discovery can report definite absence. RequirePIDAlive bool Accept func(RuntimeRecord, PingInfo) bool } +// ErrDaemonUnreachable reports a live runtime record whose endpoint could not +// be proved usable. It classifies every Probe or proof-of-possession failure, +// including transport, HTTP, decoding, service-identity, and proof errors. +// Callers should inspect the wrapped error before choosing operator guidance. +var ErrDaemonUnreachable = errors.New("daemon is unreachable") + +// UnreachableError retains the runtime and probe failure for a live daemon +// candidate that discovery could not safely select. +type UnreachableError struct { + Record RuntimeRecord + Endpoint Endpoint + Err error +} + +func (e *UnreachableError) Error() string { + return fmt.Sprintf("%v: pid %d at %s: %v", + ErrDaemonUnreachable, e.Record.PID, e.Endpoint.ConfigAddress(), e.Err) +} + +// Unwrap preserves both the classification and the endpoint probe failure. +func (e *UnreachableError) Unwrap() []error { + if e.Err == nil { + return []error{ErrDaemonUnreachable} + } + return []error{ErrDaemonUnreachable, e.Err} +} + // Discover scans runtime records and returns the first live daemon. func Discover(ctx context.Context, store RuntimeStore, opts DiscoverOptions) (RuntimeRecord, PingInfo, bool, error) { if err := ctx.Err(); err != nil { @@ -147,16 +179,21 @@ func Discover(ctx context.Context, store RuntimeStore, opts DiscoverOptions) (Ru if err != nil { return RuntimeRecord{}, PingInfo{}, false, err } + var unreachable error for _, rec := range records { if err := ctx.Err(); err != nil { return RuntimeRecord{}, PingInfo{}, false, err } - if opts.RequirePIDAlive && !ProcessAlive(rec.PID) { - continue + if opts.RequirePIDAlive { + if !ProcessAlive(rec.PID) || + CompareRuntimeProcessIdentity(rec) == ProcessIdentityMismatch { + continue + } } + ep := rec.Endpoint() var info PingInfo if opts.Proof == nil { - info, err = Probe(ctx, rec.Endpoint(), opts.Probe) + info, err = Probe(ctx, ep, opts.Probe) } else { info, err = opts.Proof.Probe(ctx, rec, opts.Probe) } @@ -164,6 +201,13 @@ func Discover(ctx context.Context, store RuntimeStore, opts DiscoverOptions) (Ru if ctxErr := ctx.Err(); ctxErr != nil { return RuntimeRecord{}, PingInfo{}, false, ctxErr } + if opts.RequirePIDAlive && unreachable == nil { + unreachable = &UnreachableError{ + Record: rec, + Endpoint: ep, + Err: err, + } + } continue } if opts.RequirePIDAlive && info.PID != rec.PID { @@ -174,5 +218,5 @@ func Discover(ctx context.Context, store RuntimeStore, opts DiscoverOptions) (Ru } return rec, info, true, nil } - return RuntimeRecord{}, PingInfo{}, false, nil + return RuntimeRecord{}, PingInfo{}, false, unreachable } diff --git a/daemon/probe_test.go b/daemon/probe_test.go index b437495..2c53cd9 100644 --- a/daemon/probe_test.go +++ b/daemon/probe_test.go @@ -5,6 +5,7 @@ import ( "bytes" "context" "encoding/json" + "errors" "fmt" "io" "net" @@ -169,6 +170,141 @@ func TestDiscoverRejectsPIDMismatchWhenRequiringLivePID(t *testing.T) { assert.False(t, ok) } +func TestDiscoverSkipsMismatchedProcessIdentityWithoutProbing(t *testing.T) { + assert := assert.New(t) + require := require.New(t) + + livePID := startLivePIDHelper(t) + recordedIdentity, ok := daemon.ReadProcessIdentity(os.Getpid()) + require.True(ok) + require.Equal(daemon.ProcessIdentityMismatch, + daemon.CompareProcessIdentity(livePID, recordedIdentity)) + + var probes atomic.Int32 + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + probes.Add(1) + http.Error(w, "must not be probed", http.StatusServiceUnavailable) + })) + defer server.Close() + + store := daemon.RuntimeStore{Dir: t.TempDir()} + _, err := store.Write(daemon.RuntimeRecord{ + PID: livePID, + ProcessIdentity: recordedIdentity, + Network: daemon.NetworkTCP, + Address: listenerAddr(t, server), + Service: "kata", + StartedAt: time.Now(), + }) + require.NoError(err) + + _, _, found, err := daemon.Discover(context.Background(), store, daemon.DiscoverOptions{ + Probe: daemon.ProbeOptions{ExpectedService: "kata"}, + RequirePIDAlive: true, + }) + require.NoError(err) + assert.False(found) + assert.Zero(probes.Load()) +} + +func TestDiscoverWithoutPIDCheckKeepsFailedProbeAsAbsence(t *testing.T) { + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + http.Error(w, "not ready", http.StatusServiceUnavailable) + })) + defer server.Close() + + store := daemon.RuntimeStore{Dir: t.TempDir()} + _, err := store.Write(daemon.NewRuntimeRecord("kata", "v1", daemon.Endpoint{ + Network: daemon.NetworkTCP, + Address: listenerAddr(t, server), + })) + require.NoError(t, err) + + _, _, found, err := daemon.Discover(context.Background(), store, daemon.DiscoverOptions{ + Probe: daemon.ProbeOptions{ExpectedService: "kata"}, + }) + require.NoError(t, err) + assert.False(t, found) +} + +func TestDiscoverReturnsUnreachableErrorForLiveRuntime(t *testing.T) { + assert := assert.New(t) + require := require.New(t) + + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + http.Error(w, "not ready", http.StatusServiceUnavailable) + })) + defer server.Close() + + rec := daemon.NewRuntimeRecord("kata", "v1", daemon.Endpoint{ + Network: daemon.NetworkTCP, + Address: listenerAddr(t, server), + }) + store := daemon.RuntimeStore{Dir: t.TempDir()} + _, err := store.Write(rec) + require.NoError(err) + + _, _, ok, err := daemon.Discover(context.Background(), store, daemon.DiscoverOptions{ + Probe: daemon.ProbeOptions{ExpectedService: "kata"}, + RequirePIDAlive: true, + }) + require.Error(err) + assert.False(ok) + require.ErrorIs(err, daemon.ErrDaemonUnreachable) + unreachable, ok := errors.AsType[*daemon.UnreachableError](err) + if !ok || unreachable == nil { + require.FailNow("expected UnreachableError") + return + } + assert.Equal(rec.PID, unreachable.Record.PID) + assert.Equal(rec.Endpoint(), unreachable.Endpoint) + require.Error(unreachable.Err) + require.ErrorIs(err, unreachable.Err) +} + +func TestDiscoverScansPastUnreachableLiveRuntime(t *testing.T) { + assert := assert.New(t) + require := require.New(t) + + unreachableServer := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + http.Error(w, "not ready", http.StatusServiceUnavailable) + })) + defer unreachableServer.Close() + reachableServer := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + _, _ = fmt.Fprintf(w, `{"ok":true,"service":"kata","pid":%d}`, os.Getpid()) + })) + defer reachableServer.Close() + + livePID := startLivePIDHelper(t) + identity, ok := daemon.ReadProcessIdentity(livePID) + require.True(ok) + store := daemon.RuntimeStore{Dir: t.TempDir()} + _, err := store.Write(daemon.RuntimeRecord{ + PID: livePID, + ProcessIdentity: identity, + Network: daemon.NetworkTCP, + Address: listenerAddr(t, unreachableServer), + Service: "kata", + StartedAt: time.Now().Add(-time.Minute), + }) + require.NoError(err) + reachable := daemon.NewRuntimeRecord("kata", "v1", daemon.Endpoint{ + Network: daemon.NetworkTCP, + Address: listenerAddr(t, reachableServer), + }) + _, err = store.Write(reachable) + require.NoError(err) + + found, info, ok, err := daemon.Discover(context.Background(), store, daemon.DiscoverOptions{ + Probe: daemon.ProbeOptions{ExpectedService: "kata"}, + RequirePIDAlive: true, + }) + require.NoError(err) + require.True(ok) + assert.Equal(reachable.PID, found.PID) + assert.Equal("kata", info.Service) +} + func TestManagerFindDoesNotDiscloseCredentialBeforeProof(t *testing.T) { assert := assert.New(t) require := require.New(t) @@ -212,8 +348,9 @@ func TestManagerFindDoesNotDiscloseCredentialBeforeProof(t *testing.T) { } _, _, ok, err := manager.Find(context.Background()) - require.NoError(err) + require.Error(err) assert.False(ok) + require.ErrorIs(err, daemon.ErrDaemonUnreachable) assert.False(credentialDisclosed.Load(), "credential material reached an unproved endpoint") } diff --git a/docs/research/vite-task-sandbox-ipc.md b/docs/research/vite-task-sandbox-ipc.md new file mode 100644 index 0000000..8abb473 --- /dev/null +++ b/docs/research/vite-task-sandbox-ipc.md @@ -0,0 +1,383 @@ +# Vite Task's sandbox-compatible IPC changes + +## Scope + +Vite+ v0.2.9 says `vp run` now works in the default Codex CLI and Claude Code +sandboxes. The release attributes the change to Vite Task PRs +[#569](https://github.com/voidzero-dev/vite-task/pull/569) and +[#576](https://github.com/voidzero-dev/vite-task/pull/576). These are two +separate fixes: runner communication stopped using Unix domain sockets, and +file-access tracking stopped using POSIX shared memory and a Linux descriptor +broker. The [release note](https://github.com/voidzero-dev/vite-plus/releases/tag/v0.2.9) +describes the user-visible result. + +Vite+ picked up both fixes by updating its Vite Task revision to +`d05b1dcdbaabaa69643ee0b89cebe3cd390957e9` in +[#2403](https://github.com/voidzero-dev/vite-plus/pull/2403). The pinned +revision appears on the +[`fspy` dependency](https://github.com/voidzero-dev/vite-plus/blob/b0fa8dc1ba242cb1d81326fb06ac54a81f75ef40/Cargo.toml#L186) +and the +[`vt` dependencies](https://github.com/voidzero-dev/vite-plus/blob/b0fa8dc1ba242cb1d81326fb06ac54a81f75ef40/Cargo.toml#L299-L304). +That revision contains the merged commits +[`f5919072`](https://github.com/voidzero-dev/vite-task/commit/f5919072f109463286f872b36fa2bed3d213cb91) +for PR #569 and +[`29fcbd62`](https://github.com/voidzero-dev/vite-task/commit/29fcbd62587360433886f265516b264129cdf439) +for PR #576. + +## What Vite Task changed + +### Runner communication uses named pipes + +The original failure happened before task code ran. The cached task runner +tried to bind a Unix domain socket and received `EPERM` in both default +sandboxes, as recorded in +[#562](https://github.com/voidzero-dev/vite-task/issues/562). PR #569 kept the +socket-like API but changed its Unix implementation to filesystem FIFOs. +Windows continues to use named pipes. The transport's public API exposes only +bind, name, accept, and connect, leaving the platform choice inside the +[`socket_ipc` crate](https://github.com/voidzero-dev/vite-task/blob/f5919072f109463286f872b36fa2bed3d213cb91/crates/socket_ipc/src/lib.rs#L20-L107). + +On Unix, each server owns a private temporary directory with a well-known +`connect` FIFO. A client creates one request FIFO and one response FIFO in that +directory, then announces a 16-byte UUID. The announcement fits within +`PIPE_BUF`, so concurrent announcements cannot interleave. The server replies +with one ready byte before the two FIFOs become the connection's byte streams. +The protocol and its permissions are documented and implemented in +[`unix.rs`](https://github.com/voidzero-dev/vite-task/blob/f5919072f109463286f872b36fa2bed3d213cb91/crates/socket_ipc/src/unix.rs#L1-L38) +and the +[`Server` setup](https://github.com/voidzero-dev/vite-task/blob/f5919072f109463286f872b36fa2bed3d213cb91/crates/socket_ipc/src/unix.rs#L68-L127). + +The interesting part is failure behavior. The client opens the rendezvous FIFO +nonblocking before it creates per-client state, translating `ENXIO` into +`ConnectionRefused` when no server reader exists. During the ready-byte +handshake it polls the response and rendezvous descriptors, then reopens the +rendezvous every 100 ms as a liveness probe for macOS, where FIFO events from +`poll` are not reliable enough. The server treats a client that dies during +the handshake as a per-client failure and accepts the next announcement. See +the +[`Client::connect` implementation](https://github.com/voidzero-dev/vite-task/blob/f5919072f109463286f872b36fa2bed3d213cb91/crates/socket_ipc/src/unix.rs#L168-L303) +and the +[`accept` loop](https://github.com/voidzero-dev/vite-task/blob/f5919072f109463286f872b36fa2bed3d213cb91/crates/socket_ipc/src/unix.rs#L98-L127). +Integration tests cover both a stale announcement and a server that disappears +before or during connection, with timeouts that make a hang observable +([tests](https://github.com/voidzero-dev/vite-task/blob/f5919072f109463286f872b36fa2bed3d213cb91/crates/socket_ipc/tests/integration.rs#L79-L175)). + +The server does not register established FIFOs with Tokio's reactor on macOS. +It hands ordinary file descriptors to Tokio's blocking file implementation +because kqueue did not wake reliably for these FIFOs +([source](https://github.com/voidzero-dev/vite-task/blob/f5919072f109463286f872b36fa2bed3d213cb91/crates/socket_ipc/src/unix.rs#L118-L125)). + +This is child-runner IPC, not general daemon discovery. The server creates a +unique endpoint and passes its opaque name to child processes through an +environment variable +([server](https://github.com/voidzero-dev/vite-task/blob/f5919072f109463286f872b36fa2bed3d213cb91/crates/vt_server/src/lib.rs#L261-L294), +[client](https://github.com/voidzero-dev/vite-task/blob/f5919072f109463286f872b36fa2bed3d213cb91/crates/vt_client/src/lib.rs#L28-L48)). +A long-lived daemon still needs a stable, access-controlled way for unrelated +clients to find the current endpoint. + +### File tracking uses a sparse temporary file + +The second failure appeared after runner IPC was unblocked. Codex's macOS +Seatbelt profile denied `shm_open`; on Linux, the earlier `memfd` design needed +a Unix socket broker to pass descriptors. Vite Task tracked those failures in +[#563](https://github.com/voidzero-dev/vite-task/issues/563) and replaced all +three platform backends in PR #576. + +The replacement creates a uniquely named sparse file in the system temporary +directory, resolves its absolute path once, and passes that path as the opaque +identifier. Unix creates it with mode `0600`; Windows relies on the per-user +temporary-directory ACL and marks the file sparse before setting its length. +Every platform maps the file with `memmap2`. This removes the descriptor +broker, global shared-memory names, and the need for an asynchronous runtime +([design](https://github.com/voidzero-dev/vite-task/blob/29fcbd62587360433886f265516b264129cdf439/crates/fspy_shm/README.md#L25-L56), +[creation code](https://github.com/voidzero-dev/vite-task/blob/29fcbd62587360433886f265516b264129cdf439/crates/fspy_shm/src/file_backed.rs#L58-L110)). + +Vite Task separates the lifetime of the name, the open file, and each mapping. +Dropping the keeper removes the name so later opens fail, while existing +handles and mappings remain usable. Channel shutdown uses a separate lock-file +gate rather than treating unlink as a stop signal +([lifetime contract](https://github.com/voidzero-dev/vite-task/blob/29fcbd62587360433886f265516b264129cdf439/crates/fspy_shm/README.md#L62-L72), +[channel gate](https://github.com/voidzero-dev/vite-task/blob/29fcbd62587360433886f265516b264129cdf439/crates/fspy_shared/src/ipc/channel/mod.rs#L41-L58)). +Tests pin name removal and continued access through existing mappings and +handles +([tests](https://github.com/voidzero-dev/vite-task/blob/29fcbd62587360433886f265516b264129cdf439/crates/fspy_shm/src/file_backed.rs#L261-L304)). + +The design accepts one cleanup limit explicitly. If the keeper process is +killed, its drop handler does not run and the backing file remains for a temp +reaper or cleanup tool. Because the file is sparse, its physical cost is tied +to pages actually written rather than its logical capacity +([lifetime notes](https://github.com/voidzero-dev/vite-task/blob/29fcbd62587360433886f265516b264129cdf439/crates/fspy_shm/README.md#L62-L72)). + +## Lessons worth carrying into daemon work + +1. Treat sandbox compatibility as a transport constraint. Vite Task chose + primitives available in the default profiles, ordinary files and named + pipes, instead of asking callers for wider sandbox permissions + ([release](https://github.com/voidzero-dev/vite-plus/releases/tag/v0.2.9)). +2. Make endpoint identity portable across process context. Vite Task sends an + opaque absolute path, then tests a child with a different working directory + and temporary-directory environment + ([implementation and test](https://github.com/voidzero-dev/vite-task/blob/29fcbd62587360433886f265516b264129cdf439/crates/fspy_shm/src/file_backed.rs#L81-L135), + [subprocess test](https://github.com/voidzero-dev/vite-task/blob/29fcbd62587360433886f265516b264129cdf439/crates/fspy_shm/src/file_backed.rs#L227-L259)). +3. Design dead-peer behavior before the happy path. Nonblocking opens, + bounded liveness probes, stale-client isolation, and explicit no-hang tests + are the strongest reusable part of PR #569 + ([client](https://github.com/voidzero-dev/vite-task/blob/f5919072f109463286f872b36fa2bed3d213cb91/crates/socket_ipc/src/unix.rs#L168-L303), + [tests](https://github.com/voidzero-dev/vite-task/blob/f5919072f109463286f872b36fa2bed3d213cb91/crates/socket_ipc/tests/integration.rs#L79-L175)). +4. Separate discovery, liveness, data lifetime, and shutdown. In Vite Task, + the endpoint name finds a server, the rendezvous detects server death, open + mappings keep bytes alive, and the channel's gate rejects new writers. + Unlinking alone does not carry all four meanings + ([FIFO protocol](https://github.com/voidzero-dev/vite-task/blob/f5919072f109463286f872b36fa2bed3d213cb91/crates/socket_ipc/src/unix.rs#L1-L38), + [mapping lifetime](https://github.com/voidzero-dev/vite-task/blob/29fcbd62587360433886f265516b264129cdf439/crates/fspy_shm/README.md#L62-L72)). +5. Test the actual restricted environment and the user-visible behavior. The + Codex fixture runs a nested cached task in the workspace profile, changes a + file read by that task, and records the next run as a cache miss + ([Codex snapshot](https://github.com/voidzero-dev/vite-task/blob/f5919072f109463286f872b36fa2bed3d213cb91/crates/vt_bin/tests/e2e_snapshots/fixtures/sandboxed_fspy/snapshots/fspy_under_codex_sandbox.md#L1-L24)). + The Claude fixture records the equivalent flow under Sandbox Runtime + ([Claude snapshot](https://github.com/voidzero-dev/vite-task/blob/f5919072f109463286f872b36fa2bed3d213cb91/crates/vt_bin/tests/e2e_snapshots/fixtures/sandboxed_fspy/snapshots/fspy_under_anthropic_sandbox_runtime.md#L1-L32)). + +The FIFO protocol is useful when both processes share a writable directory +but Unix sockets and loopback networking are unavailable. It is not a drop-in +replacement for every daemon socket. Vite Task pays for it with a custom +handshake, two FIFOs per client, SIGPIPE precautions, a macOS polling probe, +and blocking-pool I/O. The sparse mapped file solves high-volume file-access +recording, not request-response control traffic. Those boundaries are visible +in the +[`socket_ipc` implementation](https://github.com/voidzero-dev/vite-task/blob/f5919072f109463286f872b36fa2bed3d213cb91/crates/socket_ipc/src/unix.rs#L1-L38) +and the +[`fspy_shm` design](https://github.com/voidzero-dev/vite-task/blob/29fcbd62587360433886f265516b264129cdf439/crates/fspy_shm/README.md#L25-L60). + +## Comparison with Kit's daemon package + +Kit currently supports HTTP over TCP and Unix sockets. Empty-address parsing +prefers a caller-supplied Unix path, `DefaultSocketPath` prepares that path, +and `Listen` ultimately calls `net.Listen` for the chosen network +([endpoint.go](../../daemon/endpoint.go), [listen.go](../../daemon/listen.go)). +`StartDetached` creates a new Unix session with `setsid`, but it does not and +cannot remove an inherited sandbox policy +([start.go](../../daemon/start.go), [detach_unix.go](../../daemon/detach_unix.go)). + +Loopback TCP is not a dependable automatic fallback. Current official OpenAI +documentation says Codex defaults command network access off. When the command +network proxy is enabled, local binding defaults to false and Unix sockets need +explicit allow rules +([Codex agent approvals and security](https://learn.chatgpt.com/docs/agent-approvals-security#network-access)). +Claude Code also applies its sandbox boundary to every Bash command and child +process. Its documented defaults set `network.allowLocalBinding` and +`network.allowAllUnixSockets` to false +([sandboxing](https://code.claude.com/docs/en/sandboxing), +[settings](https://code.claude.com/docs/en/settings#sandbox-settings)). + +The practical options are: + +1. Improve failure classification first. Preserve the underlying permission + error, name the transport and endpoint that failed, and tell callers that + the sandbox must allow that primitive. This makes today's failure actionable + without pretending another socket type will work. +2. If the package must work in the default sandbox profiles without permission + changes, add a file-backed byte-stream transport behind `Endpoint`. Keep its + name opaque and absolute, place it in a private writable directory, and + define bounded connect and dead-peer behavior before exposing it to HTTP. + A silent Unix-to-TCP retry would not meet that requirement. +3. Keep sandbox-compatible transport separate from daemon persistence. Vite + Task proves child IPC during one command. It does not establish that a + detached daemon survives the end of a Codex or Claude command, or that later + sandbox invocations can see the same temporary directory. Those observables + need their own end-to-end tests in both real default profiles. + +## Kata and RoboRev follow-up + +Recent Kata and RoboRev changes confirm one immediate Kit defect and put useful +limits around the larger transport idea. Both applications observed a readable +runtime record whose live process could not be reached from a sandbox. Both had +to stop treating that state as daemon absence. + +### Kata + +[Kata PR #266](https://github.com/kenn-io/kata/pull/266), merged as +[`e62bb677`](https://github.com/kenn-io/kata/commit/e62bb6776e304684f33b2a5d8465793e3120e0ab), +added `ErrLocalDaemonUnreachable`. Discovery now retains the first probe error +for a runtime whose process identity is still live, continues looking for a +usable record, and returns the retained error only when no usable daemon wins. +The caller therefore does not auto-start a competing daemon merely because its +sandbox cannot dial the recorded endpoint +([discovery](https://github.com/kenn-io/kata/blob/e62bb6776e304684f33b2a5d8465793e3120e0ab/internal/client/ensure.go#L179-L218), +[error](https://github.com/kenn-io/kata/blob/e62bb6776e304684f33b2a5d8465793e3120e0ab/internal/client/client.go#L63-L117), +[tests](https://github.com/kenn-io/kata/blob/e62bb6776e304684f33b2a5d8465793e3120e0ab/internal/client/ensure_test.go#L205-L257)). +The same PR rejects definite process-identity mismatches before reporting, +selecting, or signaling a PID. This matters because an old runtime file can +refer to an unrelated process after PID reuse. + +[Kata PR #273](https://github.com/kenn-io/kata/pull/273), merged as +[`abf39b23`](https://github.com/kenn-io/kata/commit/abf39b23eb06b8f3e8576b6e973b1669d16c9cf8), +added a runtime-directory write probe before spawning a detached child. A +permission failure now names the state directory, preserves `os.ErrPermission`, +mentions filesystem or sandbox access, and never starts the child +([implementation](https://github.com/kenn-io/kata/blob/abf39b23eb06b8f3e8576b6e973b1669d16c9cf8/internal/client/ensure.go#L281-L345), +[test](https://github.com/kenn-io/kata/blob/abf39b23eb06b8f3e8576b6e973b1669d16c9cf8/internal/client/ensure_test.go#L177-L200)). +Kit already absorbed the reusable part in +[Kit PR #72](https://github.com/kenn-io/kit/pull/72), merged as +[`eda6f084`](https://github.com/kenn-io/kit/commit/eda6f0848a53c634749192101654031f8d8e2956), +through `RuntimeStore.CheckWritable`. [Kata PR +#280](https://github.com/kenn-io/kata/pull/280), merged as +[`556def29`](https://github.com/kenn-io/kata/commit/556def2934b4132698c5e45d8c6890ed8dab6e03), +then deleted Kata's local copy and kept only its operator-facing wording. The +Kit contract correctly calls this preflight advisory. It does not prove that a +child has the same filesystem access or can bind its endpoint +([Kit implementation](../../daemon/runtime.go)). + +[Kata PR #278](https://github.com/kenn-io/kata/pull/278), merged as +[`c31c2a87`](https://github.com/kenn-io/kata/commit/c31c2a875e18115a74557ef59dc94ad600e5a521), +added `kata daemon locate`. External clients can ask Kata to apply its actual +named-daemon, configured-remote, active-daemon, and local precedence, then +receive transport metadata without credentials +([contract](https://github.com/kenn-io/kata/blob/c31c2a875e18115a74557ef59dc94ad600e5a521/docs/reference/daemon-discovery.md#L19-L109)). +That command is a good consumer pattern. Its application-specific precedence +and output schema do not belong in Kit. + +[Kata PR #281](https://github.com/kenn-io/kata/pull/281), merged as +[`e6356f8a`](https://github.com/kenn-io/kata/commit/e6356f8a2a18bf56f6e315c213de55432a964e6e), +added opt-in idle shutdown for auto-started daemons. It distinguishes +foreground activity that renews residency from finite drain work that merely +blocks exit. All shutdown sources enter one coordinator, close admission, and +share one absolute deadline. An explicit `kata daemon start` replaces an +idle-enabled auto-start process because explicit start promises a resident +daemon +([design](https://github.com/kenn-io/kata/blob/e6356f8a2a18bf56f6e315c213de55432a964e6e/docs/design/autostart-idle-shutdown.md#L14-L139), +[controller](https://github.com/kenn-io/kata/blob/e6356f8a2a18bf56f6e315c213de55432a964e6e/internal/daemon/idle_controller.go#L66-L366)). +This improves intentional residency and shutdown ordering. It does not make a +daemon survive sandbox teardown, and the policy is too application-specific to +move wholesale into Kit. + +### RoboRev + +[RoboRev PR #1021](https://github.com/kenn-io/roborev/pull/1021), merged as +[`97364377`](https://github.com/kenn-io/roborev/commit/97364377f1117261911b4a8b18386b0c6c189a8a), +fixed the same false-absence path reported in +[issue #1006](https://github.com/kenn-io/roborev/issues/1006). It introduced +`ErrDaemonAccessDenied`, recognizes wrapped `os.ErrPermission`, `EACCES`, and +`EPERM`, and keeps that result distinct from `os.ErrNotExist` +([classification and discovery](https://github.com/kenn-io/roborev/blob/97364377f1117261911b4a8b18386b0c6c189a8a/internal/daemon/runtime.go#L293-L346)). +Ensure, cold start, version checks, zombie cleanup, and server startup all stop +when discovery is indeterminate. Tests verify that none of cleanup, restart, +or spawn runs after an access-denied probe +([lifecycle tests](https://github.com/kenn-io/roborev/blob/97364377f1117261911b4a8b18386b0c6c189a8a/cmd/roborev/daemon_lifecycle_test.go#L190-L335), +[cleanup test](https://github.com/kenn-io/roborev/blob/97364377f1117261911b4a8b18386b0c6c189a8a/internal/daemon/runtime_test.go#L615-L637)). + +The PR also lets a TCP daemon publish one best-effort Unix-socket alternate. +The socket lives in a private `0700` runtime directory, has mode `0600`, and +uses a data-directory hash in its service name so separate RoboRev data +directories do not collide +([listener](https://github.com/kenn-io/roborev/blob/97364377f1117261911b4a8b18386b0c6c189a8a/internal/daemon/auxiliary_listener_unix.go#L17-L66)). +Discovery probes the primary followed by the alternate and requires the ping +PID to match the runtime PID. The daemon publishes the alternate only after it +accepts requests; failure of the alternate leaves the primary service intact +([server startup](https://github.com/kenn-io/roborev/blob/97364377f1117261911b4a8b18386b0c6c189a8a/internal/daemon/server.go#L214-L320), +[tests](https://github.com/kenn-io/roborev/blob/97364377f1117261911b4a8b18386b0c6c189a8a/internal/daemon/server_test.go#L481-L533)). +Explicit Unix configuration, systemd socket activation, and Windows remain +single-transport cases. + +The alternate is a useful availability pattern, but it is not default-sandbox +compatibility. Codex and Claude may deny both loopback and Unix sockets. If +every endpoint is denied, RoboRev prints a sandbox-specific recovery message +and its Codex and Claude skills request each harness's native escalation while +forbidding a restart +([status](https://github.com/kenn-io/roborev/blob/97364377f1117261911b4a8b18386b0c6c189a8a/cmd/roborev/status.go#L42-L77), +[Codex wording](https://github.com/kenn-io/roborev/blob/97364377f1117261911b4a8b18386b0c6c189a8a/internal/skills/codex/roborev-review/SKILL.md#L23-L28), +[Claude wording](https://github.com/kenn-io/roborev/blob/97364377f1117261911b4a8b18386b0c6c189a8a/internal/skills/claude/roborev-review/SKILL.md#L24-L29)). + +[RoboRev PR #1036](https://github.com/kenn-io/roborev/pull/1036), merged as +[`94de16d9`](https://github.com/kenn-io/roborev/commit/94de16d907037469639fa6ca4bbb8eb7e831aeea), +tightened stop and replacement behavior. The daemon blocks new claims, drains +accepted work, and removes discovery metadata last. Lifecycle clients request +shutdown, then wait for the exact recorded PID to exit rather than using +endpoint failure as proof of exit. They do not force-kill an unresponsive live +process, and they remove only that process's runtime record because a service +manager may already have reused the endpoint +([client lifecycle](https://github.com/kenn-io/roborev/blob/94de16d907037469639fa6ca4bbb8eb7e831aeea/internal/daemon/runtime.go#L467-L579), +[server ordering](https://github.com/kenn-io/roborev/blob/94de16d907037469639fa6ca4bbb8eb7e831aeea/internal/daemon/server.go#L561-L637)). +The review-drain policy belongs to RoboRev, but exact-process exit and +ownership-aware cleanup are reusable daemon invariants. + +[RoboRev PR #1059](https://github.com/kenn-io/roborev/pull/1059), merged as +[`850a0766`](https://github.com/kenn-io/roborev/commit/850a0766ff7e896c6acba98018b06ccc62e8c25b), +removed a second Agent Hook daemon and moved those routes onto the regular +daemon. The practical lesson is simple: when data and work already share one +owner, another daemon adds discovery, replacement, and persistence failure +modes without creating a useful isolation boundary. RoboRev deliberately did +not add a legacy takeover path; old auxiliary daemons must be stopped with the +old release. + +### What should move into Kit next + +The evidence is sufficient for one focused improvement: Kit discovery should +return a typed live-but-unreachable result. Current `Discover` discards every +probe error and reports no match +([source](../../daemon/probe.go)). `Manager.Ensure` already returns immediately +when `Find` returns an error, so preserving the error there prevents `Start` +without changing manager control flow +([source](../../daemon/manager.go)). The reusable contract is: + +1. Reject a definite process-identity mismatch. Treat an identity that cannot + be checked as indeterminate rather than using it to authorize cleanup or + replacement. +2. Continue scanning after an unreachable live record so another usable record + can win. +3. If none wins, return a typed error carrying the runtime record, endpoint, + and wrapped probe error. Do not collapse it into absence or a boolean + `running` value. +4. Never clean up, signal, restart, or auto-start solely because a probe was + denied. + +This behavior is intentionally opt-in through `RequirePIDAlive`. Without a +live-process check, a failed probe cannot distinguish an inaccessible daemon +from a stale runtime record. Keeping the false zero value also preserves the +existing behavior for callers that do not use process-aware discovery. + +| PID checks | Recorded identity | Probe result | Discovery result when no later candidate wins | +| --- | --- | --- | --- | +| Disabled | Any | Failure | Definite absence, preserving the prior behavior | +| Enabled | Definite mismatch or dead PID | Not attempted | Definite absence | +| Enabled | Match or unknown | Failure | `UnreachableError` wrapping the probe failure | +| Enabled | Match or unknown | Success and accepted | Candidate returned | +| Enabled | Match or unknown | Success but rejected by `Accept` | Continue scanning, then definite absence | + +Context cancellation takes precedence over a retained probe failure, and a +later reachable candidate takes precedence over every earlier failure. A probe +failure includes transport denial, a non-success HTTP response, malformed ping +data, service mismatch, and proof-of-possession failure. Applications should +use the wrapped error, not only `ErrDaemonUnreachable`, when writing recovery +guidance. + +Ordered endpoint candidates are a plausible second Kit improvement. RoboRev +currently encodes its alternate in `RuntimeRecord.Metadata`, then reimplements +candidate parsing and probing. A structured candidate list would remove that +application convention and could later carry a FIFO endpoint. It should not +silently retry TCP after Unix failure, since default sandboxes may deny both. + +`RuntimeStore.CheckWritable` should remain the pre-spawn filesystem check. +Application wording stays with callers. Kata's idle controller and RoboRev's +review drain should also remain caller-owned until another reusable package +contract emerges. + +One smaller lifecycle gap remains in Kit. `StartDetached` launches a goroutine +that discards `cmd.Wait()` errors +([source](../../daemon/start.go)). A bounded child-exit signal during readiness +could preserve bind `EPERM`, configuration failures, and child-only permission +errors that the advisory write check cannot catch. The API shape needs care +because a successfully detached daemon is expected to outlive the starter. + +### Remaining unknowns + +- Neither Kata nor RoboRev added a FIFO or file-backed request transport. Both + still use Unix sockets or TCP. +- Neither project runs an end-to-end test inside the real default Codex and + Claude sandboxes. +- The inspected changes do not prove that a detached daemon survives sandbox + teardown or remains discoverable in a later sandbox invocation. +- Vite Task proves related-process IPC within one sandboxed command. It does + not settle cross-command daemon persistence. +- A write preflight does not prove child permissions or endpoint binding. +- RoboRev's alternate socket improves the chance of access only when the + sandbox permits one of the two socket types. diff --git a/git/cmd/gitcmd_test.go b/git/cmd/gitcmd_test.go index d76cae3..98bac7e 100644 --- a/git/cmd/gitcmd_test.go +++ b/git/cmd/gitcmd_test.go @@ -14,6 +14,8 @@ import ( Assert "github.com/stretchr/testify/assert" Require "github.com/stretchr/testify/require" + + gitenv "go.kenn.io/kit/git/env" ) func TestRunnerCommandUsesDefensiveEnvironment(t *testing.T) { @@ -186,7 +188,7 @@ func safeDirectoryTestEnv(t *testing.T, globalConfig string) []string { t.Helper() emptySystemConfig := filepath.Join(t.TempDir(), "system-gitconfig") Require.NoError(t, os.WriteFile(emptySystemConfig, nil, 0o600)) - return append(os.Environ(), + return append(gitenv.StripAll(os.Environ()), "GIT_CONFIG_GLOBAL="+globalConfig, "GIT_CONFIG_SYSTEM="+emptySystemConfig, "GIT_CONFIG_NOSYSTEM=0", @@ -272,6 +274,9 @@ func TestReadSafeDirectoriesConditionalInclude(t *testing.T) { // the command targets, not for the calling process's working directory. dir := t.TempDir() repo := filepath.Join(dir, "repo") + // Git exports repository-local variables to hooks. The fixture environment + // must discard them before it starts or probes its own repository. + t.Setenv("GIT_DIR", filepath.Join(dir, "hook-repository.git")) require.NoError(os.Mkdir(repo, 0o755)) // git matches gitdir patterns against resolved paths, so the pattern must // use the symlink-free form (t.TempDir is a symlink on macOS). diff --git a/git/managed/lifecycle_mr_test.go b/git/managed/lifecycle_mr_test.go index 28d515a..c10f2a0 100644 --- a/git/managed/lifecycle_mr_test.go +++ b/git/managed/lifecycle_mr_test.go @@ -15,13 +15,13 @@ import ( Require "github.com/stretchr/testify/require" gitcmd "go.kenn.io/kit/git/cmd" - gitenv "go.kenn.io/kit/git/env" ) -func lifecycleGitCommand(dir string, args ...string) *exec.Cmd { +func lifecycleGitCommand(t *testing.T, dir string, args ...string) *exec.Cmd { + t.Helper() cmd := exec.Command("git", args...) cmd.Dir = dir - cmd.Env = gitenv.StripAll(os.Environ()) + cmd.Env = lifecycleGitEnv(t) return cmd } @@ -51,7 +51,7 @@ func initOriginAndClone(t *testing.T) (string, string) { func worktreeConfig(t *testing.T, dir, key string) string { t.Helper() - cmd := lifecycleGitCommand(dir, "config", "--get", key) + cmd := lifecycleGitCommand(t, dir, "config", "--get", key) out, err := cmd.CombinedOutput() if err != nil { return "" @@ -61,7 +61,7 @@ func worktreeConfig(t *testing.T, dir, key string) string { func worktreeOnlyConfig(t *testing.T, dir, key string) string { t.Helper() - cmd := lifecycleGitCommand(dir, "config", "--worktree", "--get", key) + cmd := lifecycleGitCommand(t, dir, "config", "--worktree", "--get", key) out, err := cmd.CombinedOutput() if err != nil { return "" @@ -87,6 +87,7 @@ func TestCreateWorktreeFromMergeRequestSameRepo(t *testing.T) { dest := filepath.Join(t.TempDir(), "wt") result, err := CreateWorktreeFromMergeRequest( context.Background(), MergeRequestWorktreeOptions{ + Runner: lifecycleTestRunner(t), ProjectRoot: clone, Branch: "pr-42", Path: dest, @@ -121,6 +122,7 @@ func TestCreateWorktreeFromMergeRequestUsesExplicitProjectRemote(t *testing.T) { result, err := CreateWorktreeFromMergeRequest( t.Context(), MergeRequestWorktreeOptions{ + Runner: lifecycleTestRunner(t), ProjectRoot: clone, ProjectRemote: "upstream", Branch: "pr-explicit-remote", @@ -177,6 +179,7 @@ func TestCreateWorktreeFromMergeRequestMatchesEquivalentLocalRepositories(t *tes result, err := CreateWorktreeFromMergeRequest( t.Context(), MergeRequestWorktreeOptions{ + Runner: lifecycleTestRunner(t), ProjectRoot: clone, Branch: "pr-alternate", Path: filepath.Join(t.TempDir(), "worktree"), @@ -208,6 +211,7 @@ func TestCreateWorktreeFromMergeRequestCanonicalizesRelativeProjectIdentity(t *t result, err := CreateWorktreeFromMergeRequest( t.Context(), MergeRequestWorktreeOptions{ + Runner: lifecycleTestRunner(t), ProjectRoot: clone, Branch: "pr-relative-project", Path: filepath.Join(t.TempDir(), "worktree"), @@ -237,6 +241,7 @@ func TestCreateWorktreeFromMergeRequestGitLabRef(t *testing.T) { dest := filepath.Join(t.TempDir(), "wt") _, err := CreateWorktreeFromMergeRequest( context.Background(), MergeRequestWorktreeOptions{ + Runner: lifecycleTestRunner(t), ProjectRoot: clone, Branch: "mr-5", Path: dest, @@ -268,6 +273,7 @@ func TestCreateWorktreeFromMergeRequestPullRefFallback(t *testing.T) { dest := filepath.Join(t.TempDir(), "wt") _, err := CreateWorktreeFromMergeRequest( context.Background(), MergeRequestWorktreeOptions{ + Runner: lifecycleTestRunner(t), ProjectRoot: clone, Branch: "pr-7", Path: dest, @@ -296,6 +302,7 @@ func TestCreateWorktreeFromMergeRequestRejectsLeftoverBranch(t *testing.T) { _, err := CreateWorktreeFromMergeRequest( t.Context(), MergeRequestWorktreeOptions{ + Runner: lifecycleTestRunner(t), ProjectRoot: clone, Branch: "pr-leftover", Path: dest, @@ -325,6 +332,7 @@ func TestCreateWorktreeFromMergeRequestPreservesCancellation(t *testing.T) { _, err := CreateWorktreeFromMergeRequest( ctx, MergeRequestWorktreeOptions{ + Runner: lifecycleTestRunner(t), ProjectRoot: clone, Branch: "pr-cancel-" + phase, Path: filepath.Join(t.TempDir(), "wt"), @@ -376,6 +384,7 @@ func TestCreateWorktreeFromMergeRequestRejectsChangedHead(t *testing.T) { dest := filepath.Join(t.TempDir(), "wt") result, err := CreateWorktreeFromMergeRequest( context.Background(), MergeRequestWorktreeOptions{ + Runner: lifecycleTestRunner(t), ProjectRoot: clone, Branch: "pr-8", Path: dest, @@ -413,6 +422,7 @@ func TestCreateWorktreeFromMergeRequestRejectsIncompatibleCommonConfig(t *testin dest := filepath.Join(t.TempDir(), "wt") _, err := CreateWorktreeFromMergeRequest( t.Context(), MergeRequestWorktreeOptions{ + Runner: lifecycleTestRunner(t), ProjectRoot: clone, Branch: "pr-config-check", Path: dest, Number: 26, HeadBranch: "config-check", HeadRepoCloneURL: origin, @@ -484,6 +494,7 @@ func TestCreateWorktreeFromMergeRequestIsolatesUntrustedTreeGitPrograms(t *testi result, err := CreateWorktreeFromMergeRequest( t.Context(), MergeRequestWorktreeOptions{ + Runner: lifecycleTestRunner(t), ProjectRoot: clone, Branch: "pr-untrusted", Path: dest, @@ -568,6 +579,7 @@ func TestCreateWorktreeFromMergeRequestDisablesLaterSubmoduleFetches( dest := filepath.Join(t.TempDir(), "wt") _, err := CreateWorktreeFromMergeRequest( t.Context(), MergeRequestWorktreeOptions{ + Runner: lifecycleTestRunner(t), ProjectRoot: clone, Branch: "pr-submodule-fetch", Path: dest, Number: 29, HeadBranch: "submodule-tree", HeadRepoCloneURL: origin, ProjectRepoIdentity: identityOfCloneURL(origin), @@ -640,6 +652,7 @@ func TestCreateWorktreeFromMergeRequestNeutralizesCaseDistinctAttributeDrivers( dest := filepath.Join(t.TempDir(), "wt") _, err := CreateWorktreeFromMergeRequest( t.Context(), MergeRequestWorktreeOptions{ + Runner: lifecycleTestRunner(t), ProjectRoot: clone, Branch: "pr-drivers", Path: dest, @@ -692,6 +705,7 @@ func TestCreateWorktreeFromMergeRequestDoesNotPATHSearchDriverHelpers( dest := filepath.Join(t.TempDir(), "wt") _, err := CreateWorktreeFromMergeRequest( t.Context(), MergeRequestWorktreeOptions{ + Runner: lifecycleTestRunner(t), ProjectRoot: clone, Branch: "pr-path-driver", Path: dest, Number: 28, HeadBranch: "path-driver", HeadRepoCloneURL: origin, ProjectRepoIdentity: identityOfCloneURL(origin), @@ -702,7 +716,7 @@ func TestCreateWorktreeFromMergeRequestDoesNotPATHSearchDriverHelpers( filepath.Join(dest, "payload"), []byte("changed\n"), 0o644, )) - cmd := lifecycleGitCommand(dest, "diff", "--", "payload") + cmd := lifecycleGitCommand(t, dest, "diff", "--", "payload") cmd.Env = append(cmd.Env, "PATH="+dest+":"+os.Getenv("PATH")) out, err := cmd.CombinedOutput() @@ -754,8 +768,11 @@ func TestMergeRequestRollbackRetainsIsolatedRunner(t *testing.T) { ), 0o600)) runner := gitcmd.New() runner.NullGlobalConfig = false + // StripEnv would discard the GIT_CONFIG_GLOBAL override below and let git + // fall back to the host's real global config; Env is already sanitized. + runner.StripEnv = false runner.Env = append( - gitenv.StripAll(os.Environ()), + isolatedLifecycleBaseEnv(t), "GIT_CONFIG_GLOBAL="+globalConfig, ) @@ -828,7 +845,7 @@ func TestCreateWorktreeFromMergeRequestInspectsSelectedConfigFiles(t *testing.T) runner := gitcmd.New() runner.NullGlobalConfig = false runner.NoSystemConfig = false - runner.Env = append(gitenv.StripAll(os.Environ()), + runner.Env = append(isolatedLifecycleBaseEnv(t), "HOME="+configDir, "GIT_CONFIG_GLOBAL="+globalConfig, "GIT_CONFIG_SYSTEM="+systemConfig, @@ -863,9 +880,9 @@ func TestCreateWorktreeFromMergeRequestRejectsInheritedCommandScopeConfig( }{ { name: "GIT_CONFIG_COUNT", - runner: func(_ *testing.T) gitcmd.Runner { + runner: func(t *testing.T) gitcmd.Runner { runner := gitcmd.New() - runner.Env = append(gitenv.StripAll(os.Environ()), + runner.Env = append(isolatedLifecycleBaseEnv(t), "GIT_CONFIG_COUNT=1", "GIT_CONFIG_KEY_0=filter.inherited.smudge", "GIT_CONFIG_VALUE_0=false", @@ -875,9 +892,9 @@ func TestCreateWorktreeFromMergeRequestRejectsInheritedCommandScopeConfig( }, { name: "GIT_CONFIG_PARAMETERS", - runner: func(_ *testing.T) gitcmd.Runner { + runner: func(t *testing.T) gitcmd.Runner { runner := gitcmd.New() - runner.Env = append(gitenv.StripAll(os.Environ()), + runner.Env = append(isolatedLifecycleBaseEnv(t), "GIT_CONFIG_PARAMETERS='filter.inherited.smudge'='false'", ) return runner @@ -885,10 +902,14 @@ func TestCreateWorktreeFromMergeRequestRejectsInheritedCommandScopeConfig( }, { name: "Runner Config", - runner: func(_ *testing.T) gitcmd.Runner { - return gitcmd.New().WithConfig( - "filter.inherited.smudge", "false", - ) + runner: func(t *testing.T) gitcmd.Runner { + return gitcmd.Runner{ + Env: isolatedLifecycleBaseEnv(t), + StripEnv: true, + Config: []gitcmd.Config{{ + Key: "filter.inherited.smudge", Value: "false", + }}, + } }, }, { @@ -901,7 +922,7 @@ func TestCreateWorktreeFromMergeRequestRejectsInheritedCommandScopeConfig( 0o600, )) runner := gitcmd.New() - runner.Env = append(gitenv.StripAll(os.Environ()), + runner.Env = append(isolatedLifecycleBaseEnv(t), "GIT_CONFIG_COUNT=1", "GIT_CONFIG_KEY_0=include.path", "GIT_CONFIG_VALUE_0="+config, @@ -959,6 +980,7 @@ func TestCreateWorktreeFromMergeRequestRejectsConfiguredHooks(t *testing.T) { dest := filepath.Join(t.TempDir(), "wt") _, err := CreateWorktreeFromMergeRequest( t.Context(), MergeRequestWorktreeOptions{ + Runner: lifecycleTestRunner(t), ProjectRoot: clone, Branch: "pr-configured-hook", Path: dest, Number: 34, HeadBranch: "configured-hook", HeadRepoCloneURL: origin, ProjectRepoIdentity: identityOfCloneURL(origin), @@ -992,7 +1014,7 @@ func TestCreateWorktreeFromMergeRequestRejectsCommandScopeWorktree(t *testing.T) external := t.TempDir() marker := filepath.Join(external, "keep") require.NoError(os.WriteFile(marker, []byte("preserve"), 0o600)) - runner := gitcmd.New().WithConfig("core.worktree", external) + runner := lifecycleTestRunner(t).WithConfig("core.worktree", external) dest := filepath.Join(t.TempDir(), "wt") _, err := CreateWorktreeFromMergeRequest( @@ -1081,7 +1103,7 @@ func TestCreateWorktreeFromMergeRequestRejectsConditionalCommandScopeConfig( []byte("[filter \"conditional\"]\n\tsmudge = false\n"), 0o600, )) runner := gitcmd.New() - runner.Env = append(gitenv.StripAll(os.Environ()), + runner.Env = append(isolatedLifecycleBaseEnv(t), "GIT_CONFIG_COUNT=1", "GIT_CONFIG_KEY_0=includeIf.onbranch:pr-command-conditional.path", "GIT_CONFIG_VALUE_0="+config, @@ -1127,8 +1149,11 @@ func TestCreateWorktreeFromMergeRequestRejectsConfigFromMaterializedTree( runner := gitcmd.New() runner.NullGlobalConfig = false + // StripEnv would discard the GIT_CONFIG_GLOBAL override below and let git + // fall back to the host's real global config; Env is already sanitized. + runner.StripEnv = false runner.Env = append( - gitenv.StripAll(os.Environ()), + isolatedLifecycleBaseEnv(t), "GIT_CONFIG_GLOBAL=.gitconfig", ) dest := filepath.Join(t.TempDir(), "wt") @@ -1178,8 +1203,11 @@ func TestCreateWorktreeFromMergeRequestRejectsSymlinkedConfigFromTree( runner := gitcmd.New() runner.NullGlobalConfig = false + // StripEnv would discard the GIT_CONFIG_GLOBAL override below and let git + // fall back to the host's real global config; Env is already sanitized. + runner.StripEnv = false runner.Env = append( - gitenv.StripAll(os.Environ()), + isolatedLifecycleBaseEnv(t), "GIT_CONFIG_GLOBAL=.gitconfig", ) dest := filepath.Join(t.TempDir(), "wt") @@ -1213,7 +1241,7 @@ func TestCreateWorktreeFromMergeRequestRejectsInheritedRelativeInclude( lifecycleGit(t, origin, "checkout", "-q", "main") runner := gitcmd.New() - runner.Env = append(gitenv.StripAll(os.Environ()), + runner.Env = append(isolatedLifecycleBaseEnv(t), "GIT_CONFIG_COUNT=1", "GIT_CONFIG_KEY_0=include.path", "GIT_CONFIG_VALUE_0=.gitconfig", @@ -1296,6 +1324,7 @@ func TestCreateWorktreeFromMergeRequestInspectsConditionalIncludes(t *testing.T) dest := filepath.Join(t.TempDir(), "wt") _, err := CreateWorktreeFromMergeRequest( t.Context(), MergeRequestWorktreeOptions{ + Runner: lifecycleTestRunner(t), ProjectRoot: clone, Branch: "pr-conditional", Path: dest, Number: 24, HeadBranch: "conditional-config", HeadRepoCloneURL: origin, ProjectRepoIdentity: identityOfCloneURL(origin), @@ -1336,6 +1365,7 @@ func TestCreateWorktreeFromMergeRequestReportsCleanupFailure(t *testing.T) { _, err := CreateWorktreeFromMergeRequest( t.Context(), MergeRequestWorktreeOptions{ + Runner: lifecycleTestRunner(t), ProjectRoot: clone, Branch: "pr-cleanup-failure", Path: filepath.Join(t.TempDir(), "wt"), Number: 25, HeadBranch: "cleanup-failure", @@ -1381,6 +1411,7 @@ func TestCreateWorktreeFromMergeRequestFork(t *testing.T) { var fetches [][]string _, err := CreateWorktreeFromMergeRequest( context.Background(), MergeRequestWorktreeOptions{ + Runner: lifecycleTestRunner(t), ProjectRoot: clone, Branch: "pr-9", Path: dest, @@ -1410,7 +1441,7 @@ func TestCreateWorktreeFromMergeRequestFork(t *testing.T) { require.NoError(readErr) assert.Equal(fetchHeadSentinel, string(fetchHead)) tagCommand := lifecycleGitCommand( - clone, "show-ref", "--verify", "--quiet", "refs/tags/contributor-tag", + t, clone, "show-ref", "--verify", "--quiet", "refs/tags/contributor-tag", ) assert.Error(tagCommand.Run()) assert.Equal(headSHA, lifecycleGit(t, dest, "rev-parse", "HEAD")) @@ -1440,6 +1471,7 @@ func TestCreateWorktreeFromMergeRequestCanonicalizesRelativeForkURL(t *testing.T dest := filepath.Join(t.TempDir(), "wt") _, err := CreateWorktreeFromMergeRequest( context.Background(), MergeRequestWorktreeOptions{ + Runner: lifecycleTestRunner(t), ProjectRoot: clone, Branch: "pr-10", Path: dest, @@ -1584,6 +1616,7 @@ func TestCreateWorktreeFromMergeRequestTrackingFetchFailureIsNonFatal(t *testing dest := filepath.Join(t.TempDir(), "wt") _, err := CreateWorktreeFromMergeRequest( context.Background(), MergeRequestWorktreeOptions{ + Runner: lifecycleTestRunner(t), ProjectRoot: clone, Branch: "pr-11", Path: dest, @@ -1617,6 +1650,7 @@ func TestCreateWorktreeFromMergeRequestPropagatesTrackingRunnerFailure( _, err := CreateWorktreeFromMergeRequest( t.Context(), MergeRequestWorktreeOptions{ + Runner: lifecycleTestRunner(t), ProjectRoot: clone, Branch: "pr-tracking-failure", Path: filepath.Join(t.TempDir(), "wt"), @@ -1660,6 +1694,7 @@ func TestCreateWorktreeFromMergeRequestHookFailureRollsBack(t *testing.T) { dest := filepath.Join(t.TempDir(), "wt") _, err := CreateWorktreeFromMergeRequest( context.Background(), MergeRequestWorktreeOptions{ + Runner: lifecycleTestRunner(t), ProjectRoot: clone, Branch: "pr-42", Path: dest, @@ -1699,6 +1734,7 @@ func TestCreateWorktreeFromMergeRequestRejectsHookFromDestination(t *testing.T) hookRan := false _, err = CreateWorktreeFromMergeRequest( t.Context(), MergeRequestWorktreeOptions{ + Runner: lifecycleTestRunner(t), ProjectRoot: clone, Branch: "pr-hook", Path: dest, diff --git a/git/managed/lifecycle_test.go b/git/managed/lifecycle_test.go index c2a5e71..fa9b066 100644 --- a/git/managed/lifecycle_test.go +++ b/git/managed/lifecycle_test.go @@ -21,12 +21,44 @@ func lifecycleGit(t *testing.T, dir string, args ...string) string { t.Helper() cmd := exec.Command("git", args...) cmd.Dir = dir - cmd.Env = gitenv.StripAll(os.Environ()) + cmd.Env = lifecycleGitEnv(t) out, err := cmd.CombinedOutput() Require.NoError(t, err, "git %v: %s", args, out) return strings.TrimSpace(string(out)) } +func isolateLifecycleGitConfig(t *testing.T) { + t.Helper() + globalConfig := filepath.Join(t.TempDir(), "global.gitconfig") + Require.NoError(t, os.WriteFile(globalConfig, nil, 0o600)) + t.Setenv("GIT_CONFIG_GLOBAL", globalConfig) + t.Setenv("GIT_CONFIG_NOSYSTEM", "1") +} + +func lifecycleGitEnv(t *testing.T) []string { + t.Helper() + globalConfig := os.Getenv("GIT_CONFIG_GLOBAL") + Require.NotEmpty(t, globalConfig, "Git fixture config was not isolated") + return append( + isolatedLifecycleBaseEnv(t), + "GIT_CONFIG_GLOBAL="+globalConfig, + "GIT_CONFIG_NOSYSTEM=1", + ) +} + +func isolatedLifecycleBaseEnv(t *testing.T) []string { + t.Helper() + return append( + gitenv.StripAll(os.Environ()), + "XDG_CONFIG_HOME="+t.TempDir(), + ) +} + +func lifecycleTestRunner(t *testing.T) gitcmd.Runner { + t.Helper() + return gitcmd.Runner{Env: lifecycleGitEnv(t)} +} + // initLifecycleRepo creates a git repository with one commit on a stable // default branch named "main" so tests do not depend on the host git's // init.defaultBranch setting. @@ -35,6 +67,7 @@ func initLifecycleRepo(t *testing.T) string { if _, err := exec.LookPath("git"); err != nil { t.Skip("git not available") } + isolateLifecycleGitConfig(t) dir := filepath.Join(t.TempDir(), "repo") Require.NoError(t, os.MkdirAll(dir, 0o755)) lifecycleGit(t, dir, "init", "-q", "-b", "main") @@ -51,7 +84,7 @@ func branchExistsInRepo(t *testing.T, repo, branch string) bool { "git", "show-ref", "--verify", "--quiet", "refs/heads/"+branch, ) cmd.Dir = repo - cmd.Env = gitenv.StripAll(os.Environ()) + cmd.Env = lifecycleGitEnv(t) return cmd.Run() == nil }