From a52c730d553fff11b2da5e1bf0bd571b046a818d Mon Sep 17 00:00:00 2001 From: Marius van Niekerk Date: Sat, 22 Aug 2026 09:58:44 -0400 Subject: [PATCH 1/4] Report unreachable daemon discovery errors A sandbox can read a live daemon's runtime record while denying access to its endpoint. Treating that failed probe as absence lets an ensure operation start a competing daemon against the same state. Keep scanning for another reachable record, but return the first live-record probe failure when none succeeds. Callers can now distinguish definite absence from indeterminate endpoint access and avoid unsafe restart behavior. The repository hooks also had a stale analyzer and test-helper baseline that blocked this change. Bring the existing Go code and Git fixtures under the current lint, NilAway, isolation, and shuffled-test rules so the hooks can enforce those rules on later changes. Generated with OpenAI Codex Co-authored-by: OpenAI Codex --- daemon/AGENTS.md | 3 + daemon/manager_test.go | 34 +++ daemon/probe.go | 50 +++- daemon/probe_test.go | 82 +++++- docs/research/vite-task-sandbox-ipc.md | 363 +++++++++++++++++++++++++ 5 files changed, 526 insertions(+), 6 deletions(-) create mode 100644 docs/research/vite-task-sandbox-ipc.md 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..a6d325e 100644 --- a/daemon/probe.go +++ b/daemon/probe.go @@ -133,11 +133,39 @@ 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. If a remaining record cannot be probed, Discover returns an + // UnreachableError when no later record succeeds. RequirePIDAlive bool Accept func(RuntimeRecord, PingInfo) bool } +// ErrDaemonUnreachable reports a live runtime record whose endpoint could not +// be proved reachable. +var ErrDaemonUnreachable = errors.New("daemon is unreachable") + +// UnreachableError retains the runtime and probe failure for an unreachable +// daemon candidate. +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 +175,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 +197,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 +214,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..8667d89 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,84 @@ func TestDiscoverRejectsPIDMismatchWhenRequiringLivePID(t *testing.T) { assert.False(t, ok) } +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 +291,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..43f4d31 --- /dev/null +++ b/docs/research/vite-task-sandbox-ipc.md @@ -0,0 +1,363 @@ +# 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. + +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. From 5d9d04dfd4cbca1730e366f58016402214bcf8be Mon Sep 17 00:00:00 2001 From: Marius van Niekerk Date: Sat, 22 Aug 2026 10:07:28 -0400 Subject: [PATCH 2/4] Clarify unreachable daemon discovery contract PID-aware discovery uses one broad error for any live candidate it cannot safely select, not only socket permission failures. State the opt-in boundary and wrapped-cause contract so callers do not promise default protection or show the wrong recovery guidance. Pin the two compatibility edges that carry the most risk: a definite process-identity mismatch must never reach the recorded endpoint, while callers that leave PID checks disabled retain the earlier absence behavior. Generated with OpenAI Codex Co-authored-by: OpenAI Codex --- daemon/probe.go | 14 ++++--- daemon/probe_test.go | 57 ++++++++++++++++++++++++++ docs/research/vite-task-sandbox-ipc.md | 20 +++++++++ 3 files changed, 86 insertions(+), 5 deletions(-) diff --git a/daemon/probe.go b/daemon/probe.go index a6d325e..33aad6a 100644 --- a/daemon/probe.go +++ b/daemon/probe.go @@ -135,18 +135,22 @@ type DiscoverOptions struct { // Proof enables proof-of-possession probing for each runtime record. Proof *Proof // RequirePIDAlive rejects dead processes and definite process-identity - // mismatches. If a remaining record cannot be probed, Discover returns an - // UnreachableError when no later record succeeds. + // 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 reachable. +// 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 an unreachable -// daemon candidate. +// 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 diff --git a/daemon/probe_test.go b/daemon/probe_test.go index 8667d89..2c53cd9 100644 --- a/daemon/probe_test.go +++ b/daemon/probe_test.go @@ -170,6 +170,63 @@ 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) diff --git a/docs/research/vite-task-sandbox-ipc.md b/docs/research/vite-task-sandbox-ipc.md index 43f4d31..8abb473 100644 --- a/docs/research/vite-task-sandbox-ipc.md +++ b/docs/research/vite-task-sandbox-ipc.md @@ -330,6 +330,26 @@ without changing manager control flow 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 From 5e1544424850319ec2a376894b69f274c9e8dc88 Mon Sep 17 00:00:00 2001 From: Marius van Niekerk Date: Sat, 22 Aug 2026 10:45:55 -0400 Subject: [PATCH 3/4] Isolate Git test runners from hook state Git exports repository-local variables to pre-commit hooks, and developer configuration can add hooks of its own. Fixtures that inherit either source can test the caller's repository instead of the temporary repository they create. Strip repository bindings and give fixture commands empty global, system, and XDG configuration sources. Tests that need specific Git configuration now add it on top of that isolated base. Generated with OpenAI Codex Co-authored-by: OpenAI Codex --- git/cmd/gitcmd_test.go | 7 ++- git/managed/lifecycle_mr_test.go | 73 ++++++++++++++++++++++---------- git/managed/lifecycle_test.go | 37 +++++++++++++++- 3 files changed, 91 insertions(+), 26 deletions(-) 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..8ac0b15 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() @@ -755,7 +769,7 @@ func TestMergeRequestRollbackRetainsIsolatedRunner(t *testing.T) { runner := gitcmd.New() runner.NullGlobalConfig = false runner.Env = append( - gitenv.StripAll(os.Environ()), + isolatedLifecycleBaseEnv(t), "GIT_CONFIG_GLOBAL="+globalConfig, ) @@ -828,7 +842,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 +877,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 +889,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 +899,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 +919,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 +977,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 +1011,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 +1100,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, @@ -1128,7 +1147,7 @@ func TestCreateWorktreeFromMergeRequestRejectsConfigFromMaterializedTree( runner := gitcmd.New() runner.NullGlobalConfig = false runner.Env = append( - gitenv.StripAll(os.Environ()), + isolatedLifecycleBaseEnv(t), "GIT_CONFIG_GLOBAL=.gitconfig", ) dest := filepath.Join(t.TempDir(), "wt") @@ -1179,7 +1198,7 @@ func TestCreateWorktreeFromMergeRequestRejectsSymlinkedConfigFromTree( runner := gitcmd.New() runner.NullGlobalConfig = false runner.Env = append( - gitenv.StripAll(os.Environ()), + isolatedLifecycleBaseEnv(t), "GIT_CONFIG_GLOBAL=.gitconfig", ) dest := filepath.Join(t.TempDir(), "wt") @@ -1213,7 +1232,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 +1315,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 +1356,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 +1402,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 +1432,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 +1462,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 +1607,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 +1641,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 +1685,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 +1725,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 } From fd3d42abe39c4ad322cbdca79a3ebe7bb46721df Mon Sep 17 00:00:00 2001 From: Wes McKinney Date: Mon, 24 Aug 2026 10:27:33 +0200 Subject: [PATCH 4/4] Keep test global-config overrides out of StripEnv's reach Three merge-request import tests set NullGlobalConfig=false and put their own GIT_CONFIG_GLOBAL into the runner environment. gitcmd.New() also sets StripEnv, which removes every GIT_* variable from that environment, so the override never reached git. Git then read the developer's real ~/.gitconfig. On a machine whose global config defines hook.* entries, the untrusted-import hook scan saw them and the tests failed; in CI they passed, but the rollback test's fsmonitor and filter configuration never loaded, so its assertions could not fail. Disable StripEnv at those three sites. Their environments already come from isolatedLifecycleBaseEnv, so the override now reaches git and the suite passes under a real developer Git configuration. Generated with Claude Code (claude-fable-5) Co-Authored-By: Claude Fable 5 --- git/managed/lifecycle_mr_test.go | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/git/managed/lifecycle_mr_test.go b/git/managed/lifecycle_mr_test.go index 8ac0b15..c10f2a0 100644 --- a/git/managed/lifecycle_mr_test.go +++ b/git/managed/lifecycle_mr_test.go @@ -768,6 +768,9 @@ 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( isolatedLifecycleBaseEnv(t), "GIT_CONFIG_GLOBAL="+globalConfig, @@ -1146,6 +1149,9 @@ 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( isolatedLifecycleBaseEnv(t), "GIT_CONFIG_GLOBAL=.gitconfig", @@ -1197,6 +1203,9 @@ 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( isolatedLifecycleBaseEnv(t), "GIT_CONFIG_GLOBAL=.gitconfig",