From 63dfb2229b0f9d3f5fb0ceec607c1b7903a68cbe Mon Sep 17 00:00:00 2001 From: Jahvon Dockery Date: Tue, 28 Jul 2026 19:32:34 -0400 Subject: [PATCH 1/6] fix(runner): report nested serial/parallel exec errors --- internal/runner/engine/engine.go | 13 +++++++++++++ internal/runner/parallel/parallel.go | 2 +- internal/runner/serial/serial.go | 2 +- 3 files changed, 15 insertions(+), 2 deletions(-) diff --git a/internal/runner/engine/engine.go b/internal/runner/engine/engine.go index 5bda9e97..edfbd8e3 100644 --- a/internal/runner/engine/engine.go +++ b/internal/runner/engine/engine.go @@ -2,6 +2,7 @@ package engine import ( "context" + "errors" "fmt" "golang.org/x/sync/errgroup" @@ -30,6 +31,18 @@ func (rs ResultSummary) HasErrors() bool { return false } +// Err returns the failures as one error, naming the executable behind each, or nil when +// everything succeeded. +func (rs ResultSummary) Err() error { + var errs []error + for _, r := range rs.Results { + if r.Error != nil { + errs = append(errs, fmt.Errorf("%s: %w", r.ID, r.Error)) + } + } + return errors.Join(errs...) +} + func (rs ResultSummary) String() string { var res string if rs.HasErrors() { diff --git a/internal/runner/parallel/parallel.go b/internal/runner/parallel/parallel.go index 375bfd4b..1184aedf 100644 --- a/internal/runner/parallel/parallel.go +++ b/internal/runner/parallel/parallel.go @@ -288,7 +288,7 @@ func handleExec( } } if results.HasErrors() { - return fmt.Errorf("parallel execution failed") + return errors.Wrap(results.Err(), "parallel execution failed") } return nil } diff --git a/internal/runner/serial/serial.go b/internal/runner/serial/serial.go index 94c303f8..e8e61f02 100644 --- a/internal/runner/serial/serial.go +++ b/internal/runner/serial/serial.go @@ -268,7 +268,7 @@ func handleExec( } } if results.HasErrors() { - return fmt.Errorf("serial execution failed") + return errors.Wrap(results.Err(), "serial execution failed") } return nil } From 48d87325729224b11ce18dcbb1ba294e288bce38 Mon Sep 17 00:00:00 2001 From: Jahvon Dockery Date: Tue, 28 Jul 2026 19:34:05 -0400 Subject: [PATCH 2/6] feat(runner): provenance env vars as first class attributions --- .claude/settings.json | 4 + cmd/internal/exec.go | 6 +- docs/.vitepress/config.mts | 1 + docs/guides/run-provenance.md | 192 +++++++++++++++++++++++ internal/mcp/command_executor.go | 20 ++- internal/mcp/provenance_internal_test.go | 51 ++++++ pkg/context/context.go | 44 +++++- pkg/context/context_test.go | 57 +++++++ pkg/store/provenance_test.go | 50 ++++++ pkg/store/store.go | 29 ++++ 10 files changed, 444 insertions(+), 10 deletions(-) create mode 100644 docs/guides/run-provenance.md create mode 100644 pkg/store/provenance_test.go diff --git a/.claude/settings.json b/.claude/settings.json index 08f7954a..719d593c 100644 --- a/.claude/settings.json +++ b/.claude/settings.json @@ -54,5 +54,9 @@ "Read(./.env)", "Read(./.env.*)" ] + }, + "env": { + "FLOW_RUN_CLIENT": "claude-code", + "FLOW_RUN_SESSION": "${CLAUDE_CODE_SESSION_ID}" } } diff --git a/cmd/internal/exec.go b/cmd/internal/exec.go index 3e4d7c87..8f02f7a1 100644 --- a/cmd/internal/exec.go +++ b/cmd/internal/exec.go @@ -702,15 +702,15 @@ type provenance struct { // runProvenanceFromEnv resolves run provenance from environment variables set by the caller // (e.g. the MCP server). Source defaults to "cli". func runProvenanceFromEnv() provenance { - source := os.Getenv(store.RunSourceEnv) + source := store.RunEnvValue(store.RunSourceEnv) if source == "" { source = store.RunSourceCLI } wd, _ := os.Getwd() return provenance{ source: source, - client: os.Getenv(store.RunClientEnv), - session: os.Getenv(store.RunSessionEnv), + client: store.RunEnvValue(store.RunClientEnv), + session: store.RunEnvValue(store.RunSessionEnv), dir: wd, } } diff --git a/docs/.vitepress/config.mts b/docs/.vitepress/config.mts index 95e69132..b22cd9c4 100644 --- a/docs/.vitepress/config.mts +++ b/docs/.vitepress/config.mts @@ -73,6 +73,7 @@ export default defineConfig({ items: [ { text: 'Interactive UI', link: '/guides/interactive' }, { text: 'AI Tools', link: '/guides/ai-tools' }, + { text: 'Run Provenance', link: '/guides/run-provenance' }, { text: 'Integrations', link: '/guides/integrations' }, ] }, diff --git a/docs/guides/run-provenance.md b/docs/guides/run-provenance.md new file mode 100644 index 00000000..2e767f22 --- /dev/null +++ b/docs/guides/run-provenance.md @@ -0,0 +1,192 @@ +--- +title: Run Provenance +--- + +# Run Provenance + +Every execution flow records answers *what ran*. Provenance answers **who ran it** — a terminal, +a GUI, or an AI assistant, and which one. + +This page is the contract for setting it. For reading it back, see +[History & Logs](execution-history.md). + +## What gets recorded + +| Field | Meaning | +|---|---| +| `source` | How the run reached flow — `cli`, `desktop`, or `mcp` | +| `clientName` | Who drove it — e.g. `claude-code`, `cursor` | +| `sessionId` | What it groups with, so one assistant's related runs stay together | +| `workingDir` | Where it executed — the directory, not just the workspace | + +`source` is compared as a plain string throughout, so the set is open. The three values above +are what flow itself produces; anything embedding flow can record its own without waiting for +a release. + +## MCP runs: automatic + +Nothing to configure. When an assistant runs something through the +[MCP server](ai-tools.md) — a named `execute`, a `run_command`, or a `run_executable` — the +server tags it before handing off to the CLI: + +```shell +flow logs --source mcp --limit 1 -o json +``` +```json +{ + "ref": "exec myproject/adhoc-run-tests", + "source": "mcp", + "clientName": "claude-code", + "sessionId": "4ce5bfea-7ada-4b20-b59e-bcc0df500181", + "command": "go test ./...", + "label": "run the test suite" +} +``` + +### Reading your own session ID + +A client that wants to group its runs later needs the session ID it will be tagged with. +Over stdio nothing in the transport carries it, so the server reports it in `get_info`: + +```json +{ + "currentContext": { + "workspace": "myproject", + "sessionId": "4ce5bfea-7ada-4b20-b59e-bcc0df500181" + } +} +``` + +Call it once when you connect, keep the value, and query with it later: + +```shell +flow logs --session 4ce5bfea-7ada-4b20-b59e-bcc0df500181 +``` + +### What a session is + +**Whatever the caller says belongs together.** flow resolves it in this order: + +1. a per-connection ID from the transport, when one exists +2. otherwise `FLOW_RUN_SESSION` from the environment — the caller told us +3. otherwise a UUID minted once per `flow mcp` process + +Step 2 is what keeps a single conversation intact. An assistant does not only use MCP; it +also shells out to `flow` directly. If the server invented an ID while the shell inherited a +different one, the same conversation would land in two groups. A caller that tags its +environment gets both paths under one ID. + +The minted UUID is a last resort, and it approximates a conversation rather than being one — +a client that reconnects mid-conversation gets a new one. If you know your own conversation +identity, supply it and flow will use it instead. + +## Everything else: three environment variables + +Runs that arrive any other way — an assistant shelling out, a script, an app embedding flow — +would otherwise be indistinguishable from you typing at a prompt. flow reads provenance from +the environment for **every** run, so anything that can export a variable can attribute itself: + +| Variable | Sets | Default | +|---|---|---| +| `FLOW_RUN_SOURCE` | `source` | `cli` | +| `FLOW_RUN_CLIENT` | `clientName` | empty | +| `FLOW_RUN_SESSION` | `sessionId` | empty | + +```shell +FLOW_RUN_CLIENT=claude-code FLOW_RUN_SESSION=$MY_SESSION_ID flow run build +``` + +```shell +flow logs --client claude-code # finds it, even though source is still "cli" +``` + +Leaving `FLOW_RUN_SOURCE` alone is usually right: the run genuinely did arrive through the +CLI, and `clientName` is what says an assistant drove it. Set it when the surface itself is +distinct — a desktop app sets `desktop` so its runs are not confused with a terminal's. + +> **Export it, don't pass it.** Environment beats a flag the assistant must remember on every +> call. A model can silently omit an argument; it cannot omit a variable it never sees. This +> is also why identity is not a tool parameter — parameters are for intent, which only the +> model knows. + +### Referencing another variable + +A session ID is only known once the assistant is running, so it cannot be written into a +config file as a constant. Most harnesses store that file's values **verbatim** — no `${...}` +interpolation — which would otherwise leave you recording a literal `${...}` on every run. + +So flow resolves the reference itself. Give it `${NAME}` and it reads `NAME` from the +environment: + +```shell +FLOW_RUN_SESSION='${MY_HARNESS_SESSION_ID}' flow run build +# records the value of MY_HARNESS_SESSION_ID +``` + +If `NAME` is unset the session is recorded empty, never as the literal — one shared constant +across every run would group unrelated work together, which is worse than recording nothing. + +### Example: an editor or agent harness + +Most agent tools let you define environment variables for the commands they run. Point flow at +the variable the tool already exports for its own session: + +```json +{ + "env": { + "FLOW_RUN_CLIENT": "claude-code", + "FLOW_RUN_SESSION": "${CLAUDE_CODE_SESSION_ID}" + } +} +``` + +That works whether or not the tool expands `${...}` itself — flow handles it either way. The +variable's name lives in your config rather than in flow, so when a vendor renames theirs you +edit one line here instead of waiting for a flow release. + +### Example: embedding flow behind a UI + +Set the variables on the process you spawn: + +```go +cmd := exec.Command("flow", "run", "build") +cmd.Env = append(os.Environ(), + "FLOW_RUN_SOURCE=desktop", + "FLOW_RUN_CLIENT=my-app", +) +``` + +A wrapper that also runs an MCP server can set `FLOW_RUN_SESSION` on it too, and the server +will use that instead of minting its own — so runs the assistant makes over MCP and runs the +wrapper makes directly share one session. + +## What flow deliberately does not do + +**No client registry.** flow does not sniff for `CLAUDE_CODE_SESSION_ID`, `CURSOR_*`, or any +other vendor's variables. Those are undocumented internals that get renamed, and detection +built on them fails silently — history quietly stops grouping and nobody notices. Each tool +maps its own variables onto the contract above; flow stays neutral. + +**No conversation identity of its own.** A session is a grouping key, not a claim about what a +conversation is. flow will happily record a conversation ID you hand it — that is what step 2 +above is for — but it never goes looking for one, and it stores nothing that points back at a +chat. When nobody supplies an identity, the minted per-process UUID is an approximation, and +flow treats it as one. Deciding what a conversation *is* stays with the client, which is the +only thing that can know. + +## Querying it + +```shell +flow logs --source mcp # only assistant-launched runs +flow logs --client cursor # one client +flow logs --session # one connection's runs +flow logs --source mcp --status failed # what an assistant broke +``` + +Assistants can review their own activity with `get_execution_logs`, which takes the same +filters plus `mine: true` — scoped to the calling session without needing to know its ID. + +## What's Next? + +- [History & Logs](execution-history.md) — reading, filtering, and clearing history +- [AI Tools](ai-tools.md) — setting up the MCP server diff --git a/internal/mcp/command_executor.go b/internal/mcp/command_executor.go index a4962874..67586fa9 100644 --- a/internal/mcp/command_executor.go +++ b/internal/mcp/command_executor.go @@ -45,6 +45,9 @@ const stdioSessionID = "stdio" // is stdio-only and each client spawns its own `flow mcp` process, so one process is exactly one // client connection — the session boundary mcp-go's constant fails to draw. It is resolved once // and never changes, which is what makes it a usable grouping key in execution history. +// +// It is the last resort: a caller that supplies its own identifier knows better than we do what +// belongs together. var processSessionID = sync.OnceValue(uuid.NewString) // mcpProvenance builds run provenance for an MCP-originated command, capturing the calling client's @@ -57,10 +60,21 @@ func mcpProvenance(ctx context.Context) runProvenance { prov.Client = withInfo.GetClientInfo().Name } } - // Only substitute when the transport gave us nothing usable, so a transport that does issue - // genuine per-connection IDs keeps them. + // A transport that issues genuine per-connection IDs keeps them; only substitute when it + // gave us nothing usable. if prov.Session == "" || prov.Session == stdioSessionID { - prov.Session = processSessionID() + // An inherited session beats one we invent. A harness that tags its environment wants + // the runs it makes over MCP grouped with the ones it makes by shelling out to the CLI + // — minting our own here would split a single conversation across two IDs. + if inherited := store.RunEnvValue(store.RunSessionEnv); inherited != "" { + prov.Session = inherited + } else { + prov.Session = processSessionID() + } + } + // Same reasoning for the client, for a transport that does not report one. + if prov.Client == "" { + prov.Client = store.RunEnvValue(store.RunClientEnv) } return prov } diff --git a/internal/mcp/provenance_internal_test.go b/internal/mcp/provenance_internal_test.go index fc91f616..7f8c693e 100644 --- a/internal/mcp/provenance_internal_test.go +++ b/internal/mcp/provenance_internal_test.go @@ -20,6 +20,22 @@ func (f fakeSession) NotificationChannel() chan<- mcp.JSONRPCNotification { } func (f fakeSession) SessionID() string { return f.id } +// fakeSessionWithInfo also reports a client name, as a transport that completed the initialize +// handshake does. +type fakeSessionWithInfo struct { + fakeSession + name string +} + +func (f fakeSessionWithInfo) GetClientInfo() mcp.Implementation { + return mcp.Implementation{Name: f.name} +} +func (f fakeSessionWithInfo) SetClientInfo(mcp.Implementation) {} +func (f fakeSessionWithInfo) GetClientCapabilities() mcp.ClientCapabilities { + return mcp.ClientCapabilities{} +} +func (f fakeSessionWithInfo) SetClientCapabilities(mcp.ClientCapabilities) {} + func TestMCPProvenance(t *testing.T) { srv := server.NewMCPServer("test", "1.0.0") @@ -40,6 +56,41 @@ func TestMCPProvenance(t *testing.T) { } }) + t.Run("prefers an inherited session over one it mints", func(t *testing.T) { + // A harness that tags its environment wants its MCP runs grouped with the ones it + // makes by shelling out; minting our own would split one conversation across two IDs. + t.Setenv(store.RunSessionEnv, "harness-conversation-9") + ctx := srv.WithContext(context.Background(), fakeSession{id: stdioSessionID}) + if prov := mcpProvenance(ctx); prov.Session != "harness-conversation-9" { + t.Errorf("expected the inherited session, got %q", prov.Session) + } + }) + + t.Run("resolves an inherited session given as a ${NAME} reference", func(t *testing.T) { + t.Setenv("HARNESS_SESSION_ID", "resolved-conversation") + t.Setenv(store.RunSessionEnv, "${HARNESS_SESSION_ID}") + if prov := mcpProvenance(context.Background()); prov.Session != "resolved-conversation" { + t.Errorf("expected the referenced value, got %q", prov.Session) + } + }) + + t.Run("falls back to the client the environment names", func(t *testing.T) { + t.Setenv(store.RunClientEnv, "some-harness") + // No transport session, so nothing reports a client name. + if prov := mcpProvenance(context.Background()); prov.Client != "some-harness" { + t.Errorf("expected the inherited client, got %q", prov.Client) + } + }) + + t.Run("keeps the transport's client over the environment's", func(t *testing.T) { + t.Setenv(store.RunClientEnv, "stale-value") + sess := fakeSessionWithInfo{fakeSession: fakeSession{id: "real-7"}, name: "cursor"} + ctx := srv.WithContext(context.Background(), sess) + if prov := mcpProvenance(ctx); prov.Client != "cursor" { + t.Errorf("expected the transport's client, got %q", prov.Client) + } + }) + t.Run("keeps a genuine per-connection session ID", func(t *testing.T) { ctx := srv.WithContext(context.Background(), fakeSession{id: "real-session-7"}) if prov := mcpProvenance(ctx); prov.Session != "real-session-7" { diff --git a/pkg/context/context.go b/pkg/context/context.go index 8c3c25d5..116a7fee 100644 --- a/pkg/context/context.go +++ b/pkg/context/context.go @@ -30,7 +30,7 @@ type Context struct { ctx context.Context cancelFunc context.CancelFunc stdOut, stdIn, stdErr *os.File - callbacks []func(*Context) error + callbacks *callbackList tuiOnce sync.Once tuiContainer *tuikit.Container @@ -107,6 +107,7 @@ func NewContext(ctx context.Context, cancelFunc context.CancelFunc, opts ...Opti c := &Context{ appName: "flow", + callbacks: &callbackList{}, ctx: ctx, cancelFunc: cancelFunc, stdOut: os.Stdout, @@ -131,7 +132,10 @@ func NewContext(ctx context.Context, cancelFunc context.CancelFunc, opts ...Opti // fields (CurrentTask, ProcessTmpDir, etc.) func (ctx *Context) ShallowCopy() *Context { cp := &Context{ - appName: ctx.appName, + appName: ctx.appName, + // Shared, not copied: cleanup registered by a parallel branch running on this copy + // must reach the root's Finalize. Copying the slice dropped it on the floor. + callbacks: ctx.callbacks, ctx: ctx.ctx, cancelFunc: ctx.cancelFunc, stdOut: ctx.stdOut, @@ -281,18 +285,50 @@ func (ctx *Context) SetView(view tuikit.View) error { return ctx.TUIContainer().SetView(view) } +// callbackList collects deferred cleanup registered on a context and on every shallow copy of +// it. Copies share one list by pointer, because a copy is what gets handed to a parallel +// branch: cleanup registered in there (temporary env files, for one) has to survive back to +// whoever finalizes the root. Guarded, since those branches register concurrently. +type callbackList struct { + mu sync.Mutex + funcs []func(*Context) error +} + +func (l *callbackList) add(callback func(*Context) error) { + l.mu.Lock() + defer l.mu.Unlock() + l.funcs = append(l.funcs, callback) +} + +// drain returns the registered callbacks and empties the list, so finalizing more than once +// cannot run the same cleanup twice. +func (l *callbackList) drain() []func(*Context) error { + if l == nil { + return nil + } + l.mu.Lock() + defer l.mu.Unlock() + funcs := l.funcs + l.funcs = nil + return funcs +} + func (ctx *Context) AddCallback(callback func(*Context) error) { if callback == nil { return } - ctx.callbacks = append(ctx.callbacks, callback) + if ctx.callbacks == nil { + // Only reachable for a hand-built Context; NewContext and ShallowCopy both set it. + ctx.callbacks = &callbackList{} + } + ctx.callbacks.add(callback) } func (ctx *Context) Finalize() { _ = ctx.stdIn.Close() _ = ctx.stdOut.Close() - for _, cb := range ctx.callbacks { + for _, cb := range ctx.callbacks.drain() { if err := cb(ctx); err != nil { logger.Log().WrapError(err, "callback execution error") } diff --git a/pkg/context/context_test.go b/pkg/context/context_test.go index 7f9a5878..e69a9f6d 100644 --- a/pkg/context/context_test.go +++ b/pkg/context/context_test.go @@ -4,6 +4,7 @@ package context import ( "os" "path/filepath" + "sync" "testing" "charm.land/lipgloss/v2" @@ -102,3 +103,59 @@ var _ = ginkgo.Describe("Context", func() { func strPtr(s string) *string { return &s } + +func TestCallbacksSurviveShallowCopy(t *testing.T) { + t.Run("a copy's callbacks reach the original", func(t *testing.T) { + // A parallel branch runs on a shallow copy. Cleanup it registers — temporary env + // files, for one — used to land on the copy and never run, because only the root + // context is ever finalized. + root := &Context{callbacks: &callbackList{}} + var ran []string + + root.AddCallback(func(*Context) error { ran = append(ran, "root"); return nil }) + branch := root.ShallowCopy() + branch.AddCallback(func(*Context) error { ran = append(ran, "branch"); return nil }) + + for _, cb := range root.callbacks.drain() { + _ = cb(root) + } + + if len(ran) != 2 { + t.Fatalf("expected both callbacks to run, got %v", ran) + } + }) + + t.Run("draining twice does not run cleanup twice", func(t *testing.T) { + root := &Context{callbacks: &callbackList{}} + count := 0 + root.AddCallback(func(*Context) error { count++; return nil }) + + for range 2 { + for _, cb := range root.callbacks.drain() { + _ = cb(root) + } + } + + if count != 1 { + t.Errorf("expected the callback to run once, ran %d times", count) + } + }) + + t.Run("concurrent branches can register at once", func(t *testing.T) { + // Run with -race: parallel branches register from their own goroutines. + root := &Context{callbacks: &callbackList{}} + var wg sync.WaitGroup + for range 32 { + wg.Add(1) + go func() { + defer wg.Done() + root.ShallowCopy().AddCallback(func(*Context) error { return nil }) + }() + } + wg.Wait() + + if got := len(root.callbacks.drain()); got != 32 { + t.Errorf("expected 32 callbacks, got %d", got) + } + }) +} diff --git a/pkg/store/provenance_test.go b/pkg/store/provenance_test.go new file mode 100644 index 00000000..c1c3de7e --- /dev/null +++ b/pkg/store/provenance_test.go @@ -0,0 +1,50 @@ +package store_test + +import ( + "testing" + + "github.com/flowexec/flow/v2/pkg/store" +) + +func TestRunEnvValue(t *testing.T) { + t.Run("returns a plain value unchanged", func(t *testing.T) { + t.Setenv(store.RunClientEnv, "claude-code") + if got := store.RunEnvValue(store.RunClientEnv); got != "claude-code" { + t.Errorf("expected %q, got %q", "claude-code", got) + } + }) + + t.Run("resolves a ${NAME} reference from the environment", func(t *testing.T) { + // A harness that cannot interpolate its settings file stores the reference verbatim. + t.Setenv("HARNESS_SESSION_ID", "3f9a-conversation") + t.Setenv(store.RunSessionEnv, "${HARNESS_SESSION_ID}") + if got := store.RunEnvValue(store.RunSessionEnv); got != "3f9a-conversation" { + t.Errorf("expected the referenced value, got %q", got) + } + }) + + t.Run("yields empty when the reference points at nothing", func(t *testing.T) { + // Never the literal: a constant `${...}` on every run would group unrelated runs + // together, which is worse than recording no session at all. + t.Setenv(store.RunSessionEnv, "${NOT_SET_ANYWHERE}") + if got := store.RunEnvValue(store.RunSessionEnv); got != "" { + t.Errorf("expected empty, got %q", got) + } + }) + + t.Run("leaves values that only look like references alone", func(t *testing.T) { + for _, value := range []string{"${}", "$NAME", "{NAME}", "prefix-${NAME}", "${NAME}-suffix"} { + t.Setenv(store.RunSessionEnv, value) + if got := store.RunEnvValue(store.RunSessionEnv); got != value { + t.Errorf("expected %q unchanged, got %q", value, got) + } + } + }) + + t.Run("is empty when the variable is unset", func(t *testing.T) { + t.Setenv(store.RunSessionEnv, "") + if got := store.RunEnvValue(store.RunSessionEnv); got != "" { + t.Errorf("expected empty, got %q", got) + } + }) +} diff --git a/pkg/store/store.go b/pkg/store/store.go index e9d8f287..3519c280 100644 --- a/pkg/store/store.go +++ b/pkg/store/store.go @@ -87,6 +87,35 @@ type DataStore interface { //nolint:interfacebloat // single backing store with Close() error } +// RunEnvValue reads a provenance environment variable, resolving a `${NAME}` value by looking +// NAME up in the environment. +// +// Agent harnesses commonly let you set a static environment value but not interpolate one — +// their settings file stores what you write verbatim. Without this, mapping a harness's own +// session variable onto RunSessionEnv would record the literal `${...}` on every run: a +// constant masquerading as an identity, silently grouping unrelated runs together. That is a +// worse failure than having no session at all, because the grouping looks like it worked. +// +// Resolving here rather than recognizing specific variables keeps flow neutral: the name of +// the harness's variable lives in that harness's config, where it can be corrected when the +// vendor renames it. An unresolvable reference yields empty, since no identity beats a false one. +func RunEnvValue(key string) string { + value := os.Getenv(key) + if name, ok := envReference(value); ok { + return os.Getenv(name) + } + return value +} + +// envReference reports whether a value is a `${NAME}` reference and the name it points at. +func envReference(value string) (string, bool) { + if !strings.HasPrefix(value, "${") || !strings.HasSuffix(value, "}") { + return "", false + } + name := strings.TrimSuffix(strings.TrimPrefix(value, "${"), "}") + return name, name != "" +} + // RunStatus represents the lifecycle state of an execution record. type RunStatus string From b9e866113ec398ed44995aea2099250ea2dade7e Mon Sep 17 00:00:00 2001 From: Jahvon Dockery Date: Tue, 28 Jul 2026 20:17:26 -0400 Subject: [PATCH 3/6] feat(workspace): resolve workspaces by walking up to the nearest flow.yaml flow could only resolve a workspace by name from the user config, so a git worktree or a fresh clone had to be registered before any executable would run. Discovery now walks up from the working directory to the nearest flow.yaml, the way make and bazel find their root, and registration becomes an optimization rather than a prerequisite. A discovered workspace is named for its directory and lives in memory only: overlay caches layer it over the persisted ones so a throwaway clone can never shadow a real workspace for a later command. Closest root wins, and a workspace's flow file scan now stops at nested workspace boundaries, so a worktree checked out inside a repo no longer duplicates its parent's executables. Discovery applies in dynamic mode; --workspace and FLOW_WORKSPACE accept a name or a path and are honored in fixed mode too. MCP tools gain a `dir` parameter and set the subprocess working directory, so an agent working in a worktree no longer runs against the server's directory instead of its own. Also fixes: the dynamic-mode workspace scan picked the first map match rather than the longest, and stripped /private from only one side of the comparison. Co-Authored-By: Claude Opus 5 (1M context) --- cmd/internal/browse.go | 4 +- cmd/internal/exec.go | 60 ++++-- cmd/internal/helpers.go | 31 ++-- cmd/internal/vault.go | 22 ++- cmd/internal/workspace.go | 26 +++ cmd/root.go | 1 + docs/guides/ai-tools.md | 15 +- docs/guides/workspaces.md | 60 ++++++ internal/mcp/command_executor.go | 22 +++ internal/mcp/output_types.go | 8 + internal/mcp/resources.go | 58 ++++-- internal/mcp/server_test.go | 4 +- internal/mcp/tools_executable.go | 38 +++- internal/mcp/tools_system.go | 33 ++-- internal/runner/runner.go | 2 +- internal/utils/env/env.go | 2 +- pkg/cache/executables_cache.go | 285 +++++++++++++++-------------- pkg/cache/overlay.go | 195 ++++++++++++++++++++ pkg/cache/overlay_test.go | 191 +++++++++++++++++++ pkg/cache/templates_cache.go | 128 +++++++------ pkg/context/context.go | 173 +++++++++++------ pkg/context/context_test.go | 69 +++++-- pkg/filesystem/discovery.go | 103 +++++++++++ pkg/filesystem/discovery_test.go | 152 +++++++++++++++ pkg/filesystem/executables.go | 30 +++ pkg/filesystem/executables_test.go | 48 +++++ pkg/filesystem/resolve.go | 239 ++++++++++++++++++++++++ pkg/filesystem/resolve_test.go | 229 +++++++++++++++++++++++ pkg/filesystem/workspace.go | 23 ++- tests/discovery_e2e_test.go | 142 ++++++++++++++ tests/utils/context.go | 89 +++++++++ types/config/config.go | 101 +++++++--- types/config/config_test.go | 109 +++++++++++ 33 files changed, 2326 insertions(+), 366 deletions(-) create mode 100644 pkg/cache/overlay.go create mode 100644 pkg/cache/overlay_test.go create mode 100644 pkg/filesystem/discovery.go create mode 100644 pkg/filesystem/discovery_test.go create mode 100644 pkg/filesystem/resolve.go create mode 100644 pkg/filesystem/resolve_test.go create mode 100644 tests/discovery_e2e_test.go create mode 100644 types/config/config_test.go diff --git a/cmd/internal/browse.go b/cmd/internal/browse.go index b61bc6d3..ffb7065f 100644 --- a/cmd/internal/browse.go +++ b/cmd/internal/browse.go @@ -80,7 +80,7 @@ func executableLibrary(ctx *context.Context, cmd *cobra.Command, _ []string) { wsFilter := flags.ValueFor[string](cmd, *flags.FilterWorkspaceFlag, false) switch wsFilter { case ".": - wsFilter = ctx.Config.CurrentWorkspace + wsFilter = ctx.CurrentWorkspaceName() case executable.WildcardWorkspace: wsFilter = "" } @@ -140,7 +140,7 @@ func executableLibrary(ctx *context.Context, cmd *cobra.Command, _ []string) { func listExecutables(ctx *context.Context, cmd *cobra.Command, _ []string) { wsFilter := flags.ValueFor[string](cmd, *flags.FilterWorkspaceFlag, false) if wsFilter == "." { - wsFilter = ctx.Config.CurrentWorkspace + wsFilter = ctx.CurrentWorkspaceName() } nsFilter := flags.ValueFor[string](cmd, *flags.FilterNamespaceFlag, false) diff --git a/cmd/internal/exec.go b/cmd/internal/exec.go index 8f02f7a1..69edc317 100644 --- a/cmd/internal/exec.go +++ b/cmd/internal/exec.go @@ -92,7 +92,19 @@ func RegisterExecCmd(ctx *context.Context, rootCmd *cobra.Command) { rootCmd.AddCommand(subCmd) } -func execPreRun(_ *context.Context, _ *cobra.Command, _ []string) { +func execPreRun(ctx *context.Context, cmd *cobra.Command, _ []string) { + // --workspace has to move the whole context, not just the transient run's: a named + // executable is looked up by a ref expanded from the current workspace, and running one from + // a different workspace is refused outright. + if explicit := flags.ValueFor[string](cmd, *flags.RunWorkspaceFlag, false); explicit != "" { + res, err := filesystem.ResolveWorkspace(ctx.Config, filesystem.ResolveOptions{Override: explicit}) + if err != nil { + errhandler.HandleUsage(ctx, cmd, "%v", err) + return + } + ctx.SetCurrentWorkspace(res) + } + runner.RegisterRunner(exec.NewRunner()) runner.RegisterRunner(launch.NewRunner()) runner.RegisterRunner(request.NewRunner()) @@ -210,7 +222,7 @@ func resolveExecutableForRun( "executable '%s' belongs to workspace '%s' and cannot be run from the current workspace '%s'", ref, e.Workspace(), - ctx.Config.CurrentWorkspace, + ctx.CurrentWorkspaceName(), )) } return e, ref @@ -357,37 +369,51 @@ func resolveSpecContent(spec string) (string, error) { func setTransientContext(ctx *context.Context, cmd *cobra.Command, e *executable.Executable, runDir string) { wsName, wsPath := resolveRunWorkspace(ctx, cmd, runDir) - currentName := "" - if ctx.CurrentWorkspace != nil { - currentName = ctx.CurrentWorkspace.AssignedName() - } + currentName := ctx.CurrentWorkspaceName() if wsName != "" && wsName != currentName { logger.Log().Infof("Running in workspace '%s' (current workspace is '%s')", wsName, currentName) + } else if wsName != "" && !ctx.WorkspaceIsRegistered() { + logger.Log().Debugf("Running in unregistered workspace '%s' (%s)", wsName, wsPath) } var flowFilePath string if wsPath != "" { - flowFilePath = filepath.Join(wsPath, "flow.yaml") + flowFilePath = filepath.Join(wsPath, filesystem.WorkspaceConfigFileName) } e.SetContext(wsName, wsPath, ctx.Config.CurrentNamespace, flowFilePath) } -// resolveRunWorkspace picks the workspace a transient run should use, in priority order: -// an explicit --workspace flag, then the workspace whose location contains runDir (longest match), -// then the global current workspace. It never changes the global current workspace. +// resolveRunWorkspace picks the workspace a transient run should use, in priority order: an +// explicit --workspace flag (a registered name or a path), then the nearest workspace at or above +// runDir, then a registered workspace containing runDir, then the global current workspace. It +// never changes the global current workspace. func resolveRunWorkspace(ctx *context.Context, cmd *cobra.Command, runDir string) (name, path string) { - wsList, err := ctx.WorkspacesCache.GetWorkspaceConfigList() - if err != nil { - logger.Log().Debugf("unable to load workspaces for transient run resolution: %v", err) + if explicit := flags.ValueFor[string](cmd, *flags.RunWorkspaceFlag, false); explicit != "" { + res, err := filesystem.ResolveWorkspace(ctx.Config, filesystem.ResolveOptions{ + Dir: runDir, Override: explicit, + }) + if err != nil || res == nil { + errhandler.HandleUsage(ctx, cmd, "unable to resolve workspace %q: %v", explicit, err) + return "", "" + } + return res.Name, res.Path } - if explicit := flags.ValueFor[string](cmd, *flags.RunWorkspaceFlag, false); explicit != "" { - if ws := wsList.FindByName(explicit); ws != nil { - return ws.AssignedName(), ws.Location() + // Discovery covers a worktree or clone that is registered nowhere; the cache lookup below + // still handles a registered workspace whose own flow.yaml has gone missing. + if runDir != "" { + res, err := filesystem.ResolveWorkspace(ctx.Config, filesystem.ResolveOptions{Dir: runDir}) + if err != nil { + logger.Log().Debugf("unable to resolve workspace for transient run: %v", err) + } else if res != nil && res.Source != filesystem.SourceCurrent { + return res.Name, res.Path } - errhandler.HandleUsage(ctx, cmd, "unknown workspace %q", explicit) } + wsList, err := ctx.WorkspacesCache.GetWorkspaceConfigList() + if err != nil { + logger.Log().Debugf("unable to load workspaces for transient run resolution: %v", err) + } if ws := workspaceForPath(wsList, runDir); ws != nil { return ws.AssignedName(), ws.Location() } diff --git a/cmd/internal/helpers.go b/cmd/internal/helpers.go index 493bdd1a..1fef2ebe 100644 --- a/cmd/internal/helpers.go +++ b/cmd/internal/helpers.go @@ -105,22 +105,27 @@ func printContext(ctx *context.Context, cmd *cobra.Command) { } func workspaceOrCurrent(ctx *context.Context, workspaceName string) *workspace.Workspace { - var ws *workspace.Workspace - if workspaceName == "" { - ws = ctx.CurrentWorkspace - workspaceName = ws.AssignedName() - } else { - wsPath, wsFound := ctx.Config.Workspaces[workspaceName] - if !wsFound { + // An empty name, or the name of the workspace already resolved for this command, is answered + // from the context — which is the only place a workspace discovered from the working + // directory exists, since it is in no config. + if workspaceName == "" || ctx.CurrentWorkspaceName() == workspaceName { + if ctx.CurrentWorkspace == nil { return nil } - var err error - ws, err = filesystem.LoadWorkspaceConfig(workspaceName, wsPath) - if err != nil { - logger.Log().WrapError(err, "unable to load workspace config") - } - ws.SetContext(workspaceName, wsPath) + logger.Log().Debugf("'%s' workspace set", ctx.CurrentWorkspaceName()) + return ctx.CurrentWorkspace + } + + wsPath, wsFound := ctx.Config.Workspaces[workspaceName] + if !wsFound { + return nil + } + ws, err := filesystem.LoadWorkspaceConfig(workspaceName, wsPath) + if err != nil { + logger.Log().WrapError(err, "unable to load workspace config") + return nil } + ws.SetContext(workspaceName, wsPath) logger.Log().Debugf("'%s' workspace set", workspaceName) return ws } diff --git a/cmd/internal/vault.go b/cmd/internal/vault.go index 19a6195c..ad16653e 100644 --- a/cmd/internal/vault.go +++ b/cmd/internal/vault.go @@ -76,6 +76,25 @@ func registerCreateVaultCmd(ctx *context.Context, vaultCmd *cobra.Command) { vaultCmd.AddCommand(createCmd) } +// vaultWorkspacePath returns the directory a workspace-relative vault path resolves against. A +// vault outlives the command that created it, so anchoring one in a directory flow only found by +// walking up from here is almost never what the user meant — warn before doing it. +func vaultWorkspacePath(ctx *context.Context, cmd *cobra.Command) string { + if ctx.CurrentWorkspace == nil { + return "" + } + path := ctx.CurrentWorkspace.Location() + // Machine-readable output is parsed by the caller, so a loose warning line would corrupt it. + outputFormat := flags.ValueFor[string](cmd, *flags.OutputFormatFlag, false) + if !ctx.WorkspaceIsRegistered() && outputFormat == "" { + logger.Log().Warnf( + "workspace '%s' is not registered; run 'flow workspace add %s %s' to keep this vault reachable", + ctx.CurrentWorkspaceName(), ctx.CurrentWorkspaceName(), path, + ) + } + return path +} + func createVaultFunc(ctx *context.Context, cmd *cobra.Command, args []string) { vaultName := args[0] vaultType := flags.ValueFor[string](cmd, *flags.VaultTypeFlag, false) @@ -120,9 +139,8 @@ func createVaultFunc(ctx *context.Context, cmd *cobra.Command, args []string) { ctx.Config.Vaults = make(map[string]string) } - curWs := ctx.Config.CurrentWorkspace vaultPath = utils.ExpandDirectory( - vaultPath, ctx.Config.Workspaces[curWs], vault.CacheDirectory(vaultName), nil, + vaultPath, vaultWorkspacePath(ctx, cmd), vault.CacheDirectory(vaultName), nil, ) ctx.Config.Vaults[vaultName] = vaultPath diff --git a/cmd/internal/workspace.go b/cmd/internal/workspace.go index 1c32141b..588c2d25 100644 --- a/cmd/internal/workspace.go +++ b/cmd/internal/workspace.go @@ -298,6 +298,15 @@ func switchWorkspaceFunc(ctx *context.Context, cmd *cobra.Command, args []string ws := args[0] userConfig := ctx.Config if _, found := userConfig.Workspaces[ws]; !found { + // Switching persists a name into the config, so a workspace that exists only because + // flow found its flow.yaml from here cannot be switched to — say so rather than + // reporting it as missing when the user is standing in it. + if ctx.CurrentWorkspaceName() == ws && !ctx.WorkspaceIsRegistered() { + errhandler.HandleUsage(ctx, cmd, + "workspace '%s' was discovered at %s but is not registered - run 'flow workspace add %s %s' first", + ws, ctx.CurrentWorkspace.Location(), ws, ctx.CurrentWorkspace.Location()) + return + } errhandler.HandleFatal(ctx, cmd, flowerrors.WorkspaceNotFoundError{Workspace: ws}) } userConfig.CurrentWorkspace = ws @@ -367,10 +376,19 @@ func removeWorkspaceFunc(ctx *context.Context, cmd *cobra.Command, args []string errhandler.HandleFatal(ctx, cmd, flowerrors.WorkspaceNotFoundError{Workspace: name}) } + wsPath := userConfig.Workspaces[name] delete(userConfig.Workspaces, name) if err := filesystem.WriteConfig(userConfig); err != nil { errhandler.HandleFatal(ctx, cmd, err) } + if filesystem.WorkspaceConfigExists(wsPath) { + // The directory keeps its flow.yaml, so flow will still find it by walking up — just + // under its directory name rather than the name being removed here. + logger.Log().Infof( + "%s still has a %s, so running from %s resolves to the workspace '%s'", + wsPath, filesystem.WorkspaceConfigFileName, wsPath, filepath.Base(wsPath), + ) + } if err := cache.UpdateAll(ctx.DataStore); err != nil { errhandler.HandleFatal(ctx, cmd, errors.Wrap(err, "unable to update cache")) @@ -468,12 +486,20 @@ func getWorkspaceFunc(ctx *context.Context, cmd *cobra.Command, args []string) { if len(args) == 1 { workspaceName = args[0] wsPath = ctx.Config.Workspaces[workspaceName] + if wsPath == "" && ctx.CurrentWorkspaceName() == workspaceName { + wsPath = ctx.CurrentWorkspace.Location() + } } else { if ctx.CurrentWorkspace == nil { errhandler.HandleUsage(ctx, cmd, "no current workspace set — run 'flow workspace add' to get started") + return } workspaceName = ctx.CurrentWorkspace.AssignedName() wsPath = ctx.CurrentWorkspace.Location() + if !ctx.WorkspaceIsRegistered() { + logger.Log().Infof( + "workspace '%s' was discovered at %s and is not registered", workspaceName, wsPath) + } } wsCfg, err := filesystem.LoadWorkspaceConfig(workspaceName, wsPath) diff --git a/cmd/root.go b/cmd/root.go index edc229b2..75aa039c 100644 --- a/cmd/root.go +++ b/cmd/root.go @@ -39,6 +39,7 @@ func NewRootCmd(ctx *context.Context) *cobra.Command { case "fatal": logger.Log().SetLevel(-1) } + ctx.LogWorkspaceResolution() sync := flags.ValueFor[bool](cmd.Root(), *flags.SyncCacheFlag, true) if sync { if err := cache.UpdateAll(ctx.DataStore); err != nil { diff --git a/docs/guides/ai-tools.md b/docs/guides/ai-tools.md index bb8070de..a86f33b6 100644 --- a/docs/guides/ai-tools.md +++ b/docs/guides/ai-tools.md @@ -57,9 +57,9 @@ below: the `.mcp.json` supplies the tools, the skill tells the assistant to reac | `list_workspaces` | All registered workspaces | | `get_workspace` | Details and config for a specific workspace | | `switch_workspace` | Change the active workspace | -| `list_executables` | Browse executables — filterable by tag, verb, workspace | +| `list_executables` | Browse executables — filterable by tag, verb, workspace, and resolvable from a `dir` | | `get_executable` | Full definition and metadata for a specific executable | -| `execute` | Run a **named** executable by ref | +| `execute` | Run a **named** executable by ref, in a given `dir` or `workspace` | | `run_command` | Run one or more **arbitrary** shell commands through flow (with a `label`, working `dir`, and optional `workspace`) — captured in history like any executable | | `run_executable` | Run a **transient executable of any type** from an inline `spec` — a serial/parallel batch, an HTTP `request`, a `render`, or a `launch` — without saving a file | | `get_execution_logs` | Output from recent runs, filterable by `source`/`session`/`status`, or `mine` for this session's own runs | @@ -68,6 +68,17 @@ below: the `.mcp.json` supplies the tools, the skill tells the assistant to reac The three run tools form a ladder, closest-fit first: **`execute`** for a task you've already named, **`run_command`** for a one-off shell command, **`run_executable`** for something richer than a single command. Reaching for flow before a raw shell tool means every run inherits the workspace's environment and secrets and is recorded — see [Observability](#observability) below. +**Working in a worktree or a fresh clone** + +The MCP server inherits whatever directory it was started in, which is often not where you are +working. Pass `dir` on `execute`, `run_command`, `run_executable`, or `list_executables` and flow +resolves the workspace by walking up from *that* directory to the nearest `flow.yaml` — so a git +worktree or a just-cloned repo works without being registered first. `get_info` reports +`workspaceRegistered` and `workspaceSource` so you can tell which case you are in; an +unregistered workspace runs normally but cannot be switched to. Exporting `FLOW_WORKSPACE` in the +server's environment pins it for every call instead. See +[Unregistered workspaces](workspaces.md#unregistered-workspaces). + **Prompts** Structured prompts the assistant can invoke for common tasks: diff --git a/docs/guides/workspaces.md b/docs/guides/workspaces.md index 3d7f1b12..cc365abb 100644 --- a/docs/guides/workspaces.md +++ b/docs/guides/workspaces.md @@ -203,6 +203,66 @@ flow workspace switch my-project # Now flow always uses my-project, regardless of directory ``` +## Unregistered Workspaces + +Registration is an optimization, not a prerequisite. In dynamic mode flow finds its workspace by +walking up from your current directory to the nearest `flow.yaml` — the same way `make` and +`bazel` find their root. Clone a repo and its executables work immediately: + +```shell +git clone https://github.com/acme/service && cd service +flow test # works — no `flow workspace add` needed +``` + +The same rule is what makes git worktrees work, whether the worktree sits inside a registered +workspace or somewhere else entirely: + +```shell +git worktree add .worktrees/feature +cd .worktrees/feature +flow build # runs against the worktree's flow files, not the main checkout's +``` + +An unregistered workspace is named after its directory, runs normally, and is never written +anywhere — not to your config, not to the shared executable cache. What you give up by not +registering it: you can't `flow workspace switch` to it, and other workspaces can't reference its +executables by name. + +Register it when you want those: + +```shell +flow workspace add service /path/to/service +``` + +### Resolution order + +1. `--workspace` or `$FLOW_WORKSPACE` — a registered name **or** a path. Honored in both modes. +2. The nearest `flow.yaml` at or above the current directory (dynamic mode only). If that + directory is registered, its registered name is used. +3. A registered workspace whose directory contains the current one. +4. The workspace set by `flow workspace switch`. + +`$FLOW_WORKSPACE` is the escape hatch in fixed mode, and the convenient one for agents and CI — +export it once rather than remembering a flag on every command: + +```shell +export FLOW_WORKSPACE=/path/to/checkout +``` + +### Nested workspaces + +A directory containing its own `flow.yaml` is a workspace boundary. The closest one wins, and a +parent workspace does not scan into it — so a worktree checked out inside a repo doesn't +duplicate every executable of its parent. Override this for a specific directory by naming it in +the parent's `executables.included`. + +Two directories flow deliberately walks *past* when they sit inside a registered workspace: +vendored dependencies (`vendor/`, `node_modules/`, `third_party/`, `external/`) and repo copies +(`.git/`, `.claude/`). A `flow.yaml` in there belongs to that copy, not to your project. + +> **Note**: there is no stopping point above your home directory. A `flow.yaml` in `~` makes your +> entire home directory a workspace, exactly as registering `~` would. + ## Multi-Workspace Workflows ### Cross-Workspace References diff --git a/internal/mcp/command_executor.go b/internal/mcp/command_executor.go index 67586fa9..59db1526 100644 --- a/internal/mcp/command_executor.go +++ b/internal/mcp/command_executor.go @@ -36,6 +36,27 @@ func provenanceFromContext(ctx context.Context) (runProvenance, bool) { return p, ok } +// runDirCtxKey keys the working directory the flow subprocess should run in. +type runDirCtxKey struct{} + +// withRunDir returns a context carrying the directory to launch the flow subprocess from. +// +// This matters because flow resolves its workspace by walking up from the working directory. The +// server process inherits whatever directory the MCP client was started in, which is routinely +// not where the caller is working — an agent in a git worktree, say. Without this, every run +// would silently execute against the server's directory instead of the caller's. +func withRunDir(ctx context.Context, dir string) context.Context { + if dir == "" { + return ctx + } + return context.WithValue(ctx, runDirCtxKey{}, dir) +} + +func runDirFromContext(ctx context.Context) string { + dir, _ := ctx.Value(runDirCtxKey{}).(string) + return dir +} + // stdioSessionID is what mcp-go reports as the session ID for every stdio connection — a // package constant, not a per-connection value (see mcp-go server/stdio.go). Taken at face // value it collapses every run from every client into one "session". @@ -103,6 +124,7 @@ func (c *FlowCLIExecutor) ExecuteContext(ctx context.Context, args ...string) (s name = envName } cmd := exec.CommandContext(ctx, name, args...) // #nosec G204,G702 + cmd.Dir = runDirFromContext(ctx) if p, ok := provenanceFromContext(ctx); ok { cmd.Env = append(os.Environ(), fmt.Sprintf("%s=%s", store.RunSourceEnv, p.Source), diff --git a/internal/mcp/output_types.go b/internal/mcp/output_types.go index 5cfe9a51..e98e153f 100644 --- a/internal/mcp/output_types.go +++ b/internal/mcp/output_types.go @@ -37,6 +37,14 @@ type CurrentContext struct { Vault string `json:"vault"` WorkspaceMode string `json:"workspaceMode"` WorkspacePath string `json:"workspacePath"` + // WorkspaceRegistered is false when the workspace was found by walking up from the working + // directory rather than being registered in the user config — a git worktree or a fresh + // clone. Such a workspace runs normally but cannot be switched to, and other directories + // cannot reference its executables. + WorkspaceRegistered bool `json:"workspaceRegistered"` + // WorkspaceSource records how the workspace was chosen: "override", "registered", + // "discovered", "prefix", or "current". + WorkspaceSource string `json:"workspaceSource,omitempty"` // SessionID tags every run this connection launches, and is what `flow logs --session` // filters on. A client reads it once here rather than inferring which records are its own: // the value is knowable only to the server, since the stdio transport carries no identity. diff --git a/internal/mcp/resources.go b/internal/mcp/resources.go index 76314a2c..c73a0b7f 100644 --- a/internal/mcp/resources.go +++ b/internal/mcp/resources.go @@ -13,6 +13,7 @@ import ( "github.com/mark3labs/mcp-go/server" "github.com/flowexec/flow/v2/pkg/filesystem" + "github.com/flowexec/flow/v2/types/config" ) func addServerResources(srv *server.MCPServer) { @@ -119,11 +120,15 @@ func executableResourceHandler(_ context.Context, request mcp.ReadResourceReques return nil, fmt.Errorf("failed to load config: %w", err) } + resolved, err := filesystem.ResolveWorkspace(cfg, filesystem.ResolveOptions{}) + if err != nil { + return nil, fmt.Errorf("failed to resolve current workspace: %w", err) + } if parts.workspace == "" { - if cfg.CurrentWorkspace == "" { + if resolved == nil { return nil, fmt.Errorf("workspace is empty in URI and no current workspace is set") } - parts.workspace = cfg.CurrentWorkspace + parts.workspace = resolved.Name } if parts.namespace == "" { parts.namespace = cfg.CurrentNamespace @@ -131,7 +136,12 @@ func executableResourceHandler(_ context.Context, request mcp.ReadResourceReques wsPath, ok := cfg.Workspaces[parts.workspace] if !ok { - return nil, fmt.Errorf("workspace %q not found", parts.workspace) + // The named workspace may be one discovered from the working directory, which is absent + // from the config but still perfectly runnable. + if resolved == nil || resolved.Name != parts.workspace { + return nil, fmt.Errorf("workspace %q not found", parts.workspace) + } + wsPath = resolved.Path } ws, err := filesystem.LoadWorkspaceConfig(parts.workspace, wsPath) @@ -198,19 +208,19 @@ func flowfileResourceHandler(_ context.Context, request mcp.ReadResourceRequest) return nil, fmt.Errorf("failed to load config: %w", err) } + resolved, err := filesystem.ResolveWorkspace(cfg, filesystem.ResolveOptions{}) + if err != nil { + return nil, fmt.Errorf("failed to resolve current workspace: %w", err) + } + absPath := path - if !filepath.IsAbs(path) { - // Try to resolve relative to current workspace - if cfg.CurrentWorkspace != "" { - if wsPath, ok := cfg.Workspaces[cfg.CurrentWorkspace]; ok { - absPath = filepath.Join(wsPath, path) - } - } + if !filepath.IsAbs(path) && resolved != nil { + absPath = filepath.Join(resolved.Path, path) } - // Security check: path must be within a registered workspace - if !isPathInWorkspace(absPath, cfg.Workspaces) { - return nil, fmt.Errorf("path %q is not within a registered workspace", path) + // Security check: path must be within a workspace flow can see from here + if !isPathInWorkspace(absPath, accessibleWorkspaceRoots(cfg, resolved)) { + return nil, fmt.Errorf("path %q is not within a known workspace", path) } data, err := os.ReadFile(filepath.Clean(absPath)) @@ -289,11 +299,27 @@ func extractExecutableURIParts(uri string) executableURIParts { } // isPathInWorkspace checks if an absolute path is within any registered workspace. -func isPathInWorkspace(absPath string, workspaces map[string]string) bool { - for _, wsPath := range workspaces { - if strings.HasPrefix(absPath, wsPath) { +// isPathInWorkspace reports whether absPath lies inside one of the given workspace roots. The +// comparison is path-segment aware, so "/src/wsX" is not treated as being inside "/src/ws". +func isPathInWorkspace(absPath string, roots []string) bool { + for _, root := range roots { + if filesystem.IsPathWithin(absPath, root) { return true } } return false } + +// accessibleWorkspaceRoots returns every directory a resource may be read from: the registered +// workspaces plus the workspace resolved from the working directory, which may be a discovered +// one that appears nowhere in the config. +func accessibleWorkspaceRoots(cfg *config.Config, resolved *filesystem.ResolvedWorkspace) []string { + roots := make([]string, 0, len(cfg.Workspaces)+1) + for _, wsPath := range cfg.Workspaces { + roots = append(roots, wsPath) + } + if resolved != nil { + roots = append(roots, resolved.Path) + } + return roots +} diff --git a/internal/mcp/server_test.go b/internal/mcp/server_test.go index a97ba270..b43612df 100644 --- a/internal/mcp/server_test.go +++ b/internal/mcp/server_test.go @@ -281,7 +281,7 @@ var _ = Describe("MCP Server", func() { It("should call executor with correct arguments", func() { expectedOutput := "list execs execution results" mockExecutor.EXPECT(). - Execute("browse", "--output", "json", "--workspace", "*", "--namespace", "*"). + ExecuteContext(gomock.Any(), "browse", "--output", "json", "--workspace", "*", "--namespace", "*"). Return(expectedOutput, nil) result, err := mcpClient.CallTool(ctx, newCallToolRequest("list_executables", nil)) @@ -598,7 +598,7 @@ executables: cliJSON, _ := json.Marshal(cliResp) mockExecutor.EXPECT(). - Execute("browse", "--output", "json", "--workspace", "*", "--namespace", "*"). + ExecuteContext(gomock.Any(), "browse", "--output", "json", "--workspace", "*", "--namespace", "*"). Return(string(cliJSON), nil) result, err := mcpClient.CallTool(ctx, newCallToolRequest("list_executables", nil)) diff --git a/internal/mcp/tools_executable.go b/internal/mcp/tools_executable.go index ffa3dcea..00e7cdf9 100644 --- a/internal/mcp/tools_executable.go +++ b/internal/mcp/tools_executable.go @@ -47,6 +47,9 @@ func addExecutableTools(srv *server.MCPServer, executor CommandExecutor) { mcp.WithString("keyword", mcp.Description("Keyword filter (optional)")), mcp.WithString("tag", mcp.Description("Tag filter (optional)")), mcp.WithString("cursor", mcp.Description("Pagination cursor for next page of results")), + mcp.WithString("dir", mcp.Description( + "Directory to resolve the workspace from. Set this to the directory you are working in to see the "+ + "executables of the nearest workspace at or above it, even one that is not registered.")), ) listExecutables.Annotations = mcp.ToolAnnotation{ Title: "List executables", @@ -69,6 +72,13 @@ func addExecutableTools(srv *server.MCPServer, executor CommandExecutor) { "If the executable does not have a name, you can specify just the workspace (`ws/`), namespace (`ns:`) "+ "both (`ws/ns:`) or neither if the current workspace/namespace should be used.")), mcp.WithString("args", mcp.Description("Arguments to pass")), + mcp.WithString("dir", mcp.Description( + "Directory to resolve the workspace from and run in. Set this to the directory you are working in "+ + "— a git worktree or a freshly cloned repo — and flow uses the nearest workspace at or above it, "+ + "registered or not. Defaults to the server's own directory, which is often not yours.")), + mcp.WithString("workspace", mcp.Description( + "Workspace to run in, by registered name or by path. Overrides `dir` resolution. Does not change "+ + "the global current workspace.")), mcp.WithBoolean("sync", mcp.Description("Sync executable changes before execution")), mcp.WithOutputSchema[ExecutionOutput](), ) @@ -125,7 +135,7 @@ func getExecutableHandler(executor CommandExecutor) server.ToolHandlerFunc { } func listExecutablesHandler(executor CommandExecutor) server.ToolHandlerFunc { - return func(_ context.Context, request mcp.CallToolRequest) (*mcp.CallToolResult, error) { + return func(ctx context.Context, request mcp.CallToolRequest) (*mcp.CallToolResult, error) { wsFilter := request.GetString("workspace", executable.WildcardWorkspace) nsFilter := request.GetString("namespace", executable.WildcardNamespace) verbFilter := request.GetString("verb", "") @@ -144,7 +154,7 @@ func listExecutablesHandler(executor CommandExecutor) server.ToolHandlerFunc { cmdArgs = append(cmdArgs, "--tag", tagFilter) } - output, err := executor.Execute(cmdArgs...) + output, err := executor.ExecuteContext(withRunDir(ctx, request.GetString("dir", "")), cmdArgs...) if err != nil { return toolError(ErrCodeExecutionFailed, fmt.Sprintf("Failed to list executables: %s", output)), nil } @@ -193,12 +203,18 @@ func executeFlowHandler(srv *server.MCPServer, executor CommandExecutor) server. if args != "" { cmdArgs = append(cmdArgs, strings.Fields(args)...) } + if ws := request.GetString("workspace", ""); ws != "" { + cmdArgs = append(cmdArgs, "--workspace", ws) + } if syncFlag { cmdArgs = append(cmdArgs, "--sync") } // Capture the MCP caller's identity so the resulting execution record records who ran it. ctx = withProvenance(ctx, mcpProvenance(ctx)) + // The subprocess resolves its workspace from its working directory, so the caller's + // directory has to reach it — the server's own is rarely the right one. + ctx = withRunDir(ctx, request.GetString("dir", "")) sendProgress(srv, ctx, progressToken, 0, 2, "Preparing execution") output, err := executor.ExecuteContext(ctx, cmdArgs...) @@ -301,6 +317,9 @@ func addRunExecutableTool(srv *server.MCPServer, executor CommandExecutor) { "{\"execs\":[{\"cmd\":\"...\"},{\"cmd\":\"...\"}]}}).")), mcp.WithString("label", mcp.Description("Short human-readable label recorded in history (defaults to the executable's name).")), + mcp.WithString("dir", + mcp.Description("Directory to resolve the workspace from and run in (defaults to the server's directory). "+ + "Set this to the directory you are working in — flow uses the nearest workspace at or above it.")), mcp.WithString("workspace", mcp.Description("Workspace whose environment to use for this run. Defaults to the workspace containing "+ "the working directory, then the current workspace. Does not change the global current workspace.")), @@ -348,6 +367,9 @@ func runTransientTool( // Tag the run as MCP-originated with the caller's identity. ctx = withProvenance(ctx, mcpProvenance(ctx)) + // `dir` is forwarded as --dir for the command itself; it also has to be the subprocess's + // working directory so workspace resolution starts from where the caller is working. + ctx = withRunDir(ctx, request.GetString("dir", "")) sendProgress(srv, ctx, progressToken, 0, 2, "Preparing execution") output, err := executor.ExecuteContext(ctx, cmdArgs...) @@ -434,10 +456,14 @@ func resolveFlowfilePath(path string) (string, *mcp.CallToolResult) { if err != nil { return "", toolError(ErrCodeInternal, fmt.Sprintf("failed to load config: %s", err)) } - if cfg.CurrentWorkspace != "" { - if wsPath, ok := cfg.Workspaces[cfg.CurrentWorkspace]; ok { - return filepath.Join(wsPath, path), nil - } + // Resolve against the workspace flow would actually run in, which may be one discovered from + // the working directory rather than the one persisted in the config. + res, err := filesystem.ResolveWorkspace(cfg, filesystem.ResolveOptions{}) + if err != nil { + return "", toolError(ErrCodeInvalidInput, err.Error()) + } + if res != nil { + return filepath.Join(res.Path, path), nil } return path, nil } diff --git a/internal/mcp/tools_system.go b/internal/mcp/tools_system.go index 80c88e01..2858b97a 100644 --- a/internal/mcp/tools_system.go +++ b/internal/mcp/tools_system.go @@ -93,23 +93,30 @@ func getInfoHandler(ctx context.Context, _ mcp.CallToolRequest) (*mcp.CallToolRe } cfg.SetDefaults() - var wsName, wsPath string - if len(cfg.Workspaces) > 0 { - wsName, err = cfg.CurrentWorkspaceName() - if err != nil { - return toolError(ErrCodeInternal, fmt.Sprintf("failed to get current workspace name: %s", err)), nil - } - wsPath = cfg.Workspaces[wsName] + // Resolution walks up from this process's directory, so a workspace found there but absent + // from the config still reports correctly. Callers need to know which case they are in: + // an unregistered workspace cannot be switched to, and its executables live only here. + var wsName, wsPath, wsSource string + var wsRegistered bool + res, err := filesystem.ResolveWorkspace(cfg, filesystem.ResolveOptions{}) + if err != nil { + return toolError(ErrCodeInternal, fmt.Sprintf("failed to resolve current workspace: %s", err)), nil + } + if res != nil { + wsName, wsPath = res.Name, res.Path + wsSource, wsRegistered = string(res.Source), res.Registered } output := FlowInfoOutput{ CurrentContext: CurrentContext{ - Workspace: wsName, - Namespace: cfg.CurrentNamespace, - Vault: cfg.CurrentVaultName(), - WorkspaceMode: string(cfg.WorkspaceMode), - WorkspacePath: wsPath, - SessionID: mcpProvenance(ctx).Session, + Workspace: wsName, + Namespace: cfg.CurrentNamespace, + Vault: cfg.CurrentVaultName(), + WorkspaceMode: string(cfg.WorkspaceMode), + WorkspacePath: wsPath, + WorkspaceRegistered: wsRegistered, + WorkspaceSource: wsSource, + SessionID: mcpProvenance(ctx).Session, }, Summary: flowInfoSummary, DocsURL: docsBaseURL, diff --git a/internal/runner/runner.go b/internal/runner/runner.go index dff57e89..2c375184 100644 --- a/internal/runner/runner.go +++ b/internal/runner/runner.go @@ -96,7 +96,7 @@ func ExpressionEnv( envMap, "store", dataMap, "ctx", &CtxData{ - Workspace: ctx.CurrentWorkspace.AssignedName(), + Workspace: ctx.CurrentWorkspaceName(), Namespace: ctx.Config.CurrentNamespace, WorkspacePath: executable.WorkspacePath(), FlowFileName: fn, diff --git a/internal/utils/env/env.go b/internal/utils/env/env.go index c9f4d98d..345b1ca3 100644 --- a/internal/utils/env/env.go +++ b/internal/utils/env/env.go @@ -253,7 +253,7 @@ func EnvListToEnvMap(envList []string) map[string]string { func DefaultEnv(ctx *context.Context, executable *executable.Executable) map[string]string { envMap := make(map[string]string) envMap["FLOW_RUNNER"] = "true" - envMap["FLOW_CURRENT_WORKSPACE"] = ctx.CurrentWorkspace.AssignedName() + envMap["FLOW_CURRENT_WORKSPACE"] = ctx.CurrentWorkspaceName() envMap["FLOW_CURRENT_NAMESPACE"] = ctx.Config.CurrentNamespace if ctx.ProcessTmpDir != "" { envMap["FLOW_TMP_DIRECTORY"] = ctx.ProcessTmpDir diff --git a/pkg/cache/executables_cache.go b/pkg/cache/executables_cache.go index 90837379..e6ce527c 100644 --- a/pkg/cache/executables_cache.go +++ b/pkg/cache/executables_cache.go @@ -53,96 +53,112 @@ type ExecutableCacheImpl struct { func NewExecutableCache(wsCache WorkspaceCache, s store.DataStore) ExecutableCache { return &ExecutableCacheImpl{ - Store: s, - Data: &ExecutableCacheData{ - ExecutableMap: make(map[executable.Ref]string), - AliasMap: make(map[executable.Ref]executable.Ref), - ConfigMap: make(map[string]WorkspaceInfo), - }, + Store: s, + Data: newExecutableCacheData(), WorkspaceCache: wsCache, } } -func (c *ExecutableCacheImpl) Update() error { //nolint:gocognit - logger.Log().Debugf("Updating executable cache data") - wsCacheData, err := c.WorkspaceCache.GetLatestData() +// newExecutableCacheData returns an empty, ready-to-populate index. +func newExecutableCacheData() *ExecutableCacheData { + return &ExecutableCacheData{ + ExecutableMap: make(map[executable.Ref]string), + AliasMap: make(map[executable.Ref]executable.Ref), + ConfigMap: make(map[string]WorkspaceInfo), + } +} + +// indexWorkspaceExecutables walks one workspace's flow files and records every visible, valid +// executable (and its aliases) in data. wsCfg must already carry its name and location. +// +// This is shared by the persisted cache and the in-memory overlay built for a workspace +// discovered from the working directory, so the two agree on visibility, validation, generated +// imports, and alias expansion. +func indexWorkspaceExecutables(data *ExecutableCacheData, wsCfg *workspace.Workspace) { //nolint:gocognit + name := wsCfg.AssignedName() + flowFiles, err := filesystem.LoadWorkspaceFlowFiles(wsCfg) if err != nil { - return fmt.Errorf("failed to get workspace cache data\n%w", err) + logger.Log().Error("failed to load workspace executable configs", "workspace", name, "err", err) + return } + for _, flowFile := range flowFiles { + if len(flowFile.Imports) > 0 { + generated, err := fileparser.ExecutablesFromImports(name, flowFile) + if err != nil { + logger.Log().Error( + "failed to generate executables from files", + "flowFilePath", flowFile.ConfigPath(), + "err", err, + ) + } + flowFile.Executables = append(flowFile.Executables, generated...) + } - cacheData := c.Data - for name, wsCfg := range wsCacheData.Workspaces { - wsCfg.SetContext(name, wsCacheData.WorkspaceLocations[name]) - flowFiles, err := filesystem.LoadWorkspaceFlowFiles(wsCfg) - if err != nil { - logger.Log().Error("failed to load workspace executable configs", "workspace", wsCfg.AssignedName(), "err", err) + if flowFile.Visibility == nil || + common.Visibility(*flowFile.Visibility).IsHidden() || + len(flowFile.Executables) == 0 { continue } - for _, flowFile := range flowFiles { - if len(flowFile.Imports) > 0 { - generated, err := fileparser.ExecutablesFromImports(name, flowFile) - if err != nil { - logger.Log().Error( - "failed to generate executables from files", - "flowFilePath", flowFile.ConfigPath(), - "err", err, - ) - } - flowFile.Executables = append(flowFile.Executables, generated...) + for _, e := range flowFile.Executables { + if vErr := e.Validate(); vErr != nil { + logger.Log().Warn( + "invalid executable found during cache update", + "ref", e.Ref().String(), + "workspace", name, + "err", vErr, + ) + continue } - if flowFile.Visibility == nil || - common.Visibility(*flowFile.Visibility).IsHidden() || - len(flowFile.Executables) == 0 { + if e == nil || (e.Visibility != nil && common.Visibility(*e.Visibility).IsHidden()) { continue } - for _, e := range flowFile.Executables { - if vErr := e.Validate(); vErr != nil { - logger.Log().Warn( - "invalid executable found during cache update", - "ref", e.Ref().String(), - "workspace", wsCfg.AssignedName(), - "err", vErr, - ) - continue - } - if e == nil || (e.Visibility != nil && common.Visibility(*e.Visibility).IsHidden()) { - continue - } + if existingPath, exists := data.ExecutableMap[e.Ref()]; exists && existingPath != flowFile.ConfigPath() { + logger.Log().Warn( + "duplicate executable found during cache update", + "ref", e.Ref().String(), + "conflictPath", existingPath, + "newPath", flowFile.ConfigPath(), + "workspace", name, + ) + } + + data.ExecutableMap[e.Ref()] = flowFile.ConfigPath() - if existingPath, exists := cacheData.ExecutableMap[e.Ref()]; exists && existingPath != flowFile.ConfigPath() { + for _, ref := range enumerateExecutableAliasRefs(e, wsCfg.VerbAliases) { + if existingPrimaryRef, exists := data.AliasMap[ref]; exists && existingPrimaryRef != e.Ref() { logger.Log().Warn( - "duplicate executable found during cache update", - "ref", e.Ref().String(), - "conflictPath", existingPath, - "newPath", flowFile.ConfigPath(), - "workspace", wsCfg.AssignedName(), + "duplicate executable alias found during cache update", + "aliasRef", ref.String(), + "conflictRef", existingPrimaryRef.String(), + "primaryRef", e.Ref().String(), + "workspace", name, ) } + data.AliasMap[ref] = e.Ref() + } - cacheData.ExecutableMap[e.Ref()] = flowFile.ConfigPath() - - for _, ref := range enumerateExecutableAliasRefs(e, wsCfg.VerbAliases) { - if existingPrimaryRef, exists := cacheData.AliasMap[ref]; exists && existingPrimaryRef != e.Ref() { - logger.Log().Warn( - "duplicate executable alias found during cache update", - "aliasRef", ref.String(), - "conflictRef", existingPrimaryRef.String(), - "primaryRef", e.Ref().String(), - "workspace", wsCfg.AssignedName(), - ) - } - cacheData.AliasMap[ref] = e.Ref() - } - - cacheData.ConfigMap[flowFile.ConfigPath()] = WorkspaceInfo{ - WorkspaceName: wsCfg.AssignedName(), - WorkspacePath: wsCfg.Location(), - } + data.ConfigMap[flowFile.ConfigPath()] = WorkspaceInfo{ + WorkspaceName: name, + WorkspacePath: wsCfg.Location(), } } } +} + +func (c *ExecutableCacheImpl) Update() error { + logger.Log().Debugf("Updating executable cache data") + wsCacheData, err := c.WorkspaceCache.GetLatestData() + if err != nil { + return fmt.Errorf("failed to get workspace cache data\n%w", err) + } + + cacheData := c.Data + for name, wsCfg := range wsCacheData.Workspaces { + wsCfg.SetContext(name, wsCacheData.WorkspaceLocations[name]) + indexWorkspaceExecutables(cacheData, wsCfg) + } data, err := json.Marshal(cacheData) if err != nil { @@ -157,71 +173,91 @@ func (c *ExecutableCacheImpl) Update() error { //nolint:gocognit return nil } -func (c *ExecutableCacheImpl) GetExecutableByRef(ref executable.Ref) (*executable.Executable, error) { - err := c.initExecutableCacheData() - if err != nil { - return nil, err - } else if c.Data == nil { - return nil, errors.New("no cached executables found") - } - - if c.Data.loadedExecutables == nil { - c.Data.loadedExecutables = make(map[string]*executable.Executable) - } else if exec, found := c.Data.loadedExecutables[ref.String()]; found { +// lookupExecutable resolves ref against an index, following the alias map when the ref is not a +// primary one, and loading the owning flow file to return the executable itself. +func lookupExecutable(data *ExecutableCacheData, ref executable.Ref) (*executable.Executable, error) { + if data.loadedExecutables == nil { + data.loadedExecutables = make(map[string]*executable.Executable) + } else if exec, found := data.loadedExecutables[ref.String()]; found { return exec, nil } - var primaryRef executable.Ref - cfgPath, found := c.Data.ExecutableMap[ref] - //nolint:nestif + primaryRef := ref + cfgPath, found := data.ExecutableMap[ref] if !found { - if aliasedPrimaryRef, aliasFound := c.Data.AliasMap[ref]; aliasFound { - primaryRef = aliasedPrimaryRef - cfgPath, found = c.Data.ExecutableMap[primaryRef] - if !found { - return nil, flowErrors.NewExecutableNotFoundError(ref.String()) - } - } else { + aliasedPrimaryRef, aliasFound := data.AliasMap[ref] + if !aliasFound { + return nil, flowErrors.NewExecutableNotFoundError(ref.String()) + } + primaryRef = aliasedPrimaryRef + if cfgPath, found = data.ExecutableMap[primaryRef]; !found { return nil, flowErrors.NewExecutableNotFoundError(ref.String()) } - } else { - primaryRef = ref } - cfg, err := filesystem.LoadFlowFile(cfgPath) + wsInfo, found := data.ConfigMap[cfgPath] + if !found { + return nil, errors.Errorf("unable to find workspace info for config %s", cfgPath) + } + + cfg, err := loadFlowFileWithImports(cfgPath, wsInfo) if err != nil { - return nil, errors.Wrap(err, "unable to load executable config") + return nil, err } - wsInfo, found := c.Data.ConfigMap[cfgPath] - if !found { - return nil, errors.Wrap(err, "unable to find workspace info for config") + exec, err := cfg.Executables.FindByVerbAndID(primaryRef.Verb(), primaryRef.ID()) + if err != nil { + return nil, err + } else if exec == nil { + return nil, flowErrors.NewExecutableNotFoundError(ref.String()) + } + + data.loadedExecutables[ref.String()] = exec + + return exec, nil +} + +// listExecutables returns every executable in an index, ordered by flow file path. Callers +// paginate this list across separate calls, so map order would silently drop entries. +func listExecutables(data *ExecutableCacheData) executable.ExecutableList { + list := make(executable.ExecutableList, 0) + for _, cfgPath := range slices.Sorted(maps.Keys(data.ConfigMap)) { + cfg, err := loadFlowFileWithImports(cfgPath, data.ConfigMap[cfgPath]) + if err != nil { + logger.Log().Error("unable to load executable config", "cfgPath", cfgPath, "err", err) + continue + } + list = append(list, cfg.Executables...) } + return list +} +// loadFlowFileWithImports reads a flow file, attaches its workspace context, and appends the +// executables generated from its imports. +func loadFlowFileWithImports(cfgPath string, wsInfo WorkspaceInfo) (*executable.FlowFile, error) { + cfg, err := filesystem.LoadFlowFile(cfgPath) + if err != nil { + return nil, errors.Wrap(err, "unable to load executable config") + } cfg.SetDefaults() cfg.SetContext(wsInfo.WorkspaceName, wsInfo.WorkspacePath, cfgPath) generated, err := fileparser.ExecutablesFromImports(wsInfo.WorkspaceName, cfg) if err != nil { - logger.Log().Warn( - "failed to generate executables from files", - "cfgPath", cfgPath, - "err", err, - ) + logger.Log().Warn("failed to generate executables from files", "cfgPath", cfgPath, "err", err) } cfg.Executables = append(cfg.Executables, generated...) + return cfg, nil +} - execs := cfg.Executables - exec, err := execs.FindByVerbAndID(primaryRef.Verb(), primaryRef.ID()) +func (c *ExecutableCacheImpl) GetExecutableByRef(ref executable.Ref) (*executable.Executable, error) { + err := c.initExecutableCacheData() if err != nil { return nil, err - } else if exec == nil { - return nil, flowErrors.NewExecutableNotFoundError(ref.String()) + } else if c.Data == nil { + return nil, errors.New("no cached executables found") } - - c.Data.loadedExecutables[ref.String()] = exec - - return exec, nil + return lookupExecutable(c.Data, ref) } func (c *ExecutableCacheImpl) GetExecutableList() (executable.ExecutableList, error) { @@ -231,36 +267,7 @@ func (c *ExecutableCacheImpl) GetExecutableList() (executable.ExecutableList, er } else if c.Data == nil { return nil, errors.New("no cached executables found") } - - // Sorted: callers paginate this list across separate calls, so map order would drop entries. - list := make(executable.ExecutableList, 0) - for _, cfgPath := range slices.Sorted(maps.Keys(c.Data.ConfigMap)) { - cfg, err := filesystem.LoadFlowFile(cfgPath) - if err != nil { - logger.Log().Error("unable to load executable config", "cfgPath", cfgPath, "err", err) - continue - } - wsInfo, found := c.Data.ConfigMap[cfgPath] - if !found { - logger.Log().Error("unable to find workspace info for config", "cfgPath", cfgPath) - continue - } - cfg.SetDefaults() - cfg.SetContext(wsInfo.WorkspaceName, wsInfo.WorkspacePath, cfgPath) - - generated, err := fileparser.ExecutablesFromImports(wsInfo.WorkspaceName, cfg) - if err != nil { - logger.Log().Warn( - "failed to generate executables from files", - "cfgPath", cfgPath, - "err", err, - ) - } - cfg.Executables = append(cfg.Executables, generated...) - - list = append(list, cfg.Executables...) - } - return list, nil + return listExecutables(c.Data), nil } func (c *ExecutableCacheImpl) initExecutableCacheData() error { diff --git a/pkg/cache/overlay.go b/pkg/cache/overlay.go new file mode 100644 index 00000000..d4683298 --- /dev/null +++ b/pkg/cache/overlay.go @@ -0,0 +1,195 @@ +package cache + +import ( + "maps" + "sync" + + "github.com/flowexec/flow/v2/types/executable" + "github.com/flowexec/flow/v2/types/workspace" +) + +// The overlay caches let flow run inside a workspace the user never registered — a git worktree, +// a fresh clone — by indexing that one workspace in memory and layering it over the persisted +// cache. +// +// Nothing here is ever written to the data store. The persisted cache is shared by every flow +// invocation on the machine and is keyed by workspace name; writing an ad-hoc workspace into it +// would let a throwaway clone shadow a real workspace for every future command. The overlay +// lives and dies with the process instead, which is affordable because indexing one workspace +// costs about what the per-command cache refresh already costs. +// +// Where a discovered workspace's name collides with a registered one, the overlay wins. That is +// the same "closest wins" rule that selected it in the first place. + +// NewLocalWorkspaceCache layers a discovered workspace over the registered ones so that path +// lookups and workspace listings can see it. +func NewLocalWorkspaceCache(base WorkspaceCache, ws *workspace.Workspace) WorkspaceCache { + return &localWorkspaceCache{base: base, ws: ws} +} + +type localWorkspaceCache struct { + base WorkspaceCache + ws *workspace.Workspace +} + +func (c *localWorkspaceCache) Update() error { return c.base.Update() } + +func (c *localWorkspaceCache) GetData() *WorkspaceCacheData { + return c.inject(c.base.GetData()) +} + +func (c *localWorkspaceCache) GetLatestData() (*WorkspaceCacheData, error) { + data, err := c.base.GetLatestData() + if err != nil { + return nil, err + } + return c.inject(data), nil +} + +// inject copies the base data before adding the discovered workspace. The base implementation +// marshals its own struct straight into the data store on Update, so mutating it here would +// persist the discovered workspace — exactly what this type exists to avoid. +func (c *localWorkspaceCache) inject(data *WorkspaceCacheData) *WorkspaceCacheData { + if data == nil { + data = &WorkspaceCacheData{} + } + merged := &WorkspaceCacheData{ + Workspaces: maps.Clone(data.Workspaces), + WorkspaceLocations: maps.Clone(data.WorkspaceLocations), + } + if merged.Workspaces == nil { + merged.Workspaces = make(map[string]*workspace.Workspace) + } + if merged.WorkspaceLocations == nil { + merged.WorkspaceLocations = make(map[string]string) + } + merged.Workspaces[c.ws.AssignedName()] = c.ws + merged.WorkspaceLocations[c.ws.AssignedName()] = c.ws.Location() + return merged +} + +func (c *localWorkspaceCache) GetWorkspaceConfigList() (workspace.WorkspaceList, error) { + base, err := c.base.GetWorkspaceConfigList() + if err != nil { + return nil, err + } + list := make(workspace.WorkspaceList, 0, len(base)+1) + for _, ws := range base { + if ws.AssignedName() != c.ws.AssignedName() { + list = append(list, ws) + } + } + return append(list, c.ws), nil +} + +// NewLocalExecutableCache resolves executables from a discovered workspace first, falling back to +// the persisted cache for every other workspace. +func NewLocalExecutableCache(base ExecutableCache, ws *workspace.Workspace) ExecutableCache { + return &localExecutableCache{base: base, ws: ws} +} + +type localExecutableCache struct { + base ExecutableCache + ws *workspace.Workspace + + mu sync.Mutex + data *ExecutableCacheData +} + +// index builds the in-memory index on first use. Deferring it keeps commands that never touch +// executables (`flow config`, `flow workspace list`) from walking the workspace tree. +func (c *localExecutableCache) index() *ExecutableCacheData { + c.mu.Lock() + defer c.mu.Unlock() + if c.data == nil { + c.data = newExecutableCacheData() + indexWorkspaceExecutables(c.data, c.ws) + } + return c.data +} + +func (c *localExecutableCache) Update() error { + c.mu.Lock() + c.data = nil + c.mu.Unlock() + return c.base.Update() +} + +func (c *localExecutableCache) GetExecutableByRef(ref executable.Ref) (*executable.Executable, error) { + if exec, err := lookupExecutable(c.index(), ref); err == nil { + return exec, nil + } + return c.base.GetExecutableByRef(ref) +} + +func (c *localExecutableCache) GetExecutableList() (executable.ExecutableList, error) { + base, err := c.base.GetExecutableList() + if err != nil { + return nil, err + } + name := c.ws.AssignedName() + list := make(executable.ExecutableList, 0, len(base)) + for _, e := range base { + if e.Workspace() != name { + list = append(list, e) + } + } + return append(list, listExecutables(c.index())...), nil +} + +// NewLocalTemplateCache resolves templates from a discovered workspace first, falling back to the +// persisted cache. +func NewLocalTemplateCache(base TemplateCache, ws *workspace.Workspace) TemplateCache { + return &localTemplateCache{base: base, ws: ws} +} + +type localTemplateCache struct { + base TemplateCache + ws *workspace.Workspace + + mu sync.Mutex + data *TemplateCacheData +} + +func (c *localTemplateCache) index() *TemplateCacheData { + c.mu.Lock() + defer c.mu.Unlock() + if c.data == nil { + c.data = newTemplateCacheData() + indexWorkspaceTemplates(c.data, c.ws) + } + return c.data +} + +func (c *localTemplateCache) Update() error { + c.mu.Lock() + c.data = nil + c.mu.Unlock() + return c.base.Update() +} + +func (c *localTemplateCache) GetTemplate(name string) (*executable.Template, error) { + if tmpl, err := lookupTemplate(c.index(), name); err == nil { + return tmpl, nil + } + return c.base.GetTemplate(name) +} + +func (c *localTemplateCache) GetTemplateList() (executable.TemplateList, error) { + base, err := c.base.GetTemplateList() + if err != nil { + return nil, err + } + local := listTemplates(c.index()) + localNames := make(map[string]struct{}, len(local)) + for _, tmpl := range local { + localNames[tmpl.Name()] = struct{}{} + } + list := make(executable.TemplateList, 0, len(base)+len(local)) + for _, tmpl := range base { + if _, shadowed := localNames[tmpl.Name()]; !shadowed { + list = append(list, tmpl) + } + } + return append(list, local...), nil +} diff --git a/pkg/cache/overlay_test.go b/pkg/cache/overlay_test.go new file mode 100644 index 00000000..81269bf2 --- /dev/null +++ b/pkg/cache/overlay_test.go @@ -0,0 +1,191 @@ +package cache_test + +import ( + "encoding/json" + "os" + "path/filepath" + + "github.com/flowexec/tuikit/io/mocks" + . "github.com/onsi/ginkgo/v2" + . "github.com/onsi/gomega" + "go.uber.org/mock/gomock" + + "github.com/flowexec/flow/v2/pkg/cache" + cacheMocks "github.com/flowexec/flow/v2/pkg/cache/mocks" + "github.com/flowexec/flow/v2/pkg/filesystem" + "github.com/flowexec/flow/v2/pkg/logger" + "github.com/flowexec/flow/v2/pkg/store" + "github.com/flowexec/flow/v2/types/common" + "github.com/flowexec/flow/v2/types/executable" + "github.com/flowexec/flow/v2/types/workspace" +) + +var _ = Describe("Local workspace overlay", func() { + var ( + ds store.DataStore + baseExecCache cache.ExecutableCache + baseWsCache cache.WorkspaceCache + registeredWs, localWs *workspace.Workspace + tmpDir string + ) + + // writeWorkspace creates a workspace rooted at tmpDir/rel holding one executable named + // execName, and returns its loaded config. + writeWorkspace := func(name, rel, execName string) *workspace.Workspace { + path := filepath.Join(tmpDir, rel) + Expect(filesystem.InitWorkspaceConfig(name, path)).To(Succeed()) + wsCfg, err := filesystem.LoadWorkspaceConfig(name, path) + Expect(err).NotTo(HaveOccurred()) + + v := executable.FlowFileVisibility(common.VisibilityPrivate) + flowFile := &executable.FlowFile{ + Namespace: "ns", + Visibility: &v, + Executables: executable.ExecutableList{{Verb: "run", Name: execName, Exec: &executable.ExecExecutableType{}}}, + } + flowFile.SetContext(name, path, filepath.Join(path, "test"+executable.FlowFileExt)) + Expect(filesystem.WriteFlowFile(flowFile.ConfigPath(), flowFile)).To(Succeed()) + return wsCfg + } + + BeforeEach(func() { + mockLogger := mocks.NewMockLogger(gomock.NewController(GinkgoT())) + logger.Init(logger.InitOptions{Logger: mockLogger, TestingTB: GinkgoTB()}) + mockLogger.EXPECT().Debug(gomock.Any(), gomock.Any()).AnyTimes() + mockLogger.EXPECT().Debugf(gomock.Any()).AnyTimes() + mockLogger.EXPECT().Debug(gomock.Any(), gomock.Any(), gomock.Any()).AnyTimes() + mockLogger.EXPECT().Warn(gomock.Any(), gomock.Any()).AnyTimes() + + var err error + tmpDir, err = os.MkdirTemp("", "flow-overlay-test") + Expect(err).NotTo(HaveOccurred()) + Expect(os.Setenv(filesystem.FlowCacheDirEnvVar, tmpDir)).To(Succeed()) + + ds, err = store.NewDataStore(filepath.Join(tmpDir, "test.db")) + Expect(err).NotTo(HaveOccurred()) + DeferCleanup(func() { _ = ds.Close() }) + + registeredWs = writeWorkspace("registered", "registered", "build") + localWs = writeWorkspace("worktree", "worktree", "deploy") + + mockWsCache := cacheMocks.NewMockWorkspaceCache(gomock.NewController(GinkgoT())) + mockWsCache.EXPECT().GetLatestData().Return(&cache.WorkspaceCacheData{ + Workspaces: map[string]*workspace.Workspace{"registered": registeredWs}, + WorkspaceLocations: map[string]string{"registered": registeredWs.Location()}, + }, nil).AnyTimes() + mockWsCache.EXPECT().GetData().Return(&cache.WorkspaceCacheData{ + Workspaces: map[string]*workspace.Workspace{"registered": registeredWs}, + WorkspaceLocations: map[string]string{"registered": registeredWs.Location()}, + }).AnyTimes() + mockWsCache.EXPECT().GetWorkspaceConfigList(). + Return(workspace.WorkspaceList{registeredWs}, nil).AnyTimes() + mockWsCache.EXPECT().Update().Return(nil).AnyTimes() + baseWsCache = mockWsCache + + baseExecCache = cache.NewExecutableCache(baseWsCache, ds) + Expect(baseExecCache.Update()).To(Succeed()) + }) + + AfterEach(func() { + Expect(os.RemoveAll(tmpDir)).To(Succeed()) + Expect(os.Unsetenv(filesystem.FlowCacheDirEnvVar)).To(Succeed()) + }) + + Describe("executables", func() { + It("resolves an executable from the discovered workspace", func() { + overlay := cache.NewLocalExecutableCache(baseExecCache, localWs) + exec, err := overlay.GetExecutableByRef(executable.NewRef("worktree/ns:deploy", "run")) + Expect(err).NotTo(HaveOccurred()) + Expect(exec.Name).To(Equal("deploy")) + }) + + It("resolves a verb alias from the discovered workspace", func() { + overlay := cache.NewLocalExecutableCache(baseExecCache, localWs) + // "exec" is a related verb of "run", so the alias map has to be populated too. + exec, err := overlay.GetExecutableByRef(executable.NewRef("worktree/ns:deploy", "exec")) + Expect(err).NotTo(HaveOccurred()) + Expect(exec.Name).To(Equal("deploy")) + }) + + It("falls through to the persisted cache for other workspaces", func() { + overlay := cache.NewLocalExecutableCache(baseExecCache, localWs) + exec, err := overlay.GetExecutableByRef(executable.NewRef("registered/ns:build", "run")) + Expect(err).NotTo(HaveOccurred()) + Expect(exec.Name).To(Equal("build")) + }) + + It("returns not-found for a ref in neither", func() { + overlay := cache.NewLocalExecutableCache(baseExecCache, localWs) + _, err := overlay.GetExecutableByRef(executable.NewRef("worktree/ns:absent", "run")) + Expect(err).To(HaveOccurred()) + }) + + It("lists executables from both the overlay and the persisted cache", func() { + overlay := cache.NewLocalExecutableCache(baseExecCache, localWs) + list, err := overlay.GetExecutableList() + Expect(err).NotTo(HaveOccurred()) + Expect(list.FilterByWorkspace("worktree")).To(HaveLen(1)) + Expect(list.FilterByWorkspace("registered")).To(HaveLen(1)) + }) + + It("shadows a persisted workspace of the same name", func() { + // A clone whose directory name matches an existing workspace resolves to itself, + // so the persisted entries for that name must not leak through. + collide := writeWorkspace("registered", "clone-of-registered", "deploy") + overlay := cache.NewLocalExecutableCache(baseExecCache, collide) + + list, err := overlay.GetExecutableList() + Expect(err).NotTo(HaveOccurred()) + execs := list.FilterByWorkspace("registered") + Expect(execs).To(HaveLen(1)) + Expect(execs[0].Name).To(Equal("deploy")) + Expect(execs[0].WorkspacePath()).To(Equal(collide.Location())) + }) + }) + + Describe("persistence", func() { + It("never writes the discovered workspace into the data store", func() { + overlay := cache.NewLocalExecutableCache(baseExecCache, localWs) + wsOverlay := cache.NewLocalWorkspaceCache(baseWsCache, localWs) + + // Exercise every path that could plausibly persist: reads, then an explicit Update. + _, err := overlay.GetExecutableList() + Expect(err).NotTo(HaveOccurred()) + _, err = wsOverlay.GetLatestData() + Expect(err).NotTo(HaveOccurred()) + Expect(overlay.Update()).To(Succeed()) + Expect(wsOverlay.Update()).To(Succeed()) + + raw, err := ds.GetCacheEntry("executables") + Expect(err).NotTo(HaveOccurred()) + var persisted cache.ExecutableCacheData + Expect(json.Unmarshal(raw, &persisted)).To(Succeed()) + for ref := range persisted.ExecutableMap { + Expect(ref.String()).NotTo(ContainSubstring("worktree/")) + } + for _, info := range persisted.ConfigMap { + Expect(info.WorkspaceName).NotTo(Equal("worktree")) + } + }) + }) + + Describe("workspaces", func() { + It("includes the discovered workspace in the config list", func() { + overlay := cache.NewLocalWorkspaceCache(baseWsCache, localWs) + list, err := overlay.GetWorkspaceConfigList() + Expect(err).NotTo(HaveOccurred()) + Expect(list.FindByName("worktree")).NotTo(BeNil()) + Expect(list.FindByName("registered")).NotTo(BeNil()) + }) + + It("does not mutate the base cache data", func() { + overlay := cache.NewLocalWorkspaceCache(baseWsCache, localWs) + _, err := overlay.GetLatestData() + Expect(err).NotTo(HaveOccurred()) + + base, err := baseWsCache.GetLatestData() + Expect(err).NotTo(HaveOccurred()) + Expect(base.Workspaces).NotTo(HaveKey("worktree")) + }) + }) +}) diff --git a/pkg/cache/templates_cache.go b/pkg/cache/templates_cache.go index 05e104c4..0569da03 100644 --- a/pkg/cache/templates_cache.go +++ b/pkg/cache/templates_cache.go @@ -13,6 +13,7 @@ import ( "github.com/flowexec/flow/v2/pkg/logger" "github.com/flowexec/flow/v2/pkg/store" "github.com/flowexec/flow/v2/types/executable" + "github.com/flowexec/flow/v2/types/workspace" ) const tmplCacheKey = "templates" @@ -43,15 +44,59 @@ type TemplateCacheImpl struct { func NewTemplateCache(wsCache WorkspaceCache, s store.DataStore) TemplateCache { return &TemplateCacheImpl{ - Store: s, - Data: &TemplateCacheData{ - TemplateMap: make(map[string]string), - LocationMap: make(map[string]WorkspaceInfo), - }, + Store: s, + Data: newTemplateCacheData(), WorkspaceCache: wsCache, } } +// newTemplateCacheData returns an empty, ready-to-populate index. +func newTemplateCacheData() *TemplateCacheData { + return &TemplateCacheData{ + TemplateMap: make(map[string]string), + LocationMap: make(map[string]WorkspaceInfo), + } +} + +// indexWorkspaceTemplates records one workspace's valid templates in data. Shared by the +// persisted cache and the in-memory overlay built for a discovered workspace. +func indexWorkspaceTemplates(data *TemplateCacheData, wsCfg *workspace.Workspace) { + templates, err := filesystem.LoadWorkspaceFlowFileTemplates(wsCfg) + if err != nil { + logger.Log().Error("failed to load workspace templates", "workspace", wsCfg.AssignedName(), "err", err) + return + } + for _, tmpl := range templates { + if vErr := tmpl.Validate(); vErr != nil { + logger.Log().Warn( + "invalid template found during cache update", + "template", tmpl.Name(), + "path", tmpl.Location(), + "workspace", wsCfg.AssignedName(), + "err", vErr, + ) + continue + } + + if existingPath, exists := data.TemplateMap[tmpl.Name()]; exists && existingPath != tmpl.Location() { + logger.Log().Warn( + "duplicate template name found during cache update; "+ + "use a workspace/name qualified reference to disambiguate", + "template", tmpl.Name(), + "conflictPath", existingPath, + "newPath", tmpl.Location(), + "workspace", wsCfg.AssignedName(), + ) + } + + data.TemplateMap[tmpl.Name()] = tmpl.Location() + data.LocationMap[tmpl.Location()] = WorkspaceInfo{ + WorkspaceName: wsCfg.AssignedName(), + WorkspacePath: wsCfg.Location(), + } + } +} + func (c *TemplateCacheImpl) Update() error { logger.Log().Debugf("Updating template cache data") wsCacheData, err := c.WorkspaceCache.GetLatestData() @@ -62,40 +107,7 @@ func (c *TemplateCacheImpl) Update() error { cacheData := c.Data for name, wsCfg := range wsCacheData.Workspaces { wsCfg.SetContext(name, wsCacheData.WorkspaceLocations[name]) - templates, err := filesystem.LoadWorkspaceFlowFileTemplates(wsCfg) - if err != nil { - logger.Log().Error("failed to load workspace templates", "workspace", wsCfg.AssignedName(), "err", err) - continue - } - for _, tmpl := range templates { - if vErr := tmpl.Validate(); vErr != nil { - logger.Log().Warn( - "invalid template found during cache update", - "template", tmpl.Name(), - "path", tmpl.Location(), - "workspace", wsCfg.AssignedName(), - "err", vErr, - ) - continue - } - - if existingPath, exists := cacheData.TemplateMap[tmpl.Name()]; exists && existingPath != tmpl.Location() { - logger.Log().Warn( - "duplicate template name found during cache update; "+ - "use a workspace/name qualified reference to disambiguate", - "template", tmpl.Name(), - "conflictPath", existingPath, - "newPath", tmpl.Location(), - "workspace", wsCfg.AssignedName(), - ) - } - - cacheData.TemplateMap[tmpl.Name()] = tmpl.Location() - cacheData.LocationMap[tmpl.Location()] = WorkspaceInfo{ - WorkspaceName: wsCfg.AssignedName(), - WorkspacePath: wsCfg.Location(), - } - } + indexWorkspaceTemplates(cacheData, wsCfg) } data, err := json.Marshal(cacheData) @@ -118,13 +130,18 @@ func (c *TemplateCacheImpl) GetTemplate(name string) (*executable.Template, erro return nil, errors.New("no cached templates found") } - if c.Data.loadedTemplates == nil { - c.Data.loadedTemplates = make(map[string]*executable.Template) - } else if tmpl, found := c.Data.loadedTemplates[name]; found { + return lookupTemplate(c.Data, name) +} + +// lookupTemplate resolves a template reference against an index and loads it. +func lookupTemplate(data *TemplateCacheData, name string) (*executable.Template, error) { + if data.loadedTemplates == nil { + data.loadedTemplates = make(map[string]*executable.Template) + } else if tmpl, found := data.loadedTemplates[name]; found { return tmpl, nil } - path, err := c.resolvePath(name) + path, err := resolveTemplatePath(data, name) if err != nil { return nil, err } @@ -139,17 +156,17 @@ func (c *TemplateCacheImpl) GetTemplate(name string) (*executable.Template, erro if err != nil { return nil, errors.Wrap(err, "unable to load template") } - c.Data.loadedTemplates[name] = tmpl + data.loadedTemplates[name] = tmpl return tmpl, nil } -// resolvePath maps a template reference to a discovered file path. A bare name is looked up -// directly; a "workspace/name" reference is matched against the owning workspace so callers +// resolveTemplatePath maps a template reference to a discovered file path. A bare name is looked +// up directly; a "workspace/name" reference is matched against the owning workspace so callers // can disambiguate the same template name discovered in multiple workspaces. -func (c *TemplateCacheImpl) resolvePath(name string) (string, error) { +func resolveTemplatePath(data *TemplateCacheData, name string) (string, error) { if idx := strings.LastIndex(name, "/"); idx >= 0 { wsName, tmplName := name[:idx], name[idx+1:] - for path, wsInfo := range c.Data.LocationMap { + for path, wsInfo := range data.LocationMap { if wsInfo.WorkspaceName != wsName { continue } @@ -163,7 +180,7 @@ func (c *TemplateCacheImpl) resolvePath(name string) (string, error) { return "", fmt.Errorf("template %s not found", name) } - path, found := c.Data.TemplateMap[name] + path, found := data.TemplateMap[name] if !found { return "", fmt.Errorf("template %s not found", name) } @@ -177,9 +194,14 @@ func (c *TemplateCacheImpl) GetTemplateList() (executable.TemplateList, error) { return nil, errors.New("no cached templates found") } - list := make(executable.TemplateList, 0, len(c.Data.TemplateMap)) - for _, name := range slices.Sorted(maps.Keys(c.Data.TemplateMap)) { - path := c.Data.TemplateMap[name] + return listTemplates(c.Data), nil +} + +// listTemplates returns every template in an index, ordered by name. +func listTemplates(data *TemplateCacheData) executable.TemplateList { + list := make(executable.TemplateList, 0, len(data.TemplateMap)) + for _, name := range slices.Sorted(maps.Keys(data.TemplateMap)) { + path := data.TemplateMap[name] tmpl, err := filesystem.LoadFlowFileTemplate(name, path) if err != nil { logger.Log().Error("unable to load template", "path", path, "err", err) @@ -187,7 +209,7 @@ func (c *TemplateCacheImpl) GetTemplateList() (executable.TemplateList, error) { } list = append(list, tmpl) } - return list, nil + return list } func (c *TemplateCacheImpl) initTemplateCacheData() error { diff --git a/pkg/context/context.go b/pkg/context/context.go index 116a7fee..451dc024 100644 --- a/pkg/context/context.go +++ b/pkg/context/context.go @@ -36,10 +36,15 @@ type Context struct { Config *config.Config CurrentWorkspace *workspace.Workspace - WorkspacesCache cache.WorkspaceCache - ExecutableCache cache.ExecutableCache - TemplateCache cache.TemplateCache - DataStore store.DataStore + + // WorkspaceResolution records how CurrentWorkspace was chosen, including whether it is + // registered in the user config at all. Nil when no workspace could be resolved. + WorkspaceResolution *filesystem.ResolvedWorkspace + + WorkspacesCache cache.WorkspaceCache + ExecutableCache cache.ExecutableCache + TemplateCache cache.TemplateCache + DataStore store.DataStore // RootExecutable is the executable that is being run in the current context. // This will be nil if the context is not associated with an executable run. @@ -86,14 +91,16 @@ func NewContext(ctx context.Context, cancelFunc context.CancelFunc, opts ...Opti // This is a temporary solution until the config handling is refactored a bit _ = os.Setenv(executable.TimeoutOverrideEnv, cfg.DefaultTimeout.String()) } + // Resolution walks up from the working directory, so it succeeds in a workspace the user + // never registered — a worktree or a fresh clone. A nil result is ordinary (a fresh install + // has no workspaces at all) and must not be fatal; commands that need a workspace say so. + resolved, err := filesystem.ResolveWorkspace(cfg, filesystem.ResolveOptions{}) + if err != nil { + panic(errors.Wrap(err, "workspace config load error")) + } var wsConfig *workspace.Workspace - if len(cfg.Workspaces) > 0 { - wsConfig, err = currentWorkspace(cfg) - if err != nil { - panic(errors.Wrap(err, "workspace config load error")) - } else if wsConfig == nil { - panic(fmt.Errorf("workspace config not found in current workspace (%s)", cfg.CurrentWorkspace)) - } + if resolved != nil { + wsConfig = resolved.Workspace } ds, err := store.NewDataStore(store.Path()) @@ -102,23 +109,31 @@ func NewContext(ctx context.Context, cancelFunc context.CancelFunc, opts ...Opti } workspaceCache := cache.NewWorkspaceCache(ds) + // The base workspace cache is what feeds the persisted executable and template caches. A + // discovered workspace is layered on afterwards so it stays out of the shared data store. executableCache := cache.NewExecutableCache(workspaceCache, ds) templateCache := cache.NewTemplateCache(workspaceCache, ds) + if resolved != nil && !resolved.Registered { + workspaceCache = cache.NewLocalWorkspaceCache(workspaceCache, wsConfig) + executableCache = cache.NewLocalExecutableCache(executableCache, wsConfig) + templateCache = cache.NewLocalTemplateCache(templateCache, wsConfig) + } c := &Context{ - appName: "flow", - callbacks: &callbackList{}, - ctx: ctx, - cancelFunc: cancelFunc, - stdOut: os.Stdout, - stdIn: os.Stdin, - stdErr: os.Stderr, - Config: cfg, - CurrentWorkspace: wsConfig, - WorkspacesCache: workspaceCache, - ExecutableCache: executableCache, - TemplateCache: templateCache, - DataStore: ds, + appName: "flow", + callbacks: &callbackList{}, + ctx: ctx, + cancelFunc: cancelFunc, + stdOut: os.Stdout, + stdIn: os.Stdin, + stdErr: os.Stderr, + Config: cfg, + CurrentWorkspace: wsConfig, + WorkspaceResolution: resolved, + WorkspacesCache: workspaceCache, + ExecutableCache: executableCache, + TemplateCache: templateCache, + DataStore: ds, } for _, opt := range opts { opt(c) @@ -135,22 +150,23 @@ func (ctx *Context) ShallowCopy() *Context { appName: ctx.appName, // Shared, not copied: cleanup registered by a parallel branch running on this copy // must reach the root's Finalize. Copying the slice dropped it on the floor. - callbacks: ctx.callbacks, - ctx: ctx.ctx, - cancelFunc: ctx.cancelFunc, - stdOut: ctx.stdOut, - stdIn: ctx.stdIn, - stdErr: ctx.stdErr, - tuiContainer: ctx.tuiContainer, // share already-initialized container (if any) - Config: ctx.Config, - CurrentWorkspace: ctx.CurrentWorkspace, - WorkspacesCache: ctx.WorkspacesCache, - ExecutableCache: ctx.ExecutableCache, - TemplateCache: ctx.TemplateCache, - DataStore: ctx.DataStore, - RootExecutable: ctx.RootExecutable, - ProcessTmpDir: ctx.ProcessTmpDir, - LogArchiveID: ctx.LogArchiveID, + callbacks: ctx.callbacks, + ctx: ctx.ctx, + cancelFunc: ctx.cancelFunc, + stdOut: ctx.stdOut, + stdIn: ctx.stdIn, + stdErr: ctx.stdErr, + tuiContainer: ctx.tuiContainer, // share already-initialized container (if any) + Config: ctx.Config, + CurrentWorkspace: ctx.CurrentWorkspace, + WorkspaceResolution: ctx.WorkspaceResolution, + WorkspacesCache: ctx.WorkspacesCache, + ExecutableCache: ctx.ExecutableCache, + TemplateCache: ctx.TemplateCache, + DataStore: ctx.DataStore, + RootExecutable: ctx.RootExecutable, + ProcessTmpDir: ctx.ProcessTmpDir, + LogArchiveID: ctx.LogArchiveID, } // If the parent has already initialized the TUI container, mark the copy // as initialized too so it won't re-create one. @@ -187,11 +203,11 @@ func (ctx *Context) Value(key any) any { } func (ctx *Context) String() string { - var ws string - if ctx.CurrentWorkspace != nil { - ws = ctx.CurrentWorkspace.AssignedName() + ws := ctx.CurrentWorkspaceName() + var ns string + if ctx.Config != nil { + ns = ctx.Config.CurrentNamespace } - ns := ctx.Config.CurrentNamespace if ws == "" { ws = "unk" } @@ -376,17 +392,70 @@ func ExpandRefFromParent(parent *executable.Executable, ref executable.Ref) exec return executable.NewRef(executable.NewExecutableID(ws, ns, name), ref.Verb()) } -func currentWorkspace(cfg *config.Config) (*workspace.Workspace, error) { - ws, err := cfg.CurrentWorkspaceName() - if err != nil { - return nil, err +// LogWorkspaceResolution records which workspace was chosen and why. It is separate from +// NewContext because the global logger is not initialized yet at construction time — tools that +// build a context outside the CLI (docs generation) never call Init at all. +func (ctx *Context) LogWorkspaceResolution() { + if ctx.WorkspaceResolution == nil { + logger.Log().Debug("no workspace resolved") + return } - wsPath := cfg.Workspaces[ws] - if ws == "" || wsPath == "" { - return nil, fmt.Errorf("current workspace not found") + logger.Log().Debug( + "resolved workspace", + "workspace", ctx.WorkspaceResolution.Name, + "path", ctx.WorkspaceResolution.Path, + "source", string(ctx.WorkspaceResolution.Source), + "registered", ctx.WorkspaceResolution.Registered, + ) +} + +// SetCurrentWorkspace re-points the context at another workspace for the rest of the process, +// rebuilding the caches so an unregistered workspace's executables become resolvable (and a +// registered one stops being shadowed). It does not touch the user config — a per-invocation +// override should not change what the next command does. +func (ctx *Context) SetCurrentWorkspace(resolved *filesystem.ResolvedWorkspace) { + if resolved == nil { + return } + ctx.CurrentWorkspace = resolved.Workspace + ctx.WorkspaceResolution = resolved + + wsCache := cache.NewWorkspaceCache(ctx.DataStore) + execCache := cache.NewExecutableCache(wsCache, ctx.DataStore) + tmplCache := cache.NewTemplateCache(wsCache, ctx.DataStore) + if !resolved.Registered { + wsCache = cache.NewLocalWorkspaceCache(wsCache, resolved.Workspace) + execCache = cache.NewLocalExecutableCache(execCache, resolved.Workspace) + tmplCache = cache.NewLocalTemplateCache(tmplCache, resolved.Workspace) + } + ctx.WorkspacesCache = wsCache + ctx.ExecutableCache = execCache + ctx.TemplateCache = tmplCache +} - return filesystem.LoadWorkspaceConfig(ws, wsPath) +// WorkspaceIsRegistered reports whether the current workspace exists in the user config. A +// workspace discovered by walking up from the working directory does not, which changes what +// commands that mutate the config (workspace switch, remove) can meaningfully do. +func (ctx *Context) WorkspaceIsRegistered() bool { + if ctx.WorkspaceResolution != nil { + return ctx.WorkspaceResolution.Registered + } + // A context assembled without going through NewContext carries no resolution metadata. + // Absent metadata is not evidence of an unregistered workspace, so fall back to the config. + if ctx.Config == nil || ctx.CurrentWorkspace == nil { + return false + } + _, found := ctx.Config.Workspaces[ctx.CurrentWorkspace.AssignedName()] + return found +} + +// CurrentWorkspaceName returns the resolved workspace name, or an empty string when no workspace +// resolved. +func (ctx *Context) CurrentWorkspaceName() string { + if ctx.CurrentWorkspace == nil { + return "" + } + return ctx.CurrentWorkspace.AssignedName() } func overrideThemeColor(theme themes.Theme, palette *config.ColorPalette) themes.Theme { diff --git a/pkg/context/context_test.go b/pkg/context/context_test.go index e69a9f6d..36a189f3 100644 --- a/pkg/context/context_test.go +++ b/pkg/context/context_test.go @@ -12,7 +12,9 @@ import ( "github.com/onsi/ginkgo/v2" . "github.com/onsi/gomega" + "github.com/flowexec/flow/v2/pkg/filesystem" "github.com/flowexec/flow/v2/types/config" + "github.com/flowexec/flow/v2/types/workspace" ) func TestContext(t *testing.T) { @@ -21,18 +23,18 @@ func TestContext(t *testing.T) { } var _ = ginkgo.Describe("Context", func() { - ginkgo.Describe("currentWorkspace", func() { + ginkgo.Describe("workspace resolution", func() { var ( cfg *config.Config tmpDir string ) ginkgo.BeforeEach(func() { - tmpDir = ginkgo.GinkgoT().TempDir() + tmpDir = filesystem.NormalizePath(ginkgo.GinkgoT().TempDir()) cfg = &config.Config{ Workspaces: map[string]string{ - "ws1": filepath.Clean(filepath.Join(tmpDir, "ws1")), - "ws2": filepath.Clean(filepath.Join(tmpDir, "ws2")), + "ws1": filepath.Join(tmpDir, "ws1"), + "ws2": filepath.Join(tmpDir, "ws2"), }, CurrentWorkspace: "ws1", WorkspaceMode: config.ConfigWorkspaceModeFixed, @@ -47,28 +49,67 @@ var _ = ginkgo.Describe("Context", func() { }) ginkgo.It("should return the current workspace in fixed mode", func() { - ws, err := currentWorkspace(cfg) + res, err := filesystem.ResolveWorkspace(cfg, filesystem.ResolveOptions{}) Expect(err).NotTo(HaveOccurred()) - Expect(ws.AssignedName()).To(Equal("ws1")) - Expect(ws.Location()).To(Equal(filepath.Join(tmpDir, "ws1"))) + Expect(res.Workspace.AssignedName()).To(Equal("ws1")) + Expect(res.Workspace.Location()).To(Equal(filepath.Join(tmpDir, "ws1"))) + Expect(res.Registered).To(BeTrue()) }) ginkgo.It("should return the current workspace in dynamic mode", func() { cfg.WorkspaceMode = config.ConfigWorkspaceModeDynamic Expect(os.Mkdir(filepath.Join(tmpDir, "ws2"), 0750)).To(Succeed()) - // os.Setenv("PWD", filepath.Join(tmpDir, "ws2")) Expect(os.Chdir(filepath.Join(tmpDir, "ws2"))).To(Succeed()) - ws, err := currentWorkspace(cfg) + res, err := filesystem.ResolveWorkspace(cfg, filesystem.ResolveOptions{}) Expect(err).NotTo(HaveOccurred()) - Expect(ws.AssignedName()).To(Equal("ws2")) - Expect(ws.Location()).To(Equal(filepath.Join(tmpDir, "ws2"))) + Expect(res.Workspace.AssignedName()).To(Equal("ws2")) + Expect(res.Workspace.Location()).To(Equal(filepath.Join(tmpDir, "ws2"))) }) - ginkgo.It("should return an error if the current workspace is not found", func() { + ginkgo.It("should resolve nothing if the current workspace is not found", func() { cfg.CurrentWorkspace = "ws3" - _, err := currentWorkspace(cfg) - Expect(err).To(HaveOccurred()) + res, err := filesystem.ResolveWorkspace(cfg, filesystem.ResolveOptions{}) + Expect(err).NotTo(HaveOccurred()) + Expect(res).To(BeNil()) + }) + + ginkgo.It("should discover an unregistered workspace by walking up", func() { + cfg.WorkspaceMode = config.ConfigWorkspaceModeDynamic + clone := filepath.Join(tmpDir, "cloned-repo") + Expect(os.MkdirAll(filepath.Join(clone, "sub"), 0750)).To(Succeed()) + Expect(os.WriteFile( + filepath.Join(clone, filesystem.WorkspaceConfigFileName), []byte("{}\n"), 0600, + )).To(Succeed()) + Expect(os.Chdir(filepath.Join(clone, "sub"))).To(Succeed()) + + res, err := filesystem.ResolveWorkspace(cfg, filesystem.ResolveOptions{}) + Expect(err).NotTo(HaveOccurred()) + Expect(res.Workspace.AssignedName()).To(Equal("cloned-repo")) + Expect(res.Registered).To(BeFalse()) + }) + }) + + ginkgo.Describe("workspace accessors", func() { + ginkgo.It("reports an unresolved workspace without panicking", func() { + ctx := &Context{} + Expect(ctx.CurrentWorkspaceName()).To(BeEmpty()) + Expect(ctx.WorkspaceIsRegistered()).To(BeFalse()) + Expect(ctx.String()).To(Equal("unk/*")) + }) + + ginkgo.It("reports a discovered workspace as unregistered", func() { + ws := &workspace.Workspace{} + ws.SetContext("discovered", "/src/discovered") + ctx := &Context{ + CurrentWorkspace: ws, + WorkspaceResolution: &filesystem.ResolvedWorkspace{ + Workspace: ws, Name: "discovered", Path: "/src/discovered", + Registered: false, Source: filesystem.SourceDiscovered, + }, + } + Expect(ctx.CurrentWorkspaceName()).To(Equal("discovered")) + Expect(ctx.WorkspaceIsRegistered()).To(BeFalse()) }) }) diff --git a/pkg/filesystem/discovery.go b/pkg/filesystem/discovery.go new file mode 100644 index 00000000..cad84b1b --- /dev/null +++ b/pkg/filesystem/discovery.go @@ -0,0 +1,103 @@ +package filesystem + +import ( + "path/filepath" + "runtime" + "strings" +) + +// MaxWalkUpDepth bounds the ancestor walk. The volume root normally terminates it; this is a +// backstop against pathological paths (deep symlink cycles resolved into a path, mount loops). +const MaxWalkUpDepth = 128 + +// NormalizePath cleans a path and, on macOS, strips the "/private" prefix. Paths under /tmp, +// /var, and /etc are symlinks into /private there, and the OS hands back either form depending +// on how it was derived — os.Getwd() returns the /private form while a path the user typed +// usually doesn't. Comparisons must normalize both sides or the same directory compares unequal +// to itself. +func NormalizePath(p string) string { + if p == "" { + return "" + } + p = filepath.Clean(p) + if runtime.GOOS == "darwin" { + if p == "/private" { + return "/" + } + if strings.HasPrefix(p, "/private/") { + p = strings.TrimPrefix(p, "/private") + } + } + return p +} + +// SamePath reports whether two paths refer to the same directory. It compares the normalized +// forms first, then the symlink-resolved forms, so a workspace registered through a symlink +// still matches the real path a walk-up produced. +func SamePath(a, b string) bool { + if a == "" || b == "" { + return false + } + if NormalizePath(a) == NormalizePath(b) { + return true + } + ra, err := filepath.EvalSymlinks(a) + if err != nil { + return false + } + rb, err := filepath.EvalSymlinks(b) + if err != nil { + return false + } + return NormalizePath(ra) == NormalizePath(rb) +} + +// IsPathWithin reports whether path is base or lives underneath it. Unlike a raw string prefix +// check, "/a/wsX" is not within "/a/ws". +func IsPathWithin(path, base string) bool { + if path == "" || base == "" { + return false + } + rel, err := filepath.Rel(NormalizePath(base), NormalizePath(path)) + if err != nil { + return false + } + return rel == "." || (!strings.HasPrefix(rel, ".."+string(filepath.Separator)) && rel != "..") +} + +// FindWorkspaceRoot walks up from startDir and returns the first directory containing a +// flow.yaml. This is how a workspace is located without consulting the user config at all — +// the same way make and bazel find their root. +func FindWorkspaceRoot(startDir string) (string, bool) { + return FindWorkspaceRootExcluding(startDir, nil) +} + +// FindWorkspaceRootExcluding is FindWorkspaceRoot but continues walking upward past any +// candidate for which skip returns true. Used to keep a flow.yaml that sits inside a tree an +// ancestor workspace already excludes (vendor/, node_modules/, ...) from becoming a root of +// its own. +func FindWorkspaceRootExcluding(startDir string, skip func(root string) bool) (string, bool) { + if startDir == "" { + return "", false + } + dir := NormalizePath(startDir) + if !filepath.IsAbs(dir) { + abs, err := filepath.Abs(dir) + if err != nil { + return "", false + } + dir = NormalizePath(abs) + } + + for range MaxWalkUpDepth { + if WorkspaceConfigExists(dir) && (skip == nil || !skip(dir)) { + return dir, true + } + parent := filepath.Dir(dir) + if parent == dir { + break + } + dir = parent + } + return "", false +} diff --git a/pkg/filesystem/discovery_test.go b/pkg/filesystem/discovery_test.go new file mode 100644 index 00000000..e3336103 --- /dev/null +++ b/pkg/filesystem/discovery_test.go @@ -0,0 +1,152 @@ +package filesystem_test + +import ( + "os" + "path/filepath" + + . "github.com/onsi/ginkgo/v2" + . "github.com/onsi/gomega" + + "github.com/flowexec/flow/v2/pkg/filesystem" +) + +var _ = Describe("Discovery", func() { + var tmpDir string + + // mkWorkspace creates dir (relative to tmpDir) and marks it a workspace root. + mkWorkspace := func(rel string) string { + path := filepath.Join(tmpDir, rel) + Expect(os.MkdirAll(path, 0750)).To(Succeed()) + Expect(os.WriteFile(filepath.Join(path, filesystem.WorkspaceConfigFileName), []byte("{}\n"), 0600)).To(Succeed()) + return path + } + mkDir := func(rel string) string { + path := filepath.Join(tmpDir, rel) + Expect(os.MkdirAll(path, 0750)).To(Succeed()) + return path + } + + BeforeEach(func() { + var err error + tmpDir, err = os.MkdirTemp("", "flow-discovery-test") + Expect(err).NotTo(HaveOccurred()) + tmpDir, err = filepath.EvalSymlinks(tmpDir) + Expect(err).NotTo(HaveOccurred()) + tmpDir = filesystem.NormalizePath(tmpDir) + }) + + AfterEach(func() { + Expect(os.RemoveAll(tmpDir)).To(Succeed()) + }) + + Describe("FindWorkspaceRoot", func() { + It("finds a flow.yaml in the starting directory", func() { + root := mkWorkspace("ws") + found, ok := filesystem.FindWorkspaceRoot(root) + Expect(ok).To(BeTrue()) + Expect(found).To(Equal(filesystem.NormalizePath(root))) + }) + + It("walks up to an ancestor", func() { + root := mkWorkspace("ws") + deep := mkDir("ws/a/b/c") + found, ok := filesystem.FindWorkspaceRoot(deep) + Expect(ok).To(BeTrue()) + Expect(found).To(Equal(filesystem.NormalizePath(root))) + }) + + It("prefers the closest root when workspaces nest", func() { + mkWorkspace("ws") + nested := mkWorkspace("ws/sub/worktree") + deep := mkDir("ws/sub/worktree/pkg") + found, ok := filesystem.FindWorkspaceRoot(deep) + Expect(ok).To(BeTrue()) + Expect(found).To(Equal(filesystem.NormalizePath(nested))) + }) + + It("returns false when no flow.yaml exists above the directory", func() { + // tmpDir has no flow.yaml, and neither do the system directories above it. + _, ok := filesystem.FindWorkspaceRoot(mkDir("plain/nested")) + Expect(ok).To(BeFalse()) + }) + + It("returns false for an empty start directory", func() { + _, ok := filesystem.FindWorkspaceRoot("") + Expect(ok).To(BeFalse()) + }) + + It("ignores a directory named flow.yaml", func() { + path := filepath.Join(tmpDir, "notaws") + Expect(os.MkdirAll(filepath.Join(path, filesystem.WorkspaceConfigFileName), 0750)).To(Succeed()) + _, ok := filesystem.FindWorkspaceRoot(path) + Expect(ok).To(BeFalse()) + }) + + It("walks past candidates the skip function rejects", func() { + outer := mkWorkspace("ws") + inner := mkWorkspace("ws/vendor/dep") + found, ok := filesystem.FindWorkspaceRootExcluding(inner, func(root string) bool { + return filepath.Base(filepath.Dir(root)) == "vendor" + }) + Expect(ok).To(BeTrue()) + Expect(found).To(Equal(filesystem.NormalizePath(outer))) + }) + }) + + Describe("IsPathWithin", func() { + It("matches a directory against itself and its descendants", func() { + Expect(filesystem.IsPathWithin("/a/ws", "/a/ws")).To(BeTrue()) + Expect(filesystem.IsPathWithin("/a/ws/pkg/x", "/a/ws")).To(BeTrue()) + }) + + It("does not match a sibling that shares a name prefix", func() { + Expect(filesystem.IsPathWithin("/a/wsX", "/a/ws")).To(BeFalse()) + Expect(filesystem.IsPathWithin("/a", "/a/ws")).To(BeFalse()) + }) + + It("is false for empty operands", func() { + Expect(filesystem.IsPathWithin("", "/a")).To(BeFalse()) + Expect(filesystem.IsPathWithin("/a", "")).To(BeFalse()) + }) + }) + + Describe("SamePath", func() { + It("matches a symlinked path to its target", func() { + target := mkDir("real") + link := filepath.Join(tmpDir, "link") + Expect(os.Symlink(target, link)).To(Succeed()) + Expect(filesystem.SamePath(link, target)).To(BeTrue()) + }) + + It("does not match distinct directories", func() { + Expect(filesystem.SamePath(mkDir("a"), mkDir("b"))).To(BeFalse()) + }) + }) + + Describe("ReadWorkspaceConfig", func() { + It("does not create anything when the file is missing", func() { + path := filepath.Join(tmpDir, "absent") + _, err := filesystem.ReadWorkspaceConfig("absent", path) + Expect(err).To(HaveOccurred()) + _, statErr := os.Stat(path) + Expect(os.IsNotExist(statErr)).To(BeTrue(), "must not create the workspace directory") + }) + + It("reads an existing config and attaches the given name and location", func() { + root := mkWorkspace("ws") + cfg, err := filesystem.ReadWorkspaceConfig("custom", root) + Expect(err).NotTo(HaveOccurred()) + Expect(cfg.AssignedName()).To(Equal("custom")) + Expect(cfg.Location()).To(Equal(root)) + }) + + It("accepts an empty flow.yaml as a valid workspace marker", func() { + path := filepath.Join(tmpDir, "empty") + Expect(os.MkdirAll(path, 0750)).To(Succeed()) + Expect(os.WriteFile(filepath.Join(path, filesystem.WorkspaceConfigFileName), nil, 0600)).To(Succeed()) + cfg, err := filesystem.ReadWorkspaceConfig("empty", path) + Expect(err).NotTo(HaveOccurred()) + Expect(cfg.AssignedName()).To(Equal("empty")) + }) + }) +}) diff --git a/pkg/filesystem/executables.go b/pkg/filesystem/executables.go index efd5c786..6370136b 100644 --- a/pkg/filesystem/executables.go +++ b/pkg/filesystem/executables.go @@ -141,6 +141,7 @@ func findFiles( } excludedPaths = append(excludedPaths, defaultExcutablePaths...) + root := workspaceCfg.Location() var cfgPaths []string walkDirFunc := func(path string, entry fs.DirEntry, err error) error { if err != nil { @@ -150,6 +151,14 @@ func findFiles( } return err } + if isNestedWorkspaceRoot(path, root, entry, filter) { + logger.Log().Debug( + "skipping nested workspace root", + "path", path, + "workspace", workspaceCfg.AssignedName(), + ) + return filepath.SkipDir + } if isPathIncluded(path, workspaceCfg.Location(), includePaths) { if isPathExcluded(path, workspaceCfg.Location(), excludedPaths) { if entry.IsDir() { @@ -171,6 +180,27 @@ func findFiles( return cfgPaths, nil } +// isNestedWorkspaceRoot reports whether path is a subdirectory that holds its own flow.yaml. +// Such a directory is a workspace in its own right — a git worktree, a submodule, an embedded +// example project — and its executables belong to it, not to the tree being scanned. Without +// this, a worktree checked out inside a workspace gets indexed twice and every one of its +// executables collides with the original. +// +// A directory named explicitly in the workspace's `executables.included` is scanned anyway: that +// is a deliberate instruction to pull those executables in, and it should outrank the default. +func isNestedWorkspaceRoot(path, root string, entry fs.DirEntry, filter *workspace.ExecutableFilter) bool { + if !entry.IsDir() || filepath.Clean(path) == filepath.Clean(root) { + return false + } + if !WorkspaceConfigExists(path) { + return false + } + if filter != nil && len(filter.Included) > 0 && pathMatches(path, root, filter.Included) { + return false + } + return true +} + func pathMatches(path, basePath string, patterns []string) bool { if len(patterns) == 0 { return false diff --git a/pkg/filesystem/executables_test.go b/pkg/filesystem/executables_test.go index e681574b..11c76951 100644 --- a/pkg/filesystem/executables_test.go +++ b/pkg/filesystem/executables_test.go @@ -153,5 +153,53 @@ var _ = Describe("Executables", func() { Expect(err).NotTo(HaveOccurred()) Expect(definitions).To(BeEmpty()) }) + + Context("with a nested workspace", func() { + // nestedSetup writes one flow file at the workspace root and another inside a + // subdirectory that is itself a workspace root (a git worktree, say). + nestedSetup := func() string { + def := &executable.FlowFile{ + Namespace: "test", + Executables: executable.ExecutableList{{Verb: "exec", Name: "test-executable"}}, + } + Expect(filesystem.WriteFlowFile(filepath.Join(tmpDir, "root"+executable.FlowFileExt), def)).To(Succeed()) + + nested := filepath.Join(tmpDir, "worktree") + Expect(os.MkdirAll(nested, 0750)).To(Succeed()) + Expect(os.WriteFile( + filepath.Join(nested, filesystem.WorkspaceConfigFileName), []byte("{}\n"), 0600, + )).To(Succeed()) + Expect(filesystem.WriteFlowFile(filepath.Join(nested, "nested"+executable.FlowFileExt), def)).To(Succeed()) + + ctrl := gomock.NewController(GinkgoT()) + mockLogger := mocks.NewMockLogger(ctrl) + logger.Init(logger.InitOptions{Logger: mockLogger, TestingTB: GinkgoTB()}) + mockLogger.EXPECT().Debug(gomock.Any(), gomock.Any()).AnyTimes() + return nested + } + + It("does not load executables from a subdirectory that is its own workspace", func() { + nestedSetup() + workspaceCfg := &workspace.Workspace{} + workspaceCfg.SetContext("test", tmpDir) + + definitions, err := filesystem.LoadWorkspaceFlowFiles(workspaceCfg) + Expect(err).NotTo(HaveOccurred()) + Expect(definitions).To(HaveLen(1)) + Expect(definitions[0].ConfigPath()).To(Equal(filepath.Join(tmpDir, "root"+executable.FlowFileExt))) + }) + + It("still loads them when the nested path is explicitly included", func() { + nested := nestedSetup() + workspaceCfg := &workspace.Workspace{ + Executables: &workspace.ExecutableFilter{Included: []string{tmpDir, nested}}, + } + workspaceCfg.SetContext("test", tmpDir) + + definitions, err := filesystem.LoadWorkspaceFlowFiles(workspaceCfg) + Expect(err).NotTo(HaveOccurred()) + Expect(definitions).To(HaveLen(2)) + }) + }) }) }) diff --git a/pkg/filesystem/resolve.go b/pkg/filesystem/resolve.go new file mode 100644 index 00000000..2b916ef3 --- /dev/null +++ b/pkg/filesystem/resolve.go @@ -0,0 +1,239 @@ +package filesystem + +import ( + "fmt" + "os" + "path/filepath" + "slices" + "strings" + + "github.com/flowexec/flow/v2/types/config" + "github.com/flowexec/flow/v2/types/workspace" +) + +// WorkspaceOverrideEnv pins the workspace for a single invocation, by registered name or by +// path. Exporting it beats threading a flag through every call, which matters most for agents +// and CI where the caller can set the environment once but can't remember a flag every time. +const WorkspaceOverrideEnv = "FLOW_WORKSPACE" + +// WorkspaceSource records how a workspace was resolved, so callers can explain the choice +// (`flow workspace get`, MCP get_info) instead of leaving the user to guess. +type WorkspaceSource string + +const ( + // SourceOverride: named explicitly via --workspace or FLOW_WORKSPACE. + SourceOverride WorkspaceSource = "override" + // SourceRegistered: walking up from the working directory found a registered workspace root. + SourceRegistered WorkspaceSource = "registered" + // SourceDiscovered: walking up found a flow.yaml that is not registered anywhere. + SourceDiscovered WorkspaceSource = "discovered" + // SourcePrefix: no flow.yaml above the working directory, but it sits inside a registered + // workspace's tree. + SourcePrefix WorkspaceSource = "prefix" + // SourceCurrent: fell back to the workspace persisted in the user config. + SourceCurrent WorkspaceSource = "current" +) + +// ResolvedWorkspace is the workspace a command should operate in, plus how it was chosen. +// Registered is false for a workspace found only by walking up from the working directory — +// it is valid to run in, but it exists nowhere in the user config and is never persisted. +type ResolvedWorkspace struct { + Workspace *workspace.Workspace + Name string + Path string + Registered bool + Source WorkspaceSource +} + +// ResolveOptions tunes workspace resolution. The zero value resolves from the process working +// directory with no override, which is what almost every caller wants. +type ResolveOptions struct { + // Dir is the directory to resolve from. Defaults to the process working directory. + Dir string + // Override is an explicit workspace name or path (a --workspace flag value). When empty, + // WorkspaceOverrideEnv is consulted instead. + Override string +} + +// discoveryBoundaryDirs are directory names whose contents are copies of, or dependencies of, +// some other project. A flow.yaml inside one of them belongs to that copy, not to the tree the +// user is working in, so discovery walks past it rather than adopting it as a root. These mirror +// the paths executable discovery already refuses to scan. +var discoveryBoundaryDirs = []string{ + ".git", + ".claude", + "vendor", + "third_party", + "external", + "node_modules", +} + +// ResolveWorkspace picks the workspace for a directory. +// +// Precedence: an explicit override always wins; otherwise, in dynamic mode, the nearest +// flow.yaml at or above dir wins — the same way make and bazel locate their root — falling back +// to a registered workspace containing dir and finally to the workspace persisted in the config. +// Fixed mode skips discovery entirely, because pinning a workspace and then auto-switching on +// every cd would defeat the point of pinning it. +// +// Returns (nil, nil) when nothing resolves. That is an ordinary state — a fresh install has no +// workspaces at all — so callers must handle a nil result rather than treating it as an error. +// A sentinel error would invert that: every caller would have to remember to special-case it, +// and forgetting once turns "no workspace yet" into a fatal. +// +//nolint:nilnil +func ResolveWorkspace(cfg *config.Config, opts ResolveOptions) (*ResolvedWorkspace, error) { + if cfg == nil { + return nil, nil + } + + dir := opts.Dir + if dir == "" { + // A working directory that no longer exists is not an error here; resolution just falls + // through to the config-based paths below. + if wd, err := os.Getwd(); err == nil { + dir = wd + } + } + if dir != "" { + if abs, err := filepath.Abs(dir); err == nil { + dir = abs + } + dir = NormalizePath(dir) + } + + override := opts.Override + if override == "" { + override = os.Getenv(WorkspaceOverrideEnv) + } + if override != "" { + return resolveOverride(cfg, override) + } + + if cfg.WorkspaceMode == config.ConfigWorkspaceModeDynamic && dir != "" { + if root, found := FindWorkspaceRootExcluding(dir, discoveryBoundary(cfg)); found { + if name, registered := cfg.NameForWorkspacePath(root); registered { + return loadResolved(name, cfg.Workspaces[name], SourceRegistered) + } + return readResolved(root, SourceDiscovered) + } + + // No flow.yaml anywhere above dir. A registered workspace may still claim this path — + // its own flow.yaml could have been deleted, or the workspace root may be unreachable. + if name, found := cfg.WorkspaceForPath(dir); found { + return loadResolved(name, cfg.Workspaces[name], SourcePrefix) + } + } + + if path, found := cfg.Workspaces[cfg.CurrentWorkspace]; found && path != "" { + return loadResolved(cfg.CurrentWorkspace, path, SourceCurrent) + } + + return nil, nil +} + +// resolveOverride handles an explicit --workspace / FLOW_WORKSPACE value, which may be either a +// registered name or a path to any directory holding a flow.yaml. Overrides are honored in fixed +// mode too — they are how a fixed-mode user opts into a different workspace for one command. +func resolveOverride(cfg *config.Config, override string) (*ResolvedWorkspace, error) { + if path, found := cfg.Workspaces[override]; found && path != "" { + return loadResolved(override, path, SourceOverride) + } + + if !looksLikePath(override) { + return nil, fmt.Errorf( + "unknown workspace %q - not a registered workspace name, and not a path to a directory with a %s", + override, WorkspaceConfigFileName, + ) + } + + path, err := expandOverridePath(override) + if err != nil { + return nil, err + } + if !WorkspaceConfigExists(path) { + return nil, fmt.Errorf("no %s found in %s", WorkspaceConfigFileName, path) + } + if name, found := cfg.NameForWorkspacePath(path); found { + return loadResolved(name, cfg.Workspaces[name], SourceOverride) + } + return readResolved(path, SourceOverride) +} + +func looksLikePath(v string) bool { + return v == "." || v == ".." || + filepath.IsAbs(v) || + strings.HasPrefix(v, "~") || + strings.ContainsRune(v, filepath.Separator) || + strings.ContainsRune(v, '/') +} + +func expandOverridePath(v string) (string, error) { + if strings.HasPrefix(v, "~") { + home, err := os.UserHomeDir() + if err != nil { + return "", fmt.Errorf("unable to expand %q - %w", v, err) + } + v = filepath.Join(home, strings.TrimPrefix(strings.TrimPrefix(v, "~"), "/")) + } + abs, err := filepath.Abs(v) + if err != nil { + return "", fmt.Errorf("unable to resolve workspace path %q - %w", v, err) + } + return NormalizePath(abs), nil +} + +// discoveryBoundary rejects candidate roots that a registered workspace already refuses to scan +// — a vendored dependency or a repo copy checked out inside it. Without this, `cd vendor/somelib` +// would silently make that dependency the workspace. +// +// The check applies only *below* a registered workspace root. A standalone clone that happens to +// live in ~/external/ is a perfectly good workspace; only a copy sitting inside a workspace that +// deliberately excludes it is not. +func discoveryBoundary(cfg *config.Config) func(string) bool { + return func(root string) bool { + // An explicitly registered path is always a legitimate root, wherever it lives. + if _, registered := cfg.NameForWorkspacePath(root); registered { + return false + } + parent, found := cfg.WorkspaceForPath(root) + if !found { + return false + } + rel, err := filepath.Rel(NormalizePath(cfg.Workspaces[parent]), root) + if err != nil { + return false + } + for _, segment := range strings.Split(filepath.ToSlash(rel), "/") { + if slices.Contains(discoveryBoundaryDirs, segment) { + return true + } + } + return false + } +} + +// loadResolved builds a result for a registered workspace, creating its flow.yaml if the user +// registered a path that does not have one yet. +func loadResolved(name, path string, src WorkspaceSource) (*ResolvedWorkspace, error) { + ws, err := LoadWorkspaceConfig(name, path) + if err != nil { + return nil, err + } + return &ResolvedWorkspace{ + Workspace: ws, Name: name, Path: NormalizePath(path), Registered: true, Source: src, + }, nil +} + +// readResolved builds a result for a workspace found on disk but absent from the user config, +// reading its flow.yaml without writing anything into a directory the user never registered. +func readResolved(path string, src WorkspaceSource) (*ResolvedWorkspace, error) { + name := filepath.Base(path) + ws, err := ReadWorkspaceConfig(name, path) + if err != nil { + return nil, err + } + return &ResolvedWorkspace{ + Workspace: ws, Name: name, Path: path, Registered: false, Source: src, + }, nil +} diff --git a/pkg/filesystem/resolve_test.go b/pkg/filesystem/resolve_test.go new file mode 100644 index 00000000..eb8727c0 --- /dev/null +++ b/pkg/filesystem/resolve_test.go @@ -0,0 +1,229 @@ +package filesystem_test + +import ( + "os" + "path/filepath" + + . "github.com/onsi/ginkgo/v2" + . "github.com/onsi/gomega" + + "github.com/flowexec/flow/v2/pkg/filesystem" + "github.com/flowexec/flow/v2/types/config" +) + +var _ = Describe("ResolveWorkspace", func() { + var tmpDir string + + mkWorkspace := func(rel string) string { + path := filepath.Join(tmpDir, rel) + Expect(os.MkdirAll(path, 0750)).To(Succeed()) + Expect(os.WriteFile(filepath.Join(path, filesystem.WorkspaceConfigFileName), []byte("{}\n"), 0600)).To(Succeed()) + return path + } + mkDir := func(rel string) string { + path := filepath.Join(tmpDir, rel) + Expect(os.MkdirAll(path, 0750)).To(Succeed()) + return path + } + dynamic := func(workspaces map[string]string, current string) *config.Config { + return &config.Config{ + Workspaces: workspaces, + CurrentWorkspace: current, + WorkspaceMode: config.ConfigWorkspaceModeDynamic, + } + } + + BeforeEach(func() { + var err error + tmpDir, err = os.MkdirTemp("", "flow-resolve-test") + Expect(err).NotTo(HaveOccurred()) + tmpDir, err = filepath.EvalSymlinks(tmpDir) + Expect(err).NotTo(HaveOccurred()) + // Resolution reports normalized paths; normalize the fixture root so the two forms of a + // macOS temp dir (/private/var/... and /var/...) don't make every comparison fail. + tmpDir = filesystem.NormalizePath(tmpDir) + Expect(os.Unsetenv(filesystem.WorkspaceOverrideEnv)).To(Succeed()) + }) + + AfterEach(func() { + Expect(os.RemoveAll(tmpDir)).To(Succeed()) + Expect(os.Unsetenv(filesystem.WorkspaceOverrideEnv)).To(Succeed()) + }) + + Context("discovery", func() { + It("resolves an unregistered root by walking up, named for its directory", func() { + root := mkWorkspace("cloned-repo") + res, err := filesystem.ResolveWorkspace(dynamic(nil, ""), filesystem.ResolveOptions{ + Dir: mkDir("cloned-repo/internal/pkg"), + }) + Expect(err).NotTo(HaveOccurred()) + Expect(res).NotTo(BeNil()) + Expect(res.Name).To(Equal("cloned-repo")) + Expect(res.Path).To(Equal(root)) + Expect(res.Registered).To(BeFalse()) + Expect(res.Source).To(Equal(filesystem.SourceDiscovered)) + }) + + It("uses the registered name when the discovered root is registered", func() { + root := mkWorkspace("checkout") + cfg := dynamic(map[string]string{"myproject": root}, "myproject") + res, err := filesystem.ResolveWorkspace(cfg, filesystem.ResolveOptions{Dir: mkDir("checkout/sub")}) + Expect(err).NotTo(HaveOccurred()) + Expect(res.Name).To(Equal("myproject"), "registered name wins over the directory basename") + Expect(res.Registered).To(BeTrue()) + Expect(res.Source).To(Equal(filesystem.SourceRegistered)) + }) + + It("prefers a nested worktree over its registered parent workspace", func() { + parent := mkWorkspace("repo") + worktree := mkWorkspace("repo/.worktrees/feature") + cfg := dynamic(map[string]string{"repo": parent}, "repo") + res, err := filesystem.ResolveWorkspace(cfg, filesystem.ResolveOptions{ + Dir: mkDir("repo/.worktrees/feature/cmd"), + }) + Expect(err).NotTo(HaveOccurred()) + Expect(res.Name).To(Equal("feature")) + Expect(res.Path).To(Equal(worktree)) + Expect(res.Registered).To(BeFalse()) + }) + + It("resolves a worktree that lives outside every registered workspace", func() { + registered := mkWorkspace("repo") + worktree := mkWorkspace("elsewhere/repo-feature") + cfg := dynamic(map[string]string{"repo": registered}, "repo") + res, err := filesystem.ResolveWorkspace(cfg, filesystem.ResolveOptions{Dir: worktree}) + Expect(err).NotTo(HaveOccurred()) + Expect(res.Name).To(Equal("repo-feature")) + Expect(res.Registered).To(BeFalse()) + }) + + It("walks past a vendored copy inside a registered workspace", func() { + parent := mkWorkspace("repo") + mkWorkspace("repo/vendor/dep") + cfg := dynamic(map[string]string{"repo": parent}, "repo") + res, err := filesystem.ResolveWorkspace(cfg, filesystem.ResolveOptions{Dir: mkDir("repo/vendor/dep")}) + Expect(err).NotTo(HaveOccurred()) + Expect(res.Name).To(Equal("repo")) + Expect(res.Path).To(Equal(parent)) + }) + + It("still discovers a standalone clone under a directory named external", func() { + // The boundary only applies inside a registered workspace; an unrelated path that + // happens to contain "external" is a legitimate root. + root := mkWorkspace("external/standalone") + res, err := filesystem.ResolveWorkspace(dynamic(nil, ""), filesystem.ResolveOptions{Dir: root}) + Expect(err).NotTo(HaveOccurred()) + Expect(res.Name).To(Equal("standalone")) + }) + + It("falls back to a registered workspace containing the directory", func() { + // Registered, but its flow.yaml is gone, so there is nothing to walk up to. + root := mkDir("bare") + cfg := dynamic(map[string]string{"bare": root}, "bare") + res, err := filesystem.ResolveWorkspace(cfg, filesystem.ResolveOptions{Dir: mkDir("bare/sub")}) + Expect(err).NotTo(HaveOccurred()) + Expect(res.Name).To(Equal("bare")) + Expect(res.Source).To(Equal(filesystem.SourcePrefix)) + }) + + It("falls back to the configured current workspace outside any workspace", func() { + root := mkWorkspace("home-ws") + cfg := dynamic(map[string]string{"home-ws": root}, "home-ws") + res, err := filesystem.ResolveWorkspace(cfg, filesystem.ResolveOptions{Dir: mkDir("unrelated")}) + Expect(err).NotTo(HaveOccurred()) + Expect(res.Name).To(Equal("home-ws")) + Expect(res.Source).To(Equal(filesystem.SourceCurrent)) + }) + + It("returns nil when nothing resolves", func() { + res, err := filesystem.ResolveWorkspace(dynamic(nil, ""), filesystem.ResolveOptions{ + Dir: mkDir("nowhere"), + }) + Expect(err).NotTo(HaveOccurred()) + Expect(res).To(BeNil()) + }) + }) + + Context("fixed mode", func() { + It("ignores discovery and uses the configured workspace", func() { + pinned := mkWorkspace("pinned") + mkWorkspace("other") + cfg := &config.Config{ + Workspaces: map[string]string{"pinned": pinned}, + CurrentWorkspace: "pinned", + WorkspaceMode: config.ConfigWorkspaceModeFixed, + } + res, err := filesystem.ResolveWorkspace(cfg, filesystem.ResolveOptions{Dir: filepath.Join(tmpDir, "other")}) + Expect(err).NotTo(HaveOccurred()) + Expect(res.Name).To(Equal("pinned")) + Expect(res.Source).To(Equal(filesystem.SourceCurrent)) + }) + + It("still honors an explicit override", func() { + pinned := mkWorkspace("pinned") + other := mkWorkspace("other") + cfg := &config.Config{ + Workspaces: map[string]string{"pinned": pinned}, + CurrentWorkspace: "pinned", + WorkspaceMode: config.ConfigWorkspaceModeFixed, + } + res, err := filesystem.ResolveWorkspace(cfg, filesystem.ResolveOptions{Override: other}) + Expect(err).NotTo(HaveOccurred()) + Expect(res.Name).To(Equal("other")) + Expect(res.Source).To(Equal(filesystem.SourceOverride)) + }) + }) + + Context("override", func() { + It("accepts a registered workspace name", func() { + root := mkWorkspace("ws") + cfg := dynamic(map[string]string{"named": root}, "named") + res, err := filesystem.ResolveWorkspace(cfg, filesystem.ResolveOptions{ + Override: "named", Dir: mkDir("unrelated"), + }) + Expect(err).NotTo(HaveOccurred()) + Expect(res.Name).To(Equal("named")) + Expect(res.Registered).To(BeTrue()) + }) + + It("accepts an absolute path to an unregistered workspace", func() { + root := mkWorkspace("adhoc") + res, err := filesystem.ResolveWorkspace(dynamic(nil, ""), filesystem.ResolveOptions{Override: root}) + Expect(err).NotTo(HaveOccurred()) + Expect(res.Name).To(Equal("adhoc")) + Expect(res.Registered).To(BeFalse()) + Expect(res.Source).To(Equal(filesystem.SourceOverride)) + }) + + It("maps a path that is registered back to its registered name", func() { + root := mkWorkspace("dir-name") + cfg := dynamic(map[string]string{"registered-name": root}, "registered-name") + res, err := filesystem.ResolveWorkspace(cfg, filesystem.ResolveOptions{Override: root}) + Expect(err).NotTo(HaveOccurred()) + Expect(res.Name).To(Equal("registered-name")) + Expect(res.Registered).To(BeTrue()) + }) + + It("reads the override from the environment when no value is passed", func() { + root := mkWorkspace("env-ws") + Expect(os.Setenv(filesystem.WorkspaceOverrideEnv, root)).To(Succeed()) + res, err := filesystem.ResolveWorkspace(dynamic(nil, ""), filesystem.ResolveOptions{ + Dir: mkDir("unrelated"), + }) + Expect(err).NotTo(HaveOccurred()) + Expect(res.Name).To(Equal("env-ws")) + }) + + It("errors on an unknown name", func() { + _, err := filesystem.ResolveWorkspace(dynamic(nil, ""), filesystem.ResolveOptions{Override: "nope"}) + Expect(err).To(MatchError(ContainSubstring("unknown workspace"))) + }) + + It("errors on a path with no flow.yaml", func() { + _, err := filesystem.ResolveWorkspace(dynamic(nil, ""), filesystem.ResolveOptions{ + Override: mkDir("plain"), + }) + Expect(err).To(MatchError(ContainSubstring("no flow.yaml found"))) + }) + }) +}) diff --git a/pkg/filesystem/workspace.go b/pkg/filesystem/workspace.go index ec7c46bd..6afdc2b8 100644 --- a/pkg/filesystem/workspace.go +++ b/pkg/filesystem/workspace.go @@ -1,6 +1,7 @@ package filesystem import ( + "io" "os" "path/filepath" @@ -25,9 +26,15 @@ func InitWorkspaceConfig(name, path string) error { return nil } +// WorkspaceConfigExists reports whether workspacePath holds a flow.yaml file. Workspace +// discovery walks up through arbitrary directories testing this, so a directory that merely +// happens to be named flow.yaml must not count as a workspace root. func WorkspaceConfigExists(workspacePath string) bool { - _, err := os.Stat(filepath.Join(workspacePath, WorkspaceConfigFileName)) - return !os.IsNotExist(err) + info, err := os.Stat(filepath.Join(workspacePath, WorkspaceConfigFileName)) + if err != nil { + return false + } + return !info.IsDir() } func EnsureWorkspaceDir(workspacePath string) error { @@ -71,13 +78,22 @@ func WriteWorkspaceConfig(workspacePath string, config *workspace.Workspace) err return nil } +// LoadWorkspaceConfig reads a registered workspace's config, creating the directory and a +// default flow.yaml if either is missing. Only use this for paths the user has explicitly +// registered — see ReadWorkspaceConfig for the read-only variant discovery must use. func LoadWorkspaceConfig(workspaceName, workspacePath string) (*workspace.Workspace, error) { if err := EnsureWorkspaceDir(workspacePath); err != nil { return nil, errors.Wrap(err, "unable to ensure workspace directory") } else if err := EnsureWorkspaceConfig(workspaceName, workspacePath); err != nil { return nil, errors.Wrap(err, "unable to ensure workspace config file") } + return ReadWorkspaceConfig(workspaceName, workspacePath) +} +// ReadWorkspaceConfig reads a workspace's flow.yaml without creating anything. Workspace +// discovery resolves roots the user never registered, so it must never write into them the way +// LoadWorkspaceConfig does. +func ReadWorkspaceConfig(workspaceName, workspacePath string) (*workspace.Workspace, error) { wsCfg := &workspace.Workspace{} wsFile := filepath.Join(workspacePath, WorkspaceConfigFileName) file, err := os.Open(filepath.Clean(wsFile)) @@ -87,7 +103,8 @@ func LoadWorkspaceConfig(workspaceName, workspacePath string) (*workspace.Worksp defer file.Close() err = yaml.NewDecoder(file).Decode(wsCfg) - if err != nil { + if err != nil && !errors.Is(err, io.EOF) { + // An empty flow.yaml is a valid workspace marker with all-default settings. return nil, errors.Wrap(err, "unable to decode workspace config file") } diff --git a/tests/discovery_e2e_test.go b/tests/discovery_e2e_test.go new file mode 100644 index 00000000..66a5ddcd --- /dev/null +++ b/tests/discovery_e2e_test.go @@ -0,0 +1,142 @@ +//go:build e2e + +package tests_test + +import ( + stdCtx "context" + "os" + "path/filepath" + + . "github.com/onsi/ginkgo/v2" + . "github.com/onsi/gomega" + + "github.com/flowexec/flow/v2/pkg/filesystem" + "github.com/flowexec/flow/v2/tests/utils" + "github.com/flowexec/flow/v2/types/config" +) + +// These specs build their context through context.NewContext from a real working directory, +// which is the only way to exercise workspace discovery. That changes the process working +// directory, hence Serial. +var _ = Describe("workspace discovery e2e", Serial, func() { + var root string + + dynamicCtx := func(dir string, registered map[string]string) *utils.DiscoveryContext { + return utils.NewDiscoveryContext( + stdCtx.Background(), GinkgoTB(), dir, registered, config.ConfigWorkspaceModeDynamic, + ) + } + + BeforeEach(func() { + root = filesystem.NormalizePath(GinkgoTB().TempDir()) + }) + + It("runs in a freshly cloned repo with no workspaces registered at all", func() { + clone := utils.WriteWorkspace(GinkgoTB(), filepath.Join(root, "cloned-repo"), "greet") + nested := filepath.Join(clone, "internal", "pkg") + Expect(os.MkdirAll(nested, 0750)).To(Succeed()) + + ctx := dynamicCtx(nested, nil) + + Expect(ctx.CurrentWorkspaceName()).To(Equal("cloned-repo")) + Expect(ctx.WorkspaceIsRegistered()).To(BeFalse()) + Expect(ctx.CurrentWorkspace.Location()).To(Equal(clone)) + + list, err := ctx.ExecutableCache.GetExecutableList() + Expect(err).NotTo(HaveOccurred()) + Expect(list.FilterByWorkspace("cloned-repo")).To(HaveLen(1)) + }) + + It("prefers a worktree nested inside a registered workspace", func() { + parent := utils.WriteWorkspace(GinkgoTB(), filepath.Join(root, "repo"), "build") + worktree := utils.WriteWorkspace(GinkgoTB(), filepath.Join(parent, ".worktrees", "feature"), "deploy") + + ctx := dynamicCtx(worktree, map[string]string{"repo": parent}) + + Expect(ctx.CurrentWorkspaceName()).To(Equal("feature")) + Expect(ctx.WorkspaceIsRegistered()).To(BeFalse()) + + list, err := ctx.ExecutableCache.GetExecutableList() + Expect(err).NotTo(HaveOccurred()) + Expect(list.FilterByWorkspace("feature")).To(HaveLen(1)) + }) + + It("does not index a nested worktree's executables into its parent workspace", func() { + parent := utils.WriteWorkspace(GinkgoTB(), filepath.Join(root, "repo"), "build") + utils.WriteWorkspace(GinkgoTB(), filepath.Join(parent, ".worktrees", "feature"), "deploy") + + ctx := dynamicCtx(parent, map[string]string{"repo": parent}) + Expect(ctx.CurrentWorkspaceName()).To(Equal("repo")) + Expect(ctx.ExecutableCache.Update()).To(Succeed()) + + list, err := ctx.ExecutableCache.GetExecutableList() + Expect(err).NotTo(HaveOccurred()) + execs := list.FilterByWorkspace("repo") + Expect(execs).To(HaveLen(1)) + Expect(execs[0].Name).To(Equal("build")) + }) + + It("resolves a worktree that lives outside every registered workspace", func() { + registered := utils.WriteWorkspace(GinkgoTB(), filepath.Join(root, "repo"), "build") + worktree := utils.WriteWorkspace(GinkgoTB(), filepath.Join(root, "worktrees", "repo-feature"), "deploy") + + ctx := dynamicCtx(worktree, map[string]string{"repo": registered}) + Expect(ctx.CurrentWorkspaceName()).To(Equal("repo-feature")) + Expect(ctx.WorkspaceIsRegistered()).To(BeFalse()) + }) + + It("keeps the registered name when the directory basename differs", func() { + dir := utils.WriteWorkspace(GinkgoTB(), filepath.Join(root, "dir-name"), "build") + + ctx := dynamicCtx(dir, map[string]string{"registered-name": dir}) + Expect(ctx.CurrentWorkspaceName()).To(Equal("registered-name")) + Expect(ctx.WorkspaceIsRegistered()).To(BeTrue()) + }) + + It("never writes a discovered workspace to the user config or the shared cache", func() { + clone := utils.WriteWorkspace(GinkgoTB(), filepath.Join(root, "throwaway"), "greet") + registered := utils.WriteWorkspace(GinkgoTB(), filepath.Join(root, "kept"), "build") + + ctx := dynamicCtx(clone, map[string]string{"kept": registered}) + Expect(ctx.CurrentWorkspaceName()).To(Equal("throwaway")) + + // Exercise the paths that populate and persist the caches. + _, err := ctx.ExecutableCache.GetExecutableList() + Expect(err).NotTo(HaveOccurred()) + Expect(ctx.ExecutableCache.Update()).To(Succeed()) + Expect(ctx.WorkspacesCache.Update()).To(Succeed()) + + persisted, err := filesystem.LoadConfig() + Expect(err).NotTo(HaveOccurred()) + Expect(persisted.Workspaces).To(HaveKey("kept")) + Expect(persisted.Workspaces).NotTo(HaveKey("throwaway")) + + raw, err := ctx.DataStore.GetCacheEntry("workspaces") + Expect(err).NotTo(HaveOccurred()) + Expect(string(raw)).NotTo(ContainSubstring("throwaway")) + }) + + Context("overrides", func() { + It("honors FLOW_WORKSPACE pointing at an unregistered path", func() { + other := utils.WriteWorkspace(GinkgoTB(), filepath.Join(root, "other"), "deploy") + here := utils.WriteWorkspace(GinkgoTB(), filepath.Join(root, "here"), "build") + GinkgoTB().Setenv(filesystem.WorkspaceOverrideEnv, other) + + ctx := dynamicCtx(here, nil) + Expect(ctx.CurrentWorkspaceName()).To(Equal("other")) + Expect(ctx.WorkspaceResolution.Source).To(Equal(filesystem.SourceOverride)) + }) + + It("is the only thing that moves the workspace in fixed mode", func() { + pinned := utils.WriteWorkspace(GinkgoTB(), filepath.Join(root, "pinned"), "build") + elsewhere := utils.WriteWorkspace(GinkgoTB(), filepath.Join(root, "elsewhere"), "deploy") + + fixed := utils.NewDiscoveryContext( + stdCtx.Background(), GinkgoTB(), elsewhere, + map[string]string{"pinned": pinned}, config.ConfigWorkspaceModeFixed, + ) + Expect(fixed.CurrentWorkspaceName()).To(Equal("pinned"), + "fixed mode must ignore the working directory") + }) + }) +}) diff --git a/tests/utils/context.go b/tests/utils/context.go index 02a567fa..6225f5b6 100644 --- a/tests/utils/context.go +++ b/tests/utils/context.go @@ -281,6 +281,95 @@ func newTestContext( return ctxx, configDir, cacheDir, wsDir } +// DiscoveryContext is a test context built the way the real CLI builds one — through +// context.NewContext, from a working directory — rather than assembled field by field. That is +// the only way to exercise workspace discovery, which resolves by walking up from the working +// directory and is bypassed entirely by the hand-built contexts above. +// +// Because it changes the process working directory, suites using it must run Serial. +type DiscoveryContext struct { + *context.Context + Dir string +} + +// NewDiscoveryContext points flow's config and cache at fresh temp directories, registers the +// given workspaces (name -> path, may be empty), chdirs to dir, and builds a real context from +// there. +func NewDiscoveryContext( + ctx stdCtx.Context, tb testing.TB, dir string, registered map[string]string, mode config.ConfigWorkspaceMode, +) *DiscoveryContext { + tb.Helper() + root := tb.TempDir() + configDir := filepath.Join(root, userConfigSubdir) + cacheDir := filepath.Join(root, cacheSubdir) + setTestEnv(tb, configDir, cacheDir) + + if err := filesystem.InitConfig(); err != nil { + tb.Fatalf("unable to init config: %v", err) + } + userCfg, err := filesystem.LoadConfig() + if err != nil { + tb.Fatalf("unable to load config: %v", err) + } + userCfg.DefaultLogMode = tuikitIO.Text + userCfg.Interactive = &config.Interactive{Enabled: false} + userCfg.WorkspaceMode = mode + userCfg.Workspaces = map[string]string{} + userCfg.CurrentWorkspace = "" + for name, path := range registered { + userCfg.Workspaces[name] = path + if userCfg.CurrentWorkspace == "" { + userCfg.CurrentWorkspace = name + } + } + if err := filesystem.WriteConfig(userCfg); err != nil { + tb.Fatalf("unable to write config: %v", err) + } + + stdOut, stdIn := createTempIOFiles(tb) + logger.Init(logger.InitOptions{ + Logger: tuikitIO.NewLogger( + tuikitIO.WithOutput(stdOut), + tuikitIO.WithTheme(logger.Theme("")), + tuikitIO.WithMode(tuikitIO.Text), + ), + TestingTB: tb, + }) + + tb.Chdir(dir) + cancel := func() { <-ctx.Done() } + ctxx := context.NewContext(ctx, cancel, context.WithStdIn(stdIn), context.WithStdOut(stdOut)) + tb.Cleanup(func() { + if ctxx.DataStore != nil { + _ = ctxx.DataStore.Close() + } + }) + return &DiscoveryContext{Context: ctxx, Dir: dir} +} + +// WriteWorkspace marks dir as a workspace root and gives it one `run`-verb executable that +// echoes its own name, so a test can prove which workspace an executable resolved from. +func WriteWorkspace(tb testing.TB, dir, execName string) string { + tb.Helper() + if err := os.MkdirAll(dir, 0750); err != nil { + tb.Fatalf("unable to create workspace dir: %v", err) + } + wsFile := filepath.Join(dir, filesystem.WorkspaceConfigFileName) + if err := os.WriteFile(wsFile, []byte("displayName: "+filepath.Base(dir)+"\n"), 0600); err != nil { + tb.Fatalf("unable to write workspace config: %v", err) + } + if execName != "" { + flowFile := fmt.Sprintf( + "namespace: ns\nexecutables:\n - verb: run\n name: %s\n exec:\n cmd: echo %s\n", + execName, execName, + ) + if err := os.WriteFile(filepath.Join(dir, "test.flow"), []byte(flowFile), 0600); err != nil { + tb.Fatalf("unable to write flow file: %v", err) + } + } + return filesystem.NormalizePath(dir) +} + func initTestDirectories(tb testing.TB) (string, string, string) { replacer := strings.NewReplacer("-", "", "'", "-", "/", "-", " ", "_") suiteName := getSuiteName() diff --git a/types/config/config.go b/types/config/config.go index 20e19e7c..f49e58cf 100644 --- a/types/config/config.go +++ b/types/config/config.go @@ -3,6 +3,7 @@ package config import ( "encoding/json" "fmt" + "maps" "os" "path/filepath" "runtime" @@ -10,7 +11,6 @@ import ( "strings" tuikitIO "github.com/flowexec/tuikit/io" - "golang.org/x/exp/maps" "gopkg.in/yaml.v3" ) @@ -39,7 +39,9 @@ func (c *Config) SetDefaults() { c.Workspaces = make(map[string]string) } if c.CurrentWorkspace == "" && len(c.Workspaces) > 0 { - c.CurrentWorkspace = maps.Keys(c.Workspaces)[0] + // Sorted, not arbitrary map order: an unset current workspace would otherwise land on a + // different workspace each run. + c.CurrentWorkspace = slices.Sorted(maps.Keys(c.Workspaces))[0] } if c.WorkspaceMode == "" { c.WorkspaceMode = ConfigWorkspaceModeDynamic @@ -60,40 +62,79 @@ func (c *Config) CurrentVaultName() string { return *c.CurrentVault } +// normalizePath cleans a path and, on macOS, strips the "/private" prefix. Paths under /tmp and +// friends are symlinks into /private there and the OS hands back either form, so both sides of +// a comparison have to be normalized or the same directory compares unequal to itself. +// +// pkg/filesystem.NormalizePath is the same function; it cannot be reused here because +// pkg/filesystem imports this package. +func normalizePath(p string) string { + if p == "" { + return "" + } + p = filepath.Clean(p) + if runtime.GOOS == "darwin" { + if p == "/private" { + return "/" + } + p = strings.TrimPrefix(p, "/private/") + if !strings.HasPrefix(p, "/") { + p = "/" + p + } + } + return p +} + +// NameForWorkspacePath returns the registered workspace whose path is exactly path. +func (c *Config) NameForWorkspacePath(path string) (string, bool) { + target := normalizePath(path) + if target == "" { + return "", false + } + for _, name := range slices.Sorted(maps.Keys(c.Workspaces)) { + if normalizePath(c.Workspaces[name]) == target { + return name, true + } + } + return "", false +} + +// WorkspaceForPath returns the registered workspace containing dir, preferring the longest +// matching path so a workspace nested inside another wins. Iteration is over sorted names +// because Go map order would otherwise make ties nondeterministic between runs. +func (c *Config) WorkspaceForPath(dir string) (string, bool) { + target := normalizePath(dir) + if target == "" { + return "", false + } + + var bestName, bestPath string + for _, name := range slices.Sorted(maps.Keys(c.Workspaces)) { + path := normalizePath(c.Workspaces[name]) + if path == "" { + continue + } + rel, err := filepath.Rel(path, target) + if err != nil || rel == ".." || strings.HasPrefix(rel, ".."+string(filepath.Separator)) { + continue + } + if bestName == "" || len(path) > len(bestPath) { + bestName, bestPath = name, path + } + } + return bestName, bestName != "" +} + func (c *Config) CurrentWorkspaceName() (string, error) { var ws string - mode := c.WorkspaceMode - - switch mode { - case ConfigWorkspaceModeDynamic: + if c.WorkspaceMode == ConfigWorkspaceModeDynamic { wd, err := os.Getwd() if err != nil { return "", err } - if runtime.GOOS == "darwin" { - // On macOS, paths that start with /tmp (and some other system directories) - // are actually symbolic links to paths under /private. The OS may return - // either form of the path - e.g., both "/tmp/file" and "/private/tmp/file" - // refer to the same location. We strip the "/private" prefix for consistent - // path comparison, while preserving the original paths for filesystem operations. - wd = strings.TrimPrefix(wd, "/private") - } - - for wsName, path := range c.Workspaces { - rel, err := filepath.Rel(filepath.Clean(path), filepath.Clean(wd)) - if err != nil { - return "", err - } - if !strings.HasPrefix(rel, "..") { - ws = wsName - break - } - } - fallthrough - case ConfigWorkspaceModeFixed: - if ws != "" { - break - } + ws, _ = c.WorkspaceForPath(wd) + } + if ws == "" { ws = c.CurrentWorkspace } if ws == "" { diff --git a/types/config/config_test.go b/types/config/config_test.go new file mode 100644 index 00000000..125f9f5f --- /dev/null +++ b/types/config/config_test.go @@ -0,0 +1,109 @@ +package config_test + +import ( + "testing" + + . "github.com/onsi/ginkgo/v2" + . "github.com/onsi/gomega" + + "github.com/flowexec/flow/v2/types/config" +) + +func TestConfig(t *testing.T) { + RegisterFailHandler(Fail) + RunSpecs(t, "Config Suite") +} + +var _ = Describe("Workspace path lookups", func() { + Describe("WorkspaceForPath", func() { + It("returns the workspace containing the path", func() { + cfg := &config.Config{Workspaces: map[string]string{"a": "/src/a", "b": "/src/b"}} + name, found := cfg.WorkspaceForPath("/src/a/pkg/deep") + Expect(found).To(BeTrue()) + Expect(name).To(Equal("a")) + }) + + It("prefers the longest match when workspaces nest", func() { + // Map iteration order used to decide this, so the nested workspace won only by luck. + cfg := &config.Config{Workspaces: map[string]string{ + "outer": "/src/repo", + "inner": "/src/repo/sub/worktree", + }} + for range 20 { + name, found := cfg.WorkspaceForPath("/src/repo/sub/worktree/cmd") + Expect(found).To(BeTrue()) + Expect(name).To(Equal("inner")) + } + }) + + It("does not match a sibling sharing a name prefix", func() { + cfg := &config.Config{Workspaces: map[string]string{"ws": "/src/ws"}} + _, found := cfg.WorkspaceForPath("/src/wsX") + Expect(found).To(BeFalse()) + }) + + It("matches the workspace root itself", func() { + cfg := &config.Config{Workspaces: map[string]string{"ws": "/src/ws"}} + name, found := cfg.WorkspaceForPath("/src/ws") + Expect(found).To(BeTrue()) + Expect(name).To(Equal("ws")) + }) + + It("returns false for an empty path or an empty config", func() { + cfg := &config.Config{Workspaces: map[string]string{"ws": "/src/ws"}} + _, found := cfg.WorkspaceForPath("") + Expect(found).To(BeFalse()) + _, found = (&config.Config{}).WorkspaceForPath("/src/ws") + Expect(found).To(BeFalse()) + }) + }) + + Describe("NameForWorkspacePath", func() { + It("matches an exact root only", func() { + cfg := &config.Config{Workspaces: map[string]string{"ws": "/src/ws"}} + name, found := cfg.NameForWorkspacePath("/src/ws") + Expect(found).To(BeTrue()) + Expect(name).To(Equal("ws")) + + _, found = cfg.NameForWorkspacePath("/src/ws/sub") + Expect(found).To(BeFalse()) + }) + + It("tolerates unclean paths", func() { + cfg := &config.Config{Workspaces: map[string]string{"ws": "/src/ws/"}} + _, found := cfg.NameForWorkspacePath("/src/ws/./") + Expect(found).To(BeTrue()) + }) + }) + + Describe("CurrentWorkspaceName", func() { + It("falls back to the configured workspace in fixed mode", func() { + cfg := &config.Config{ + Workspaces: map[string]string{"pinned": "/src/pinned"}, + CurrentWorkspace: "pinned", + WorkspaceMode: config.ConfigWorkspaceModeFixed, + } + name, err := cfg.CurrentWorkspaceName() + Expect(err).NotTo(HaveOccurred()) + Expect(name).To(Equal("pinned")) + }) + + It("errors when nothing is configured", func() { + cfg := &config.Config{WorkspaceMode: config.ConfigWorkspaceModeFixed} + _, err := cfg.CurrentWorkspaceName() + Expect(err).To(MatchError(ContainSubstring("current workspace not found"))) + }) + }) + + Describe("SetDefaults", func() { + It("picks the first workspace by sort order, not map order", func() { + for range 20 { + cfg := &config.Config{Workspaces: map[string]string{ + "zulu": "/z", "alpha": "/a", "mike": "/m", + }} + cfg.SetDefaults() + Expect(cfg.CurrentWorkspace).To(Equal("alpha")) + } + }) + }) +}) From 7565cc265c48a97f456c084c428ebc00bd555e26 Mon Sep 17 00:00:00 2001 From: Jahvon Dockery Date: Tue, 28 Jul 2026 20:27:49 -0400 Subject: [PATCH 4/6] test(mcp): make provenance tests independent of the ambient environment MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit mcpProvenance prefers an inherited FLOW_RUN_SESSION over one it mints, but the subtests covering the mint-our-own fallback never cleared that variable. Running them from inside a harness that exports it — an agent session, or CI — failed on that machine and passed on every other one. Clear both provenance variables for the test; the subtests that want an inherited value already set their own. Co-Authored-By: Claude Opus 5 (1M context) --- internal/mcp/provenance_internal_test.go | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/internal/mcp/provenance_internal_test.go b/internal/mcp/provenance_internal_test.go index 7f8c693e..c16646de 100644 --- a/internal/mcp/provenance_internal_test.go +++ b/internal/mcp/provenance_internal_test.go @@ -39,6 +39,14 @@ func (f fakeSessionWithInfo) SetClientCapabilities(mcp.ClientCapabilities) {} func TestMCPProvenance(t *testing.T) { srv := server.NewMCPServer("test", "1.0.0") + // Start from an environment that inherits nothing. mcpProvenance deliberately prefers an + // inherited session over one it mints, so a developer running these tests from inside a + // harness that exports FLOW_RUN_SESSION — an agent session, or CI — would otherwise see the + // fallback cases fail on their machine and pass on everyone else's. Subtests that want an + // inherited value set it themselves. + t.Setenv(store.RunSessionEnv, "") + t.Setenv(store.RunClientEnv, "") + t.Run("replaces mcp-go's stdio constant with the process session ID", func(t *testing.T) { ctx := srv.WithContext(context.Background(), fakeSession{id: stdioSessionID}) prov := mcpProvenance(ctx) From 0d22d8c428bcc80a94aeb697d36584e8b4d4a7ff Mon Sep 17 00:00:00 2001 From: Jahvon Dockery Date: Sun, 2 Aug 2026 19:10:12 -0400 Subject: [PATCH 5/6] logger fix --- pkg/logger/discard_internal_test.go | 47 +++++++++++++++++++++++++++++ pkg/logger/logger.go | 22 +++++++++++++- pkg/logger/logger_test.go | 36 ++++++++++++++++++++++ 3 files changed, 104 insertions(+), 1 deletion(-) create mode 100644 pkg/logger/discard_internal_test.go diff --git a/pkg/logger/discard_internal_test.go b/pkg/logger/discard_internal_test.go new file mode 100644 index 00000000..d4854b0f --- /dev/null +++ b/pkg/logger/discard_internal_test.go @@ -0,0 +1,47 @@ +package logger + +import ( + "os" + "testing" +) + +// TestDiscardOutputIsNotStandardInput pins the exact defect: the fallback sink used to be +// os.NewFile(0, os.DevNull), which adopts descriptor 0 rather than opening anything. +// +// In-package because the descriptor is the whole assertion, and it is not reachable from +// outside. An external test can only observe the consequence — a descriptor closed by a +// finalizer at an arbitrary later moment — which is exactly what made this so hard to +// trace the first time. +func TestDiscardOutputIsNotStandardInput(t *testing.T) { + sink := discardOutput() + + if sink == nil { + t.Fatal("discardOutput returned nil") + } + if sink.Fd() == 0 { + t.Error("the fallback log sink is standard input; it must open the null device instead") + } +} + +// TestDiscardOutputIsReused guards the second half of the fix. One file per process means +// one finalizer; a file per call leaves a finalizer for every call, each waiting to close +// whichever descriptor number it ends up holding. +func TestDiscardOutputIsReused(t *testing.T) { + first := discardOutput() + for range 50 { + if discardOutput() != first { + t.Fatal("discardOutput allocated a new file; every call must share one") + } + } +} + +// TestDiscardOutputIsWritable confirms the sink is usable, so switching away from +// standard input did not simply move the silence somewhere else. +func TestDiscardOutputIsWritable(t *testing.T) { + if discardOutput() == os.Stdout { + t.Skip("null device unavailable; the fallback is stdout by design") + } + if _, err := discardOutput().WriteString("discarded\n"); err != nil { + t.Errorf("the fallback sink is not writable: %v", err) + } +} diff --git a/pkg/logger/logger.go b/pkg/logger/logger.go index 37cb178a..3d1850cf 100644 --- a/pkg/logger/logger.go +++ b/pkg/logger/logger.go @@ -17,8 +17,28 @@ var ( once sync.Once loggerMutex sync.RWMutex testLoggerEnvKey = "FLOW_TEST_LOGGER" + + discardOnce sync.Once + discardFile *os.File ) +// discardOutput returns a writer that throws log output away, for tests that have not +// registered a logger of their own. +// +// Falls back to stdout if the device cannot be opened: a noisy test logger is a smaller +// problem than one writing to an arbitrary descriptor. +func discardOutput() *os.File { + discardOnce.Do(func() { + f, err := os.OpenFile(os.DevNull, os.O_WRONLY, 0) + if err != nil { + discardFile = os.Stdout + return + } + discardFile = f + }) + return discardFile +} + type InitOptions struct { StdOut *os.File ArchiveDirectory string @@ -79,7 +99,7 @@ func Log() io.Logger { return logger.(io.Logger) //nolint:errcheck } } - return io.NewLogger(io.WithOutput(os.NewFile(0, os.DevNull))) + return io.NewLogger(io.WithOutput(discardOutput())) } if globalLogger == nil { diff --git a/pkg/logger/logger_test.go b/pkg/logger/logger_test.go index a2f1a3ed..b901ba48 100644 --- a/pkg/logger/logger_test.go +++ b/pkg/logger/logger_test.go @@ -2,6 +2,7 @@ package logger_test import ( "os" + "runtime" "testing" "github.com/flowexec/tuikit/io" @@ -34,3 +35,38 @@ var _ = Describe("Global Logger", func() { Expect(logger1).ToNot(Equal(logger2)) }) }) + +// The test-mode fallback logger once wrapped descriptor 0 with os.NewFile, which adopts +// standard input rather than opening the null device, and attaches a finalizer that closes +// whatever descriptor it holds. Every Log() call left another finalizer waiting to close +// fd 0, so the runtime would close standard input part-way through a test run and the next +// file the process opened inherited the freed number — under -coverprofile, that was the +// coverage profile, and the write failed with EBADF while every test passed. +// +// The descriptor the sink actually holds is asserted in discard_internal_test.go, which +// can reach it; this covers the consequence, which is what anyone hitting the bug sees. +var _ = Describe("Test-mode fallback logger", func() { + + // A fresh file per call is what accumulates finalizers. Reusing one keeps the count at + // one for the life of the process, however many times Log() is called. + It("survives repeated use with the garbage collector running", func() { + for range 200 { + logger.Log().Debugf("keeping the fallback busy") + } + runtime.GC() + runtime.GC() + + // Standard input must still be open. If a finalizer had closed it, this fails — + // and so would anything the process opened afterwards. + _, err := os.Stdin.Stat() + Expect(err).ToNot(HaveOccurred(), "standard input was closed by a finalizer") + + // A file opened now must be usable, which is precisely what the coverage writer + // needs at process exit. + f, err := os.CreateTemp(GinkgoT().TempDir(), "after-gc-*") + Expect(err).ToNot(HaveOccurred()) + defer f.Close() + _, err = f.WriteString("still writable") + Expect(err).ToNot(HaveOccurred(), "a file opened after GC was closed underneath us") + }) +}) From cc9afd6ccba06e07a5307b44376f9b038acb018c Mon Sep 17 00:00:00 2001 From: Jahvon Dockery Date: Sun, 2 Aug 2026 19:34:32 -0400 Subject: [PATCH 6/6] fixes --- .claude/settings.json | 4 ---- .claude/skills/setup/SKILL.md | 9 +++++++++ CLAUDE.md | 10 +--------- tests/container_exec_e2e_test.go | 24 +++++++++++++++++++----- 4 files changed, 29 insertions(+), 18 deletions(-) create mode 100644 .claude/skills/setup/SKILL.md diff --git a/.claude/settings.json b/.claude/settings.json index 719d593c..5c631979 100644 --- a/.claude/settings.json +++ b/.claude/settings.json @@ -40,13 +40,9 @@ ], "deny": [ "Edit(types/**/*.gen.go)", - "Write(types/**/*.gen.go)", "Edit(docs/cli/**)", - "Write(docs/cli/**)", "Edit(docs/types/**)", - "Write(docs/types/**)", "Edit(docs/public/schemas/**)", - "Write(docs/public/schemas/**)", "Bash(flow secret get:*)", "Bash(flow secret list:*)", "Bash(env)", diff --git a/.claude/skills/setup/SKILL.md b/.claude/skills/setup/SKILL.md new file mode 100644 index 00000000..61f66c99 --- /dev/null +++ b/.claude/skills/setup/SKILL.md @@ -0,0 +1,9 @@ +--- +name: setup +description: Set up a fresh clone of the flow repo for development (Go toolchain, workspace registration, dev tools, validation). +--- + +1. Go 1.25+, flow CLI installed +2. `flow workspace add flow . --set` +3. `flow install tools` +4. `flow validate` diff --git a/CLAUDE.md b/CLAUDE.md index 5783599b..50f47fd9 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -121,9 +121,7 @@ The CLI emits a structured error envelope (`{"error":{"code","message","details" `--output json|yaml`, plain text otherwise. Both paths go through `cmd/internal/errors.HandleFatal`. Typed errors in `pkg/errors/errors.go` implement `Code() string`. Extend that set rather than -returning bare `fmt.Errorf` when a stable machine-readable code matters. Codes: `INVALID_INPUT`, -`NOT_FOUND`, `EXECUTION_FAILED`, `TIMEOUT`, `CANCELLED`, `VALIDATION_FAILED`, `INTERNAL_ERROR`, -`PERMISSION_DENIED`. +returning bare `fmt.Errorf` when a stable machine-readable code matters. --- @@ -144,9 +142,3 @@ Run the `validate` executable before marking a PR ready. Commit messages: impera - **`.claude/settings.local.json`** — gitignored, per-user. Where absolute paths belong, e.g. `permissions.additionalDirectories` for the module cache and sibling checkouts. -## Development Setup - -1. Go 1.25+, flow CLI installed -2. `flow workspace add flow . --set` -3. `flow install tools` -4. `flow validate` diff --git a/tests/container_exec_e2e_test.go b/tests/container_exec_e2e_test.go index b7b5d775..da5cce87 100644 --- a/tests/container_exec_e2e_test.go +++ b/tests/container_exec_e2e_test.go @@ -6,6 +6,7 @@ import ( stdCtx "context" "os/exec" "runtime" + "time" . "github.com/onsi/ginkgo/v2" . "github.com/onsi/gomega" @@ -14,21 +15,34 @@ import ( ) // canRunLinuxContainers reports whether a runtime capable of running the Linux -// test image is available. Windows hosts are excluded: docker.exe may be on PATH -// there (e.g. Windows CI), but it cannot run a Linux image, so the test would -// fail rather than exercise the feature. +// test image is available and actually reachable. Windows hosts are excluded: +// docker.exe may be on PATH there (e.g. Windows CI), but it cannot run a Linux +// image, so the test would fail rather than exercise the feature. +// +// Mirrors the runner's own "auto" precedence (docker before podman, see +// internal/services/run/container.go ResolveRuntime): a binary on PATH isn't +// enough (Docker Desktop can be installed but not running), so the runtime +// that "auto" would actually pick also has to respond before we call it +// available — otherwise the test fails instead of skipping. func canRunLinuxContainers() bool { if runtime.GOOS == "windows" { return false } for _, rt := range []string{"docker", "podman"} { - if _, err := exec.LookPath(rt); err == nil { - return true + if _, err := exec.LookPath(rt); err != nil { + continue } + return runtimeIsLive(rt) } return false } +func runtimeIsLive(rt string) bool { + ctx, cancel := stdCtx.WithTimeout(stdCtx.Background(), 3*time.Second) + defer cancel() + return exec.CommandContext(ctx, rt, "info").Run() == nil +} + var _ = Describe("container exec e2e", func() { var ctx *utils.Context