Merge pull request #20 from bnema/fix/subagent-binary-resolution
fix(subagents): resolve child zot executable through PATH
2 files · +149 -2
M packages/agent/subagents/runner.go +89 -2
| @@ -164,6 +164,85 @@ func subagentWorkerArgs(opts subagentWorkerArgsOpts) []string { | |||
| 164 | 164 | return args | |
| 165 | 165 | } | |
| 166 | 166 | ||
| 167 | + | // resolveSubagentExecutable finds a runnable zot binary without assuming a | |
| 168 | + | // particular installation directory. A parent process can outlive the file | |
| 169 | + | // it was started from (for example after a reinstall), so all known | |
| 170 | + | // candidates are checked concurrently and the first successful lookup wins. | |
| 171 | + | // The cancellation signal stops candidates that have not finished yet. PATH | |
| 172 | + | // is one of the candidates because it covers GOBIN, user-local bin | |
| 173 | + | // directories, package-manager locations, and system installs without | |
| 174 | + | // hard-coding any of them. | |
| 175 | + | func resolveSubagentExecutable(ctx context.Context, self, argv0 string, lookPath func(context.Context, string) (string, error)) (string, error) { | |
| 176 | + | if ctx == nil { | |
| 177 | + | ctx = context.Background() | |
| 178 | + | } | |
| 179 | + | if lookPath == nil { | |
| 180 | + | lookPath = func(ctx context.Context, candidate string) (string, error) { | |
| 181 | + | select { | |
| 182 | + | case <-ctx.Done(): | |
| 183 | + | return "", ctx.Err() | |
| 184 | + | default: | |
| 185 | + | } | |
| 186 | + | return exec.LookPath(candidate) | |
| 187 | + | } | |
| 188 | + | } | |
| 189 | + | ||
| 190 | + | candidates := make([]string, 0, 5) | |
| 191 | + | addCandidate := func(candidate string) { | |
| 192 | + | if candidate == "" || candidate == "." { | |
| 193 | + | return | |
| 194 | + | } | |
| 195 | + | for _, existing := range candidates { | |
| 196 | + | if existing == candidate { | |
| 197 | + | return | |
| 198 | + | } | |
| 199 | + | } | |
| 200 | + | candidates = append(candidates, candidate) | |
| 201 | + | } | |
| 202 | + | addCandidate(self) | |
| 203 | + | addCandidate(argv0) | |
| 204 | + | if self != "" { | |
| 205 | + | addCandidate(filepath.Base(self)) | |
| 206 | + | } | |
| 207 | + | if argv0 != "" { | |
| 208 | + | addCandidate(filepath.Base(argv0)) | |
| 209 | + | } | |
| 210 | + | addCandidate("zot") | |
| 211 | + | ||
| 212 | + | if len(candidates) == 0 { | |
| 213 | + | return "", fmt.Errorf("locate zot executable: no executable candidates") | |
| 214 | + | } | |
| 215 | + | ||
| 216 | + | type lookupResult struct { | |
| 217 | + | candidate string | |
| 218 | + | path string | |
| 219 | + | err error | |
| 220 | + | } | |
| 221 | + | childCtx, cancel := context.WithCancel(ctx) | |
| 222 | + | defer cancel() | |
| 223 | + | results := make(chan lookupResult, len(candidates)) | |
| 224 | + | for _, candidate := range candidates { | |
| 225 | + | go func(candidate string) { | |
| 226 | + | path, err := lookPath(childCtx, candidate) | |
| 227 | + | if err == nil && path == "" { | |
| 228 | + | err = fmt.Errorf("empty executable path") | |
| 229 | + | } | |
| 230 | + | results <- lookupResult{candidate: candidate, path: path, err: err} | |
| 231 | + | }(candidate) | |
| 232 | + | } | |
| 233 | + | ||
| 234 | + | lookupErrors := make([]string, 0, len(candidates)) | |
| 235 | + | for range candidates { | |
| 236 | + | result := <-results | |
| 237 | + | if result.err == nil { | |
| 238 | + | cancel() | |
| 239 | + | return result.path, nil | |
| 240 | + | } | |
| 241 | + | lookupErrors = append(lookupErrors, fmt.Sprintf("%q: %v", result.candidate, result.err)) | |
| 242 | + | } | |
| 243 | + | return "", fmt.Errorf("locate zot executable: %s", strings.Join(lookupErrors, "; ")) | |
| 244 | + | } | |
| 245 | + | ||
| 167 | 246 | func (r *execRunner) Run(ctx context.Context, sink Sink) error { | |
| 168 | 247 | // SessionPath resolution order: | |
| 169 | 248 | // 1. explicit r.SessionPath set by the test / caller | |
| @@ -204,9 +283,17 @@ func (r *execRunner) Run(ctx context.Context, sink Sink) error { | |||
| 204 | 283 | ||
| 205 | 284 | args := r.Command | |
| 206 | 285 | if len(args) == 0 { | |
| 207 | - | exe, err := os.Executable() | |
| 286 | + | self, selfErr := os.Executable() | |
| 287 | + | argv0 := "" | |
| 288 | + | if len(os.Args) > 0 { | |
| 289 | + | argv0 = os.Args[0] | |
| 290 | + | } | |
| 291 | + | exe, err := resolveSubagentExecutable(ctx, self, argv0, nil) | |
| 208 | 292 | if err != nil { | |
| 209 | - | return fmt.Errorf("locate self: %w", err) | |
| 293 | + | if selfErr != nil { | |
| 294 | + | return fmt.Errorf("locate zot executable (os.Executable: %v): %w", selfErr, err) | |
| 295 | + | } | |
| 296 | + | return err | |
| 210 | 297 | } | |
| 211 | 298 | args = defaultChildArgs(exe, r.agent, sessionPath, inboxPath) | |
| 212 | 299 | } | |
| @@ -164,6 +164,85 @@ func subagentWorkerArgs(opts subagentWorkerArgsOpts) []string { | |||||
| 164 | return args | 164 | return args | ||
| 165 | } | 165 | } | ||
| 166 | 166 | ||||
| 167 | + | // resolveSubagentExecutable finds a runnable zot binary without assuming a | |||
| 168 | + | // particular installation directory. A parent process can outlive the file | |||
| 169 | + | // it was started from (for example after a reinstall), so all known | |||
| 170 | + | // candidates are checked concurrently and the first successful lookup wins. | |||
| 171 | + | // The cancellation signal stops candidates that have not finished yet. PATH | |||
| 172 | + | // is one of the candidates because it covers GOBIN, user-local bin | |||
| 173 | + | // directories, package-manager locations, and system installs without | |||
| 174 | + | // hard-coding any of them. | |||
| 175 | + | func resolveSubagentExecutable(ctx context.Context, self, argv0 string, lookPath func(context.Context, string) (string, error)) (string, error) { | |||
| 176 | + | if ctx == nil { | |||
| 177 | + | ctx = context.Background() | |||
| 178 | + | } | |||
| 179 | + | if lookPath == nil { | |||
| 180 | + | lookPath = func(ctx context.Context, candidate string) (string, error) { | |||
| 181 | + | select { | |||
| 182 | + | case <-ctx.Done(): | |||
| 183 | + | return "", ctx.Err() | |||
| 184 | + | default: | |||
| 185 | + | } | |||
| 186 | + | return exec.LookPath(candidate) | |||
| 187 | + | } | |||
| 188 | + | } | |||
| 189 | + | ||||
| 190 | + | candidates := make([]string, 0, 5) | |||
| 191 | + | addCandidate := func(candidate string) { | |||
| 192 | + | if candidate == "" || candidate == "." { | |||
| 193 | + | return | |||
| 194 | + | } | |||
| 195 | + | for _, existing := range candidates { | |||
| 196 | + | if existing == candidate { | |||
| 197 | + | return | |||
| 198 | + | } | |||
| 199 | + | } | |||
| 200 | + | candidates = append(candidates, candidate) | |||
| 201 | + | } | |||
| 202 | + | addCandidate(self) | |||
| 203 | + | addCandidate(argv0) | |||
| 204 | + | if self != "" { | |||
| 205 | + | addCandidate(filepath.Base(self)) | |||
| 206 | + | } | |||
| 207 | + | if argv0 != "" { | |||
| 208 | + | addCandidate(filepath.Base(argv0)) | |||
| 209 | + | } | |||
| 210 | + | addCandidate("zot") | |||
| 211 | + | ||||
| 212 | + | if len(candidates) == 0 { | |||
| 213 | + | return "", fmt.Errorf("locate zot executable: no executable candidates") | |||
| 214 | + | } | |||
| 215 | + | ||||
| 216 | + | type lookupResult struct { | |||
| 217 | + | candidate string | |||
| 218 | + | path string | |||
| 219 | + | err error | |||
| 220 | + | } | |||
| 221 | + | childCtx, cancel := context.WithCancel(ctx) | |||
| 222 | + | defer cancel() | |||
| 223 | + | results := make(chan lookupResult, len(candidates)) | |||
| 224 | + | for _, candidate := range candidates { | |||
| 225 | + | go func(candidate string) { | |||
| 226 | + | path, err := lookPath(childCtx, candidate) | |||
| 227 | + | if err == nil && path == "" { | |||
| 228 | + | err = fmt.Errorf("empty executable path") | |||
| 229 | + | } | |||
| 230 | + | results <- lookupResult{candidate: candidate, path: path, err: err} | |||
| 231 | + | }(candidate) | |||
| 232 | + | } | |||
| 233 | + | ||||
| 234 | + | lookupErrors := make([]string, 0, len(candidates)) | |||
| 235 | + | for range candidates { | |||
| 236 | + | result := <-results | |||
| 237 | + | if result.err == nil { | |||
| 238 | + | cancel() | |||
| 239 | + | return result.path, nil | |||
| 240 | + | } | |||
| 241 | + | lookupErrors = append(lookupErrors, fmt.Sprintf("%q: %v", result.candidate, result.err)) | |||
| 242 | + | } | |||
| 243 | + | return "", fmt.Errorf("locate zot executable: %s", strings.Join(lookupErrors, "; ")) | |||
| 244 | + | } | |||
| 245 | + | ||||
| 167 | func (r *execRunner) Run(ctx context.Context, sink Sink) error { | 246 | func (r *execRunner) Run(ctx context.Context, sink Sink) error { | ||
| 168 | // SessionPath resolution order: | 247 | // SessionPath resolution order: | ||
| 169 | // 1. explicit r.SessionPath set by the test / caller | 248 | // 1. explicit r.SessionPath set by the test / caller | ||
| @@ -204,9 +283,17 @@ func (r *execRunner) Run(ctx context.Context, sink Sink) error { | |||||
| 204 | 283 | ||||
| 205 | args := r.Command | 284 | args := r.Command | ||
| 206 | if len(args) == 0 { | 285 | if len(args) == 0 { | ||
| 207 | - | exe, err := os.Executable() | 286 | + | self, selfErr := os.Executable() |
| 287 | + | argv0 := "" | |||
| 288 | + | if len(os.Args) > 0 { | |||
| 289 | + | argv0 = os.Args[0] | |||
| 290 | + | } | |||
| 291 | + | exe, err := resolveSubagentExecutable(ctx, self, argv0, nil) | |||
| 208 | if err != nil { | 292 | if err != nil { | ||
| 209 | - | return fmt.Errorf("locate self: %w", err) | 293 | + | if selfErr != nil { |
| 294 | + | return fmt.Errorf("locate zot executable (os.Executable: %v): %w", selfErr, err) | |||
| 295 | + | } | |||
| 296 | + | return err | |||
| 210 | } | 297 | } | ||
| 211 | args = defaultChildArgs(exe, r.agent, sessionPath, inboxPath) | 298 | args = defaultChildArgs(exe, r.agent, sessionPath, inboxPath) | ||
| 212 | } | 299 | } | ||
M packages/agent/subagents/runner_test.go +60 -0
| @@ -194,6 +194,66 @@ func TestSubagentWorkerArgs(t *testing.T) { | |||
| 194 | 194 | } | |
| 195 | 195 | } | |
| 196 | 196 | ||
| 197 | + | func TestResolveSubagentExecutableUsesCurrentWhenAvailable(t *testing.T) { | |
| 198 | + | got, err := resolveSubagentExecutable(context.Background(), "/current/bin/zot", "/old/bin/zot", func(_ context.Context, candidate string) (string, error) { | |
| 199 | + | if candidate == "/current/bin/zot" { | |
| 200 | + | return candidate, nil | |
| 201 | + | } | |
| 202 | + | return "", os.ErrNotExist | |
| 203 | + | }) | |
| 204 | + | if err != nil { | |
| 205 | + | t.Fatal(err) | |
| 206 | + | } | |
| 207 | + | if got != "/current/bin/zot" { | |
| 208 | + | t.Fatalf("executable = %q, want current executable", got) | |
| 209 | + | } | |
| 210 | + | } | |
| 211 | + | ||
| 212 | + | func TestResolveSubagentExecutableFallsBackToPath(t *testing.T) { | |
| 213 | + | got, err := resolveSubagentExecutable(context.Background(), "/removed/.local/bin/zot", "/removed/.local/bin/zot", func(_ context.Context, candidate string) (string, error) { | |
| 214 | + | if candidate == "zot" { | |
| 215 | + | return "/active/go/bin/zot", nil | |
| 216 | + | } | |
| 217 | + | return "", os.ErrNotExist | |
| 218 | + | }) | |
| 219 | + | if err != nil { | |
| 220 | + | t.Fatal(err) | |
| 221 | + | } | |
| 222 | + | if got != "/active/go/bin/zot" { | |
| 223 | + | t.Fatalf("executable = %q, want PATH fallback", got) | |
| 224 | + | } | |
| 225 | + | } | |
| 226 | + | ||
| 227 | + | func TestResolveSubagentExecutableCancelsOtherLookupsAfterFirstSuccess(t *testing.T) { | |
| 228 | + | slowStarted := make(chan struct{}) | |
| 229 | + | slowCanceled := make(chan struct{}) | |
| 230 | + | got, err := resolveSubagentExecutable(context.Background(), "/current/bin/zot", "/old/bin/zot", func(ctx context.Context, candidate string) (string, error) { | |
| 231 | + | switch candidate { | |
| 232 | + | case "/current/bin/zot": | |
| 233 | + | close(slowStarted) | |
| 234 | + | <-ctx.Done() | |
| 235 | + | close(slowCanceled) | |
| 236 | + | return "", ctx.Err() | |
| 237 | + | case "zot": | |
| 238 | + | <-slowStarted | |
| 239 | + | return "/active/go/bin/zot", nil | |
| 240 | + | default: | |
| 241 | + | return "", os.ErrNotExist | |
| 242 | + | } | |
| 243 | + | }) | |
| 244 | + | if err != nil { | |
| 245 | + | t.Fatal(err) | |
| 246 | + | } | |
| 247 | + | if got != "/active/go/bin/zot" { | |
| 248 | + | t.Fatalf("executable = %q, want first successful lookup", got) | |
| 249 | + | } | |
| 250 | + | select { | |
| 251 | + | case <-slowCanceled: | |
| 252 | + | case <-time.After(time.Second): | |
| 253 | + | t.Fatal("slower lookup was not canceled") | |
| 254 | + | } | |
| 255 | + | } | |
| 256 | + | ||
| 197 | 257 | func TestSubagentWorkerArgsPropagatesProviderConnectionSettings(t *testing.T) { | |
| 198 | 258 | args := subagentWorkerArgs(subagentWorkerArgsOpts{ | |
| 199 | 259 | Exe: "/zot", | |
| @@ -194,6 +194,66 @@ func TestSubagentWorkerArgs(t *testing.T) { | |||||
| 194 | } | 194 | } | ||
| 195 | } | 195 | } | ||
| 196 | 196 | ||||
| 197 | + | func TestResolveSubagentExecutableUsesCurrentWhenAvailable(t *testing.T) { | |||
| 198 | + | got, err := resolveSubagentExecutable(context.Background(), "/current/bin/zot", "/old/bin/zot", func(_ context.Context, candidate string) (string, error) { | |||
| 199 | + | if candidate == "/current/bin/zot" { | |||
| 200 | + | return candidate, nil | |||
| 201 | + | } | |||
| 202 | + | return "", os.ErrNotExist | |||
| 203 | + | }) | |||
| 204 | + | if err != nil { | |||
| 205 | + | t.Fatal(err) | |||
| 206 | + | } | |||
| 207 | + | if got != "/current/bin/zot" { | |||
| 208 | + | t.Fatalf("executable = %q, want current executable", got) | |||
| 209 | + | } | |||
| 210 | + | } | |||
| 211 | + | ||||
| 212 | + | func TestResolveSubagentExecutableFallsBackToPath(t *testing.T) { | |||
| 213 | + | got, err := resolveSubagentExecutable(context.Background(), "/removed/.local/bin/zot", "/removed/.local/bin/zot", func(_ context.Context, candidate string) (string, error) { | |||
| 214 | + | if candidate == "zot" { | |||
| 215 | + | return "/active/go/bin/zot", nil | |||
| 216 | + | } | |||
| 217 | + | return "", os.ErrNotExist | |||
| 218 | + | }) | |||
| 219 | + | if err != nil { | |||
| 220 | + | t.Fatal(err) | |||
| 221 | + | } | |||
| 222 | + | if got != "/active/go/bin/zot" { | |||
| 223 | + | t.Fatalf("executable = %q, want PATH fallback", got) | |||
| 224 | + | } | |||
| 225 | + | } | |||
| 226 | + | ||||
| 227 | + | func TestResolveSubagentExecutableCancelsOtherLookupsAfterFirstSuccess(t *testing.T) { | |||
| 228 | + | slowStarted := make(chan struct{}) | |||
| 229 | + | slowCanceled := make(chan struct{}) | |||
| 230 | + | got, err := resolveSubagentExecutable(context.Background(), "/current/bin/zot", "/old/bin/zot", func(ctx context.Context, candidate string) (string, error) { | |||
| 231 | + | switch candidate { | |||
| 232 | + | case "/current/bin/zot": | |||
| 233 | + | close(slowStarted) | |||
| 234 | + | <-ctx.Done() | |||
| 235 | + | close(slowCanceled) | |||
| 236 | + | return "", ctx.Err() | |||
| 237 | + | case "zot": | |||
| 238 | + | <-slowStarted | |||
| 239 | + | return "/active/go/bin/zot", nil | |||
| 240 | + | default: | |||
| 241 | + | return "", os.ErrNotExist | |||
| 242 | + | } | |||
| 243 | + | }) | |||
| 244 | + | if err != nil { | |||
| 245 | + | t.Fatal(err) | |||
| 246 | + | } | |||
| 247 | + | if got != "/active/go/bin/zot" { | |||
| 248 | + | t.Fatalf("executable = %q, want first successful lookup", got) | |||
| 249 | + | } | |||
| 250 | + | select { | |||
| 251 | + | case <-slowCanceled: | |||
| 252 | + | case <-time.After(time.Second): | |||
| 253 | + | t.Fatal("slower lookup was not canceled") | |||
| 254 | + | } | |||
| 255 | + | } | |||
| 256 | + | ||||
| 197 | func TestSubagentWorkerArgsPropagatesProviderConnectionSettings(t *testing.T) { | 257 | func TestSubagentWorkerArgsPropagatesProviderConnectionSettings(t *testing.T) { | ||
| 198 | args := subagentWorkerArgs(subagentWorkerArgsOpts{ | 258 | args := subagentWorkerArgs(subagentWorkerArgsOpts{ | ||
| 199 | Exe: "/zot", | 259 | Exe: "/zot", | ||