diff --git a/control-plane/internal/cli/commands/install.go b/control-plane/internal/cli/commands/install.go index 50fe2cf47..e06f44e9b 100644 --- a/control-plane/internal/cli/commands/install.go +++ b/control-plane/internal/cli/commands/install.go @@ -36,6 +36,7 @@ func (cmd *InstallCommand) GetDescription() string { func (cmd *InstallCommand) BuildCobraCommand() *cobra.Command { var force bool var verbose bool + var path string cobraCmd := &cobra.Command{ Use: "install ", @@ -47,28 +48,40 @@ The package can be: - A GitHub repository URL - A package name from the AgentField registry +Use --path to install a package that lives in a subdirectory of the source, so a +single repository can ship more than one installable node. The subdirectory must +contain its own agentfield-package.yaml; that subtree becomes the package root. +--path is relative to the source root and may not escape it. It composes with an +@ref pin on a Git URL. + Examples: agentfield install ./my-agent agentfield install https://github.com/user/agent-repo + agentfield install https://github.com/user/agent-repo --path go + agentfield install https://github.com/user/agent-repo@v1.2.3 --path go agentfield install agent-name`, Args: cobra.ExactArgs(1), RunE: func(cobraCmd *cobra.Command, args []string) error { // Update output formatter with verbose setting cmd.output.SetVerbose(verbose) - return cmd.execute(args[0], force, verbose) + return cmd.execute(args[0], force, verbose, path) }, } cobraCmd.Flags().BoolVarP(&force, "force", "f", false, "Force reinstall if package exists") cobraCmd.Flags().BoolVarP(&verbose, "verbose", "v", false, "Verbose output") + cobraCmd.Flags().StringVar(&path, "path", "", "Install the package from this subdirectory of the source (relative to its root)") return cobraCmd } // execute performs the actual installation -func (cmd *InstallCommand) execute(packagePath string, force, verbose bool) error { +func (cmd *InstallCommand) execute(packagePath string, force, verbose bool, path string) error { cmd.output.PrintHeader("Installing AgentField Package") cmd.output.PrintInfo(fmt.Sprintf("Package: %s", packagePath)) + if path != "" { + cmd.output.PrintInfo(fmt.Sprintf("Subdirectory: %s", path)) + } if verbose { cmd.output.PrintVerbose("Using new framework-based install command") @@ -78,6 +91,7 @@ func (cmd *InstallCommand) execute(packagePath string, force, verbose bool) erro options := domain.InstallOptions{ Force: force, Verbose: verbose, + Path: path, } // Show progress diff --git a/control-plane/internal/core/domain/models.go b/control-plane/internal/core/domain/models.go index efd9992b8..3aed03340 100644 --- a/control-plane/internal/core/domain/models.go +++ b/control-plane/internal/core/domain/models.go @@ -66,6 +66,12 @@ type AgentFieldConfig struct { type InstallOptions struct { Force bool `json:"force"` Verbose bool `json:"verbose"` + // Path optionally selects a subdirectory within the source (git repo or local + // directory) whose agentfield-package.yaml should be installed, letting one + // repository ship multiple installable nodes. Empty means the default + // root-first behavior. It applies only to the top-level source, never to + // recursively-installed node dependencies. + Path string `json:"path"` } // RunOptions represents options for running an agent diff --git a/control-plane/internal/core/services/coverage_targeted_test.go b/control-plane/internal/core/services/coverage_targeted_test.go index 684df6d22..ab24a0d2b 100644 --- a/control-plane/internal/core/services/coverage_targeted_test.go +++ b/control-plane/internal/core/services/coverage_targeted_test.go @@ -324,7 +324,7 @@ func TestPackageServiceAdditionalCoverage(t *testing.T) { require.NoError(t, os.WriteFile(filepath.Join(home, "installed.yaml"), data, 0o644)) service := &DefaultPackageService{agentfieldHome: home} - require.NoError(t, service.installLocalPackage(sourcePath, true, false)) + require.NoError(t, service.installLocalPackage(sourcePath, "", true, false)) updatedData, err := os.ReadFile(filepath.Join(home, "installed.yaml")) require.NoError(t, err) @@ -341,7 +341,7 @@ func TestPackageServiceAdditionalCoverage(t *testing.T) { require.NoError(t, os.WriteFile(filepath.Join(home, "packages"), []byte("blocker"), 0o644)) service := &DefaultPackageService{agentfieldHome: home} - err := service.installLocalPackage(sourcePath, false, false) + err := service.installLocalPackage(sourcePath, "", false, false) require.Error(t, err) assert.Contains(t, err.Error(), "failed to copy package") }) diff --git a/control-plane/internal/core/services/install_path_test.go b/control-plane/internal/core/services/install_path_test.go new file mode 100644 index 000000000..0e275628b --- /dev/null +++ b/control-plane/internal/core/services/install_path_test.go @@ -0,0 +1,116 @@ +package services + +import ( + "os" + "path/filepath" + "testing" + + "github.com/Agent-Field/agentfield/control-plane/internal/core/domain" + "github.com/Agent-Field/agentfield/control-plane/internal/packages" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + "gopkg.in/yaml.v3" +) + +// Validation contract for `af install --path `: +// 1. --path installs the node whose manifest is at /. +// 2. No --path installs the root node (unchanged). +// 3. --path with no manifest there errors, naming the expected path; nothing +// is installed. +// 4. Absolute / escaping --path is rejected; nothing is installed. +// These mirror the git-path contract for a local source. + +// writeNode writes a minimal, dependency-free Python node (so install does no +// venv/network work) at dir with the given package name. +func writeNode(t *testing.T, dir, name string) { + t.Helper() + require.NoError(t, os.MkdirAll(dir, 0o755)) + manifest := "name: " + name + "\nversion: 1.0.0\nmain: main.py\n" + require.NoError(t, os.WriteFile(filepath.Join(dir, "agentfield-package.yaml"), []byte(manifest), 0o644)) + require.NoError(t, os.WriteFile(filepath.Join(dir, "main.py"), []byte("print('ok')\n"), 0o644)) +} + +// installedNamesFromRegistry reads ~/.agentfield/installed.yaml and returns the +// set of installed package names (empty when the registry file is absent). +func installedNamesFromRegistry(t *testing.T, home string) map[string]bool { + t.Helper() + names := map[string]bool{} + data, err := os.ReadFile(filepath.Join(home, "installed.yaml")) + if os.IsNotExist(err) { + return names + } + require.NoError(t, err) + var reg packages.InstallationRegistry + require.NoError(t, yaml.Unmarshal(data, ®)) + for name := range reg.Installed { + names[name] = true + } + return names +} + +func newLocalPackageService(t *testing.T, home string) *DefaultPackageService { + t.Helper() + return NewPackageService(newMockPackageRegistryStorage(), newMockFileSystemAdapter(), home).(*DefaultPackageService) +} + +func TestInstallLocalPackage_PathSelectsSubdir(t *testing.T) { + home := t.TempDir() + src := filepath.Join(t.TempDir(), "repo") + writeNode(t, src, "root-node") + writeNode(t, filepath.Join(src, "sub"), "sub-node") + + svc := newLocalPackageService(t, home) + require.NoError(t, svc.InstallPackage(src, domain.InstallOptions{Path: "sub"})) + + names := installedNamesFromRegistry(t, home) + assert.True(t, names["sub-node"], "the --path subdir node should be installed") + assert.False(t, names["root-node"], "the root node must NOT be installed when --path selects a subdir") + + // The installed package directory is keyed by the subdir manifest name and + // contains that subtree at its root. + pkgDir := filepath.Join(home, "packages", "sub-node") + if _, err := os.Stat(filepath.Join(pkgDir, "agentfield-package.yaml")); err != nil { + t.Fatalf("expected sub-node package copied to %s: %v", pkgDir, err) + } +} + +func TestInstallLocalPackage_NoPathInstallsRoot(t *testing.T) { + home := t.TempDir() + src := filepath.Join(t.TempDir(), "repo") + writeNode(t, src, "root-node") + writeNode(t, filepath.Join(src, "sub"), "sub-node") + + svc := newLocalPackageService(t, home) + require.NoError(t, svc.InstallPackage(src, domain.InstallOptions{})) + + names := installedNamesFromRegistry(t, home) + assert.True(t, names["root-node"], "bare install must install the root node") + assert.False(t, names["sub-node"], "bare install must not reach into subdirectories") +} + +func TestInstallLocalPackage_PathMissingManifest(t *testing.T) { + home := t.TempDir() + src := filepath.Join(t.TempDir(), "repo") + writeNode(t, src, "root-node") + + svc := newLocalPackageService(t, home) + err := svc.InstallPackage(src, domain.InstallOptions{Path: "nope"}) + require.Error(t, err) + assert.Contains(t, err.Error(), filepath.Join(src, "nope", "agentfield-package.yaml")) + assert.Empty(t, installedNamesFromRegistry(t, home), "a failed --path install must not mutate the registry") +} + +func TestInstallLocalPackage_PathRejectsAbsoluteAndEscape(t *testing.T) { + for _, bad := range []string{"/etc", "../escape"} { + t.Run(bad, func(t *testing.T) { + home := t.TempDir() + src := filepath.Join(t.TempDir(), "repo") + writeNode(t, src, "root-node") + + svc := newLocalPackageService(t, home) + err := svc.InstallPackage(src, domain.InstallOptions{Path: bad}) + require.Error(t, err) + assert.Empty(t, installedNamesFromRegistry(t, home), "a rejected --path must not install anything") + }) + } +} diff --git a/control-plane/internal/core/services/package_service.go b/control-plane/internal/core/services/package_service.go index 8cff30326..17489d03d 100644 --- a/control-plane/internal/core/services/package_service.go +++ b/control-plane/internal/core/services/package_service.go @@ -47,7 +47,12 @@ func (ps *DefaultPackageService) InstallPackage(source string, options domain.In return err } - return ps.installNodeDependencies(before, options) + // The --path selector targets a subdirectory of THIS source only. Node + // dependencies are their own installable sources, so never carry the selector + // into recursive dependency installs. + depOptions := options + depOptions.Path = "" + return ps.installNodeDependencies(before, depOptions) } // installOne installs a single package from a git URL or local path. @@ -57,12 +62,13 @@ func (ps *DefaultPackageService) installOne(source string, options domain.Instal installer := &packages.GitInstaller{ AgentFieldHome: ps.agentfieldHome, Verbose: options.Verbose, + Subdir: options.Path, } return installer.InstallFromGit(source, options.Force) } // Handle local package installation - return ps.installLocalPackage(source, options.Force, options.Verbose) + return ps.installLocalPackage(source, options.Path, options.Force, options.Verbose) } // installedNames returns the set of currently-installed package names. @@ -134,8 +140,20 @@ func resolveNodeRef(ref string) (source string, name string) { return ref, "" } -// installLocalPackage installs a package from a local source path -func (ps *DefaultPackageService) installLocalPackage(sourcePath string, force bool, verbose bool) error { +// installLocalPackage installs a package from a local source path. When subdir is +// non-empty (the --path selector) the package root is resolved to +// /, which must contain an agentfield-package.yaml; that +// subdirectory is what gets validated, copied, and installed. Resolution happens +// before any copy or registry mutation, so a bad selector fails cleanly. +func (ps *DefaultPackageService) installLocalPackage(sourcePath string, subdir string, force bool, verbose bool) error { + if strings.TrimSpace(subdir) != "" { + resolved, err := packages.ResolvePackageSubdir(sourcePath, subdir) + if err != nil { + return err + } + sourcePath = resolved + } + // Get package name first for better messaging metadata, err := ps.parsePackageMetadata(sourcePath) if err != nil { diff --git a/control-plane/internal/packages/git.go b/control-plane/internal/packages/git.go index 5065d287e..df691f24c 100644 --- a/control-plane/internal/packages/git.go +++ b/control-plane/internal/packages/git.go @@ -26,6 +26,12 @@ type GitPackageInfo struct { type GitInstaller struct { AgentFieldHome string Verbose bool + // Subdir optionally selects a package subdirectory within the cloned repo + // (the `--path` flag). Empty means the historical root-first walk. When set, + // the manifest MUST live at //agentfield-package.yaml and that + // subdirectory becomes the package root that is copied and installed. It + // composes with an @ref pin on the URL, which is parsed independently. + Subdir string } // newSpinner creates a new spinner with the given message @@ -106,6 +112,12 @@ func checkGitAvailable() error { // InstallFromGit installs a package from any Git repository func (gi *GitInstaller) InstallFromGit(gitURL string, force bool) error { + // Reject a malformed --path selector (absolute / escaping) up front, before + // any install work (clone, copy, registry mutation) happens. + if err := validateSubdirSelector(gi.Subdir); err != nil { + return err + } + // Check if Git is available if err := checkGitAvailable(); err != nil { return err @@ -136,7 +148,7 @@ func (gi *GitInstaller) InstallFromGit(gitURL string, force bool) error { spinner = gi.newSpinner("Validating package structure") spinner.Start() - packagePath, err := gi.findPackageRoot(tempDir) + packagePath, err := gi.resolvePackageRoot(tempDir) if err != nil { spinner.Error("Invalid package structure") return fmt.Errorf("invalid package structure: %w", err) @@ -257,6 +269,26 @@ func (gi *GitInstaller) cloneRepository(info *GitPackageInfo) (string, error) { return tempDir, nil } +// resolvePackageRoot determines which directory of the cloned repository is the +// package to install. With no --path selector it defers to findPackageRoot's +// root-first walk (unchanged behavior). With a selector it resolves and validates +// /, requiring the manifest to exist there, so one repo can ship +// multiple installable nodes selected explicitly. +func (gi *GitInstaller) resolvePackageRoot(cloneDir string) (string, error) { + if strings.TrimSpace(gi.Subdir) == "" { + return gi.findPackageRoot(cloneDir) + } + root, err := ResolvePackageSubdir(cloneDir, gi.Subdir) + if err != nil { + return "", err + } + // A selected subdir must still be a valid, startable agent node. + if err := ValidatePackage(root); err != nil { + return "", err + } + return root, nil +} + // findPackageRoot finds the root directory containing agentfield-package.yaml func (gi *GitInstaller) findPackageRoot(cloneDir string) (string, error) { var packageRoot string diff --git a/control-plane/internal/packages/install_path_test.go b/control-plane/internal/packages/install_path_test.go new file mode 100644 index 000000000..77703290c --- /dev/null +++ b/control-plane/internal/packages/install_path_test.go @@ -0,0 +1,233 @@ +package packages + +import ( + "os" + "path/filepath" + "strings" + "testing" +) + +// Validation contract for the `--path` subdirectory selector (one repo, multiple +// installable nodes): +// 1. --path selects the node whose manifest lives at +// //agentfield-package.yaml; that subtree is the package root. +// 2. No selector => historical root-first behavior (unchanged). +// 3. A selector pointing where there is no manifest fails, naming the full +// expected manifest path. +// 4. An absolute or escaping (`..`) selector is rejected syntactically, before +// any filesystem/clone work. +// 5. The selector is orthogonal to the @ref URL pin — both are honored. +// 6. A Go-language subdir package builds relative to the subdir-as-root. + +// writeRootPythonPackage lays down a root Python node (manifest + main.py) and a +// Go node under go/ so one clone directory carries two installable packages. +func writeMultiNodeClone(t *testing.T, cloneDir string) (goSub string) { + t.Helper() + writeTestPackage(t, cloneDir, "name: root-node\nversion: 1.0.0\nentrypoint:\n start: python -m root.app\n") + goSub = filepath.Join(cloneDir, "go") + writeGoManifest(t, goSub, + "name: go-node\nversion: 2.0.0\nlanguage: go\nentrypoint:\n build: ./cmd/node\n start: bin/node\n", + "1.21", "") + return goSub +} + +// Contract 4 (syntax): absolute and escaping selectors are rejected without any +// filesystem access; empty and in-tree selectors pass the syntax gate. +func TestValidateSubdirSelector(t *testing.T) { + t.Parallel() + valid := []string{"", "go", "go/nested", "./go", "a/../b"} + for _, s := range valid { + if err := ValidateSubdirSelector(s); err != nil { + t.Errorf("ValidateSubdirSelector(%q) unexpected error: %v", s, err) + } + } + invalid := []string{"/abs", "/etc/passwd", "..", "../escape", "../../x", "a/../../b"} + for _, s := range invalid { + if err := ValidateSubdirSelector(s); err == nil { + t.Errorf("ValidateSubdirSelector(%q) = nil; want rejection", s) + } + } +} + +// Contracts 1, 3, 4 at the resolver level (root selection, manifest existence, +// containment) independent of git. +func TestResolvePackageSubdir(t *testing.T) { + t.Parallel() + root := t.TempDir() + goSub := writeMultiNodeClone(t, root) + + t.Run("empty selector returns root unchanged", func(t *testing.T) { + got, err := ResolvePackageSubdir(root, "") + if err != nil || got != root { + t.Fatalf("ResolvePackageSubdir(root, \"\") = %q,%v; want %q,nil", got, err, root) + } + }) + + t.Run("selects existing subdir", func(t *testing.T) { + got, err := ResolvePackageSubdir(root, "go") + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + if got != goSub { + t.Fatalf("got %q; want %q", got, goSub) + } + }) + + t.Run("in-tree traversal that stays inside is allowed", func(t *testing.T) { + // "go/../go" cleans to "go" and stays within root. + got, err := ResolvePackageSubdir(root, "go/../go") + if err != nil || got != goSub { + t.Fatalf("ResolvePackageSubdir(root, \"go/../go\") = %q,%v; want %q,nil", got, err, goSub) + } + }) + + t.Run("missing manifest names the expected path", func(t *testing.T) { + _, err := ResolvePackageSubdir(root, "nope") + if err == nil { + t.Fatal("expected error for missing manifest") + } + want := filepath.Join(root, "nope", "agentfield-package.yaml") + if !strings.Contains(err.Error(), want) { + t.Fatalf("error %q should name expected manifest path %q", err.Error(), want) + } + }) + + t.Run("absolute path rejected", func(t *testing.T) { + if _, err := ResolvePackageSubdir(root, "/etc"); err == nil { + t.Fatal("expected rejection of absolute path") + } + }) + + t.Run("escaping path rejected", func(t *testing.T) { + if _, err := ResolvePackageSubdir(root, "../escape"); err == nil { + t.Fatal("expected rejection of escaping path") + } + }) +} + +// Contracts 1, 2, 3, 4 via the GitInstaller's post-clone resolution against a +// faked clone directory (no network/git needed). +func TestGitInstallerResolvePackageRoot(t *testing.T) { + t.Parallel() + clone := t.TempDir() + goSub := writeMultiNodeClone(t, clone) + + t.Run("no selector installs the root package (unchanged)", func(t *testing.T) { + gi := &GitInstaller{AgentFieldHome: t.TempDir()} + root, err := gi.resolvePackageRoot(clone) + if err != nil { + t.Fatalf("resolvePackageRoot: %v", err) + } + md, err := ParsePackageMetadata(root) + if err != nil { + t.Fatalf("parse: %v", err) + } + if md.Name != "root-node" { + t.Fatalf("root install picked %q; want root-node", md.Name) + } + }) + + t.Run("--path go installs the go subtree package", func(t *testing.T) { + gi := &GitInstaller{AgentFieldHome: t.TempDir(), Subdir: "go"} + root, err := gi.resolvePackageRoot(clone) + if err != nil { + t.Fatalf("resolvePackageRoot: %v", err) + } + if root != goSub { + t.Fatalf("resolved root %q; want %q", root, goSub) + } + md, err := ParsePackageMetadata(root) + if err != nil { + t.Fatalf("parse: %v", err) + } + if md.Name != "go-node" { + t.Fatalf("--path go picked %q; want go-node", md.Name) + } + if !md.IsGo() { + t.Fatalf("go subtree package should be a Go node") + } + }) + + t.Run("--path with no manifest errors, naming the path", func(t *testing.T) { + gi := &GitInstaller{AgentFieldHome: t.TempDir(), Subdir: "missing"} + _, err := gi.resolvePackageRoot(clone) + if err == nil { + t.Fatal("expected error for a subdir with no manifest") + } + want := filepath.Join(clone, "missing", "agentfield-package.yaml") + if !strings.Contains(err.Error(), want) { + t.Fatalf("error %q should name %q", err.Error(), want) + } + }) + + t.Run("absolute and escaping --path rejected before clone work", func(t *testing.T) { + for _, bad := range []string{"/abs", "../escape"} { + gi := &GitInstaller{AgentFieldHome: t.TempDir(), Subdir: bad} + // resolvePackageRoot rejects it... + if _, err := gi.resolvePackageRoot(clone); err == nil { + t.Fatalf("resolvePackageRoot(%q) should error", bad) + } + // ...and InstallFromGit rejects it up front with no git available, + // proving the selector is validated before any clone is attempted. + if err := gi.InstallFromGit("https://github.com/acme/repo", false); err == nil { + t.Fatalf("InstallFromGit with --path %q should reject before cloning", bad) + } + } + }) +} + +// Contract 5: the @ref URL pin and the --path selector are independent and both +// honored — ref is parsed from the URL, subdir is resolved after clone. +func TestInstallPathComposesWithRef(t *testing.T) { + t.Parallel() + clone := t.TempDir() + goSub := writeMultiNodeClone(t, clone) + + info, err := ParseGitURL("https://github.com/acme/repo@v1.2.3") + if err != nil { + t.Fatalf("ParseGitURL: %v", err) + } + if info.Ref != "v1.2.3" { + t.Fatalf("ref=%q; want v1.2.3 (the @ref must still be parsed with --path in play)", info.Ref) + } + if info.CloneURL != "https://github.com/acme/repo" { + t.Fatalf("cloneURL=%q; want the ref stripped", info.CloneURL) + } + + gi := &GitInstaller{AgentFieldHome: t.TempDir(), Subdir: "go"} + root, err := gi.resolvePackageRoot(clone) + if err != nil { + t.Fatalf("resolvePackageRoot: %v", err) + } + if root != goSub { + t.Fatalf("subdir resolution %q; want %q — ref and path must compose", root, goSub) + } +} + +// Contract 6: a Go subdir package builds relative to the subdir-as-package-root. +// The stubbed toolchain materializes the -o output; asserting it lands under the +// subdir (and not the repo root) proves the build is rooted at the subdir. +func TestInstallPathGoSubdirBuildsRelativeToSubdir(t *testing.T) { + clone := t.TempDir() + goSub := writeMultiNodeClone(t, clone) + stubGo(t, "1.21.0") + + gi := &GitInstaller{AgentFieldHome: t.TempDir(), Subdir: "go"} + root, err := gi.resolvePackageRoot(clone) + if err != nil { + t.Fatalf("resolvePackageRoot: %v", err) + } + md, err := ParsePackageMetadata(root) + if err != nil { + t.Fatalf("parse: %v", err) + } + if err := InstallDependencies(root, md); err != nil { + t.Fatalf("InstallDependencies (go subdir): %v", err) + } + if _, err := os.Stat(filepath.Join(goSub, "bin", "node")); err != nil { + t.Fatalf("expected the Go binary at /bin/node: %v", err) + } + if _, err := os.Stat(filepath.Join(clone, "bin", "node")); !os.IsNotExist(err) { + t.Fatalf("build must be rooted at the subdir, not the repo root") + } +} diff --git a/control-plane/internal/packages/installer.go b/control-plane/internal/packages/installer.go index 45b66d175..17f59e968 100644 --- a/control-plane/internal/packages/installer.go +++ b/control-plane/internal/packages/installer.go @@ -920,6 +920,66 @@ func fileExistsAt(dir, name string) bool { return err == nil } +// validateSubdirSelector checks the *syntax* of a `--path` subdirectory selector +// without touching the filesystem, so it can be enforced before any install work +// (e.g. before cloning a repo). An empty selector is valid (no selection). A +// non-empty selector must be relative and must not escape the source root via +// "..". Absolute paths and escaping paths are rejected with an actionable message. +func validateSubdirSelector(subdir string) error { + subdir = strings.TrimSpace(subdir) + if subdir == "" { + return nil + } + if filepath.IsAbs(subdir) { + return fmt.Errorf("--path must be a subdirectory relative to the package root, not an absolute path (got %q)", subdir) + } + clean := filepath.Clean(subdir) + if clean == ".." || strings.HasPrefix(clean, ".."+string(filepath.Separator)) { + return fmt.Errorf("--path %q must stay within the package root (it may not use %q to escape)", subdir, "..") + } + return nil +} + +// ValidateSubdirSelector is the exported form of validateSubdirSelector: it +// checks that a `--path` selector is syntactically safe (relative, non-escaping) +// without requiring the source to be present on disk. Callers that need to reject +// a bad selector before doing any install work (e.g. before a git clone) use this. +func ValidateSubdirSelector(subdir string) error { + return validateSubdirSelector(subdir) +} + +// ResolvePackageSubdir resolves a `--path` subdirectory selector against a source +// root (a cloned git repository directory or a local source directory) and returns +// the package root to install. It enforces that: +// - subdir is relative (absolute paths are rejected), +// - subdir does not escape root via "..", +// - an agentfield-package.yaml exists at the resolved directory. +// +// An empty subdir returns root unchanged (the caller handles the no-selector +// root-first walk itself). A missing manifest is reported with the full expected +// path so the user can see exactly where it was looked for. +func ResolvePackageSubdir(root, subdir string) (string, error) { + if err := validateSubdirSelector(subdir); err != nil { + return "", err + } + subdir = strings.TrimSpace(subdir) + if subdir == "" { + return root, nil + } + target := filepath.Join(root, filepath.Clean(subdir)) + // Defense in depth: after joining, confirm the target is still contained in + // root (guards against any residual traversal the syntax check missed). + if rel, err := filepath.Rel(root, target); err != nil || + rel == ".." || strings.HasPrefix(rel, ".."+string(filepath.Separator)) { + return "", fmt.Errorf("--path %q resolves outside the package root", subdir) + } + manifest := filepath.Join(target, "agentfield-package.yaml") + if _, err := os.Stat(manifest); err != nil { + return "", fmt.Errorf("no agentfield-package.yaml found for --path %q (expected at %s)", subdir, manifest) + } + return target, nil +} + // hasRequirementsFile checks if requirements.txt exists func (pi *PackageInstaller) hasRequirementsFile(packagePath string) bool { requirementsPath := filepath.Join(packagePath, "requirements.txt") diff --git a/docs/installing-agent-nodes.md b/docs/installing-agent-nodes.md index b16efce4d..4a1f33c53 100644 --- a/docs/installing-agent-nodes.md +++ b/docs/installing-agent-nodes.md @@ -219,6 +219,33 @@ doesn't already have, recursively. Already-installed nodes are skipped, which also breaks dependency cycles. `af run ` starts a node's installed dependencies first (in dependency order) before the node itself. +## One repo, many nodes: `--path` + +By default `af install ` looks for the `agentfield-package.yaml` at the root +of the source (a git repo or a local directory). When a single repository ships +more than one installable node — for example a Python node at the root and a Go +port under `go/` — use `--path` to select the subdirectory to install: + +```bash +# Install the node whose manifest lives at go/agentfield-package.yaml +af install https://github.com/Agent-Field/SWE-AF --path go + +# Composes with an @ref pin on the URL +af install https://github.com/Agent-Field/SWE-AF@v1.2.3 --path go + +# Also works for a local source +af install ./SWE-AF --path go +``` + +The subdirectory must contain its own `agentfield-package.yaml`; that subtree +becomes the package root that is copied to `~/.agentfield/packages/` and +installed (a Go node builds relative to it). Because registry entries are keyed by +the manifest `name`, the root node and a `--path` node from the same repo coexist +as separate installs. `--path` is a path **relative to the source root**: absolute +paths and paths that escape the root with `..` are rejected, and a missing manifest +is reported with the full expected path. A bare `af install ` (no `--path`) is +unchanged — the root manifest is always what you get by default. + ## Secrets: encrypted, shared, runtime-only Secrets are never written to disk in plaintext and never baked into the package. @@ -267,6 +294,7 @@ server URL is resolved from your local configuration. | Command | Does | | --------------------------- | -------------------------------------------------------------- | | `af install ` | Install from a local path, git URL, or registry name + node deps | +| `af install --path ` | Install the node in `` (one repo shipping multiple nodes) | | `af run ` | Start a node (and its node deps) in the background | | `af list` | Show installed nodes and runtime state | | `af logs ` | Tail a node's process log | diff --git a/sdk/go/agent/agent.go b/sdk/go/agent/agent.go index 872b64aa8..89b02ec4a 100644 --- a/sdk/go/agent/agent.go +++ b/sdk/go/agent/agent.go @@ -9,12 +9,15 @@ import ( "io" "log" "math/rand" + "net" "net/http" "net/url" "os" "runtime" + "strconv" "strings" "sync" + "syscall" "time" "github.com/Agent-Field/agentfield/sdk/go/ai" @@ -365,11 +368,16 @@ type Config struct { // plane does not expect heartbeats. LeaseRefreshInterval time.Duration - // CallTimeout bounds every outbound HTTP call this agent makes as a - // client - cross-agent Call()s, memory backend requests, etc. - // Optional. Default: 15s. A reasoning-model-backed reasoner chained - // behind Call() (search + a large max_tokens reasoning response) can - // easily exceed the old hardcoded 15s, so raise this for such workloads. + // CallTimeout bounds every INDIVIDUAL outbound HTTP request this agent + // makes through its shared client (e.g. memory backend requests). + // Optional. Default: 15s. + // + // NOTE: cross-agent Call() does NOT use this timeout. Its async submit and + // status polls run through dedicated clients bounded by + // AGENTFIELD_CALL_SUBMIT_TIMEOUT_SECONDS / AGENTFIELD_CALL_POLL_TIMEOUT_SECONDS, + // with transient failures retried within AGENTFIELD_CALL_RETRY_WINDOW_SECONDS, + // so a child reasoner may run arbitrarily long; bound the overall Call wait + // with the ctx passed to Call instead. CallTimeout time.Duration // DisableLeaseLoop disables automatic periodic lease refreshes. @@ -474,11 +482,24 @@ type Agent struct { cfg Config client *client.Client httpClient *http.Client - reasoners map[string]*Reasoner - skills map[string]*Reasoner - sessions map[string]SessionDefinition - aiClient *ai.Client // AI/LLM client - memory *Memory // Memory system for state management + + // callSubmitClient and callPollClient are dedicated HTTP clients for the + // async Agent.Call path. They are separate from httpClient so the submit + // and poll requests carry their own (larger) per-request timeouts without + // affecting memory-backend or other client traffic. Resolved once in New() + // from AGENTFIELD_CALL_SUBMIT_TIMEOUT_SECONDS / AGENTFIELD_CALL_POLL_TIMEOUT_SECONDS. + callSubmitClient *http.Client + callPollClient *http.Client + // callRetryWindow bounds how long consecutive transient poll/submit + // failures are tolerated before the call is declared failed with an + // "unreachable" error. Resolved from AGENTFIELD_CALL_RETRY_WINDOW_SECONDS. + callRetryWindow time.Duration + + reasoners map[string]*Reasoner + skills map[string]*Reasoner + sessions map[string]SessionDefinition + aiClient *ai.Client // AI/LLM client + memory *Memory // Memory system for state management // DID/VC subsystem didManager *did.Manager @@ -559,6 +580,17 @@ func New(cfg Config) (*Agent, error) { Timeout: cfg.CallTimeout, } + // Dedicated clients + retry window for the async Agent.Call path. See the + // resolver's doc comment for the env vars and defaults. A slow-but-healthy + // control plane must not abort an already-accepted submit, so the submit + // client's timeout is intentionally generous (default 120s), and each poll + // gets its own (default 60s) rather than inheriting CallTimeout's 15s. + callSubmitTimeout := resolveCallDurationEnv(envCallSubmitTimeoutSeconds, defaultCallSubmitTimeout) + callPollTimeout := resolveCallDurationEnv(envCallPollTimeoutSeconds, defaultCallPollTimeout) + callRetryWindow := resolveCallDurationEnv(envCallRetryWindowSeconds, defaultCallRetryWindow) + callSubmitClient := &http.Client{Timeout: callSubmitTimeout} + callPollClient := &http.Client{Timeout: callPollTimeout} + // Initialize AI client if config provided var aiClient *ai.Client var err error @@ -572,6 +604,9 @@ func New(cfg Config) (*Agent, error) { a := &Agent{ cfg: cfg, httpClient: httpClient, + callSubmitClient: callSubmitClient, + callPollClient: callPollClient, + callRetryWindow: callRetryWindow, reasoners: make(map[string]*Reasoner), skills: make(map[string]*Reasoner), sessions: make(map[string]SessionDefinition), @@ -1169,6 +1204,21 @@ func (a *Agent) handleExecute(w http.ResponseWriter, r *http.Request) { writeJSON(w, statusCode, response) return } + // A reasoner that ran but failed can return &ReasonerFailed{Result: ...} + // to preserve its structured outcome; carry it onto the error response + // (mirroring the async status payload) so the result is not lost. + var rf *ReasonerFailed + if errors.As(err, &rf) { + response := map[string]any{"error": rf.Message} + if rf.Result != nil { + response["result"] = rf.Result + } + if rf.ErrorDetails != nil { + response["error_details"] = rf.ErrorDetails + } + writeJSON(w, http.StatusInternalServerError, response) + return + } writeJSON(w, http.StatusInternalServerError, map[string]any{"error": err.Error()}) return } @@ -1372,6 +1422,20 @@ func (a *Agent) handleReasoner(w http.ResponseWriter, r *http.Request) { writeJSON(w, statusCode, response) return } + // Preserve a ReasonerFailed's structured result/details on the sync + // error response, mirroring the async failed-status payload. + var rf *ReasonerFailed + if errors.As(err, &rf) { + response := map[string]any{"error": rf.Message} + if rf.Result != nil { + response["result"] = rf.Result + } + if rf.ErrorDetails != nil { + response["error_details"] = rf.ErrorDetails + } + writeJSON(w, http.StatusInternalServerError, response) + return + } writeJSON(w, http.StatusInternalServerError, map[string]any{ "error": err.Error(), @@ -1484,6 +1548,21 @@ func (a *Agent) executeReasonerAsync(reasoner *Reasoner, input map[string]any, e if err != nil { payload["status"] = "failed" payload["error"] = err.Error() + // A reasoner that ran, determined its own work failed, and wants its + // structured outcome preserved returns &ReasonerFailed{Result: ...}. + // Carry that result/details onto the failed-status payload so the single + // (5x-retried) status post records status=failed WITHOUT discarding the + // rich result — the control plane stores the result regardless of + // terminal status. Byte-parity with the Python async handler. + var rf *ReasonerFailed + if errors.As(err, &rf) { + if rf.Result != nil { + payload["result"] = rf.Result + } + if rf.ErrorDetails != nil { + payload["error_details"] = rf.ErrorDetails + } + } a.logExecutionError(ctx, "reasoner.invoke.failed", "reasoner execution failed", map[string]any{ "reasoner_id": reasoner.Name, "mode": "async", @@ -1565,7 +1644,179 @@ func (a *Agent) postExecutionStatus(ctx context.Context, callbackURL string, pay return lastErr } +// Poll pacing for Call's async-submit + wait loop. Mirrors the Python SDK's +// _await_execution_sync (sdk/python/agentfield/client.py:970-1011) with the +// AsyncConfig defaults from sdk/python/agentfield/async_config.py: +// interval starts at max(initial_poll_interval, 0.25s), doubles per poll +// capped at max_poll_interval (4s), and each sleep is jittered by +// uniform(0.8, 1.2) clamped to [50ms, 4s] (client.py:1141-1143). +const ( + callInitialPollInterval = 250 * time.Millisecond + callMaxPollInterval = 4 * time.Second + callMinPollInterval = 50 * time.Millisecond +) + +// Env vars controlling Agent.Call resilience to transient control-plane +// outages. They follow the AGENTFIELD_* integer-seconds convention used by +// AGENTFIELD_HARNESS_IDLE_SECONDS (sdk/go/harness/cli.go). All are read once +// per Agent in New() and cached on the struct. +const ( + // envCallPollTimeoutSeconds bounds each individual status-poll HTTP + // request. Default 60s. + envCallPollTimeoutSeconds = "AGENTFIELD_CALL_POLL_TIMEOUT_SECONDS" + // envCallSubmitTimeoutSeconds bounds the async-submit POST. Default 120s — + // deliberately generous so a slow-but-healthy CP does not abort a request + // that may already have been accepted (re-POSTing is not idempotent). + envCallSubmitTimeoutSeconds = "AGENTFIELD_CALL_SUBMIT_TIMEOUT_SECONDS" + // envCallRetryWindowSeconds bounds how long consecutive transient failures + // (poll timeouts/5xx, or submit dial failures) are tolerated before the + // call fails with an "unreachable" error. Default 300s. A single success + // resets the window. + envCallRetryWindowSeconds = "AGENTFIELD_CALL_RETRY_WINDOW_SECONDS" +) + +const ( + defaultCallPollTimeout = 60 * time.Second + defaultCallSubmitTimeout = 120 * time.Second + defaultCallRetryWindow = 5 * time.Minute + + // callRetryBackoffMin/Max bound the exponential backoff BETWEEN consecutive + // failed submit/poll attempts (distinct from the healthy poll pacing above). + callRetryBackoffMin = 500 * time.Millisecond + callRetryBackoffMax = 15 * time.Second + + // callNotFoundRetryWindow bounds how long a 404 on a just-submitted + // execution is retried before failing with a distinct error. The control + // plane can briefly lag between accepting a submit and making the execution + // row queryable, so a 404 is treated as transient for a SHORTER window than + // a general outage. The effective window is min(this, callRetryWindow) so a + // shrunk retry window (tests) shrinks the 404 window too. + callNotFoundRetryWindow = 30 * time.Second +) + +// resolveCallDurationEnv reads an integer-seconds env var, falling back to def +// when unset, unparseable, or non-positive. Mirrors resolveIdleSeconds' laxity. +func resolveCallDurationEnv(name string, def time.Duration) time.Duration { + raw := strings.TrimSpace(os.Getenv(name)) + if raw == "" { + return def + } + v, err := strconv.Atoi(raw) + if err != nil || v <= 0 { + return def + } + return time.Duration(v) * time.Second +} + +// requestNeverReachedServer reports whether an HTTP client error proves the +// request never reached the server, so re-sending it cannot cause a duplicate +// side effect. Only dial-phase failures qualify: the connection was never +// established, hence no bytes of the request body were delivered. Ambiguous +// failures (a timeout while awaiting response headers — the server may have +// accepted and be processing the request) and caller cancellation return +// false, so a non-idempotent submit is never blindly retried. +func requestNeverReachedServer(err error) bool { + if err == nil { + return false + } + // Caller-driven cancellation / deadline is not a "never reached" signal: + // the request may have been sent before the caller gave up. + if errors.Is(err, context.Canceled) || errors.Is(err, context.DeadlineExceeded) { + return false + } + // Connection actively refused: nothing is listening, so no request landed. + if errors.Is(err, syscall.ECONNREFUSED) { + return true + } + // DNS resolution failed: the request never left the client. + var dnsErr *net.DNSError + if errors.As(err, &dnsErr) { + return true + } + // A net.OpError whose Op is "dial" means the failure happened while + // establishing the connection, before any request bytes were written. + var opErr *net.OpError + if errors.As(err, &opErr) && opErr.Op == "dial" { + return true + } + return false +} + +// isTransientPollStatus reports whether an HTTP status returned by a status +// poll is a transient control-plane condition worth retrying (overload, gateway +// hiccups) rather than a permanent client error. 404 is handled separately +// (bounded, distinct message) because it means "not visible yet", and other 4xx +// (auth, bad request) are permanent and abort immediately. +func isTransientPollStatus(code int) bool { + switch code { + case http.StatusRequestTimeout, // 408 + http.StatusTooManyRequests: // 429 + return true + } + return code >= 500 +} + +// nextRetryBackoff doubles the failure backoff, clamped to +// [callRetryBackoffMin, callRetryBackoffMax]. +func nextRetryBackoff(current time.Duration) time.Duration { + next := current * 2 + if next > callRetryBackoffMax { + return callRetryBackoffMax + } + if next < callRetryBackoffMin { + return callRetryBackoffMin + } + return next +} + +// sleepCtx sleeps for d unless ctx is cancelled first. It returns true if the +// full duration elapsed, false if ctx was cancelled during the wait. +func sleepCtx(ctx context.Context, d time.Duration) bool { + if d <= 0 { + return ctx.Err() == nil + } + t := time.NewTimer(d) + defer t.Stop() + select { + case <-ctx.Done(): + return false + case <-t.C: + return true + } +} + +// nextCallPollInterval applies the Python _next_poll_interval jitter: +// uniform(0.8, 1.2) * current, clamped to [callMinPollInterval, callMaxPollInterval]. +func nextCallPollInterval(current time.Duration) time.Duration { + jittered := time.Duration(float64(current) * (0.8 + 0.4*rand.Float64())) + if jittered < callMinPollInterval { + return callMinPollInterval + } + if jittered > callMaxPollInterval { + return callMaxPollInterval + } + return jittered +} + // Call invokes another reasoner via the AgentField control plane, preserving execution context. +// +// The call is submitted to the asynchronous execute endpoint +// (POST /api/v1/execute/async/{target}) and the result is awaited by polling +// GET /api/v1/executions/{execution_id} until the execution reaches a terminal +// status — mirroring the Python SDK's app.call +// (sdk/python/agentfield/client.py _submit_execution_sync/_await_execution_sync). +// +// Timeout semantics: each submit and each poll is an independent HTTP request +// bounded by its own dedicated client timeout +// (AGENTFIELD_CALL_SUBMIT_TIMEOUT_SECONDS / AGENTFIELD_CALL_POLL_TIMEOUT_SECONDS), +// NOT by Config.CallTimeout and NOT by the end-to-end call. Transient submit +// and poll failures are retried within AGENTFIELD_CALL_RETRY_WINDOW_SECONDS +// (see submitAsyncExecution / awaitExecutionResult), so a brief control-plane +// outage does not kill a long-running call. A child reasoner may run +// arbitrarily long; the overall wait is bounded only by ctx. Cancelling ctx +// aborts the wait but does NOT cancel the child execution server-side — the +// child keeps running on its node and the control plane records its result +// (the Python SDK behaves the same way when the caller stops waiting). func (a *Agent) Call(ctx context.Context, target string, input map[string]any) (map[string]any, error) { if strings.TrimSpace(a.cfg.AgentFieldURL) == "" { return nil, errors.New("AgentFieldURL is required to call other reasoners") @@ -1591,16 +1842,26 @@ func (a *Agent) Call(ctx context.Context, target string, input map[string]any) ( "target": target, "reasoner_id": strings.TrimPrefix(target, a.cfg.NodeID+"."), }) - url := fmt.Sprintf("%s/api/v1/execute/%s", strings.TrimSuffix(a.cfg.AgentFieldURL, "/"), strings.TrimPrefix(target, "/")) - req, err := http.NewRequestWithContext(ctx, http.MethodPost, url, bytes.NewReader(body)) + + base := strings.TrimSuffix(a.cfg.AgentFieldURL, "/") + + executionID, submittedRunID, err := a.submitAsyncExecution(ctx, base, target, body, execCtx, runID) if err != nil { - a.logExecutionError(ctx, "call.outbound.failed", "failed to build cross-node call request", map[string]any{ - "target": target, - "error": err.Error(), - }) - return nil, fmt.Errorf("build request: %w", err) + return nil, err } - req.Header.Set("Content-Type", "application/json") + if submittedRunID != "" { + runID = submittedRunID + } + + return a.awaitExecutionResult(ctx, base, target, executionID, runID, execCtx) +} + +// applyCallHeaders sets the execution-context lineage headers shared by the +// async submit and every status poll. The control plane reads these via +// readExecutionHeaders (control-plane/internal/handlers/execute.go) through +// the same prepareExecution path for both the sync and async endpoints, so +// workflow DAG parentage is recorded identically. +func (a *Agent) applyCallHeaders(req *http.Request, execCtx ExecutionContext, runID string) { req.Header.Set("Accept", "application/json") req.Header.Set("X-Run-ID", runID) if execCtx.ExecutionID != "" { @@ -1628,28 +1889,108 @@ func (a *Agent) Call(ctx context.Context, target string, input map[string]any) ( if a.cfg.Token != "" { req.Header.Set("Authorization", "Bearer "+a.cfg.Token) } +} - // Sign request with DID auth headers if configured - if a.client != nil { - a.client.SignHTTPRequest(req, body) - } +// submitAsyncExecution POSTs to /api/v1/execute/async/{target} and returns the +// accepted execution's identifiers. Mirrors Python _submit_execution_sync +// (client.py:866-905): any HTTP status >= 400 is an ExecuteError, and a +// response without an execution_id is rejected. +func (a *Agent) submitAsyncExecution( + ctx context.Context, + base, target string, + body []byte, + execCtx ExecutionContext, + runID string, +) (executionID, submittedRunID string, err error) { + url := fmt.Sprintf("%s/api/v1/execute/async/%s", base, strings.TrimPrefix(target, "/")) + + var ( + resp *http.Response + streakStart time.Time + backoff = callRetryBackoffMin + attempt int + ) + for { + attempt++ + req, buildErr := http.NewRequestWithContext(ctx, http.MethodPost, url, bytes.NewReader(body)) + if buildErr != nil { + a.logExecutionError(ctx, "call.outbound.failed", "failed to build cross-node call request", map[string]any{ + "target": target, + "error": buildErr.Error(), + }) + return "", "", fmt.Errorf("build request: %w", buildErr) + } + req.Header.Set("Content-Type", "application/json") + a.applyCallHeaders(req, execCtx, runID) - resp, err := a.httpClient.Do(req) - if err != nil { - a.logExecutionError(ctx, "call.outbound.failed", "cross-node call failed", map[string]any{ - "target": target, - "error": err.Error(), + // Sign request with DID auth headers if configured. Re-signed on every + // attempt so a retry carries a fresh signature timestamp. + if a.client != nil { + a.client.SignHTTPRequest(req, body) + } + + var doErr error + resp, doErr = a.callSubmitClient.Do(req) + if doErr == nil { + break + } + // Caller-driven cancellation/deadline: surface verbatim, never retry. + if ctxErr := ctx.Err(); ctxErr != nil { + a.logExecutionError(ctx, "call.outbound.failed", "cross-node call submit cancelled", map[string]any{ + "target": target, + "error": ctxErr.Error(), + }) + return "", "", ctxErr + } + // Re-POSTing execute/async is NOT idempotent — a duplicate would + // double-run the target reasoner. Retry ONLY when the request provably + // never reached the server (dial/DNS/connection-refused); every + // ambiguous failure (e.g. a timeout awaiting headers, where the CP may + // already have accepted the request) fails without a retry. The submit + // client's generous timeout (default 120s) is what protects a + // slow-but-healthy CP from a premature abort. + if !requestNeverReachedServer(doErr) { + a.logExecutionError(ctx, "call.outbound.failed", "cross-node call failed", map[string]any{ + "target": target, + "error": doErr.Error(), + }) + return "", "", fmt.Errorf("perform execute call (not retried to avoid double-execution): %w", doErr) + } + now := time.Now() + if streakStart.IsZero() { + streakStart = now + } + elapsed := now.Sub(streakStart) + if elapsed >= a.callRetryWindow { + a.logExecutionError(ctx, "call.outbound.failed", "cross-node call submit unreachable", map[string]any{ + "target": target, + "attempts": attempt, + "elapsed_ms": elapsed.Milliseconds(), + "error": doErr.Error(), + }) + return "", "", fmt.Errorf("control plane unreachable for %s: submit to %s never connected after %d attempts: %w", + elapsed.Round(time.Second), target, attempt, doErr) + } + a.logExecutionWarn(ctx, "call.outbound.submit_retry", "cross-node call submit failed to connect, retrying", map[string]any{ + "target": target, + "attempt": attempt, + "elapsed_ms": elapsed.Milliseconds(), + "window_ms": a.callRetryWindow.Milliseconds(), + "error": doErr.Error(), }) - return nil, fmt.Errorf("perform execute call: %w", err) + if !sleepCtx(ctx, backoff) { + return "", "", ctx.Err() + } + backoff = nextRetryBackoff(backoff) } defer resp.Body.Close() bodyBytes, err := io.ReadAll(resp.Body) if err != nil { - return nil, fmt.Errorf("read execute response: %w", err) + return "", "", fmt.Errorf("read execute response: %w", err) } - if resp.StatusCode != http.StatusOK { + if resp.StatusCode >= 400 { // Try to parse structured error from control plane response. var errResp struct { Error string `json:"error"` @@ -1661,7 +2002,7 @@ func (a *Agent) Call(ctx context.Context, target string, input map[string]any) ( "status": resp.StatusCode, "error": errResp.Error, }) - return nil, &ExecuteError{ + return "", "", &ExecuteError{ StatusCode: resp.StatusCode, Message: errResp.Error, ErrorDetails: errResp.ErrorDetails, @@ -1671,57 +2012,335 @@ func (a *Agent) Call(ctx context.Context, target string, input map[string]any) ( "target": target, "status": resp.StatusCode, }) - return nil, &ExecuteError{ + return "", "", &ExecuteError{ StatusCode: resp.StatusCode, Message: fmt.Sprintf("execute failed (%d): %s", resp.StatusCode, strings.TrimSpace(string(bodyBytes))), } } - var execResp struct { - ExecutionID string `json:"execution_id"` - RunID string `json:"run_id"` - Status string `json:"status"` - Result map[string]any `json:"result"` - ErrorMessage *string `json:"error_message"` - ErrorDetails interface{} `json:"error_details"` + var submitResp struct { + ExecutionID string `json:"execution_id"` + RunID string `json:"run_id"` + Status string `json:"status"` } - if err := json.Unmarshal(bodyBytes, &execResp); err != nil { + if err := json.Unmarshal(bodyBytes, &submitResp); err != nil { a.logExecutionError(ctx, "call.outbound.failed", "failed to decode cross-node call response", map[string]any{ "target": target, "error": err.Error(), }) - return nil, fmt.Errorf("decode execute response: %w", err) + return "", "", fmt.Errorf("decode execute response: %w", err) } - - if execResp.ErrorMessage != nil && *execResp.ErrorMessage != "" { - a.logExecutionError(ctx, "call.outbound.failed", "cross-node call returned execution error", map[string]any{ + if submitResp.ExecutionID == "" { + a.logExecutionError(ctx, "call.outbound.failed", "async submission missing execution id", map[string]any{ "target": target, - "error": *execResp.ErrorMessage, }) - return nil, &ExecuteError{ - StatusCode: resp.StatusCode, - Message: *execResp.ErrorMessage, - ErrorDetails: execResp.ErrorDetails, + return "", "", errors.New("execution submission missing identifiers") + } + + return submitResp.ExecutionID, submitResp.RunID, nil +} + +// awaitExecutionResult polls GET /api/v1/executions/{execution_id} until the +// execution reaches a terminal status. Statuses are normalized, "succeeded" +// returns the result, and {failed, cancelled, timeout} surface the execution +// error (with the status endpoint's "error" field coalesced into the message — +// Python parity, client.py:1000-1002). +// +// Resilience to transient control-plane outages (this is the key departure from +// the Python SDK, whose _await_execution_sync raise_for_status()es on every +// poll and so lets a single blip kill a 30-minute call): a transient poll +// failure — a transport error/timeout, or a 408/429/5xx — does NOT fail the +// call. Consecutive failures are retried with exponential backoff (capped at +// callRetryBackoffMax) for up to a.callRetryWindow of UNBROKEN failure; a single +// successful poll resets that window. Only when the window is exceeded does the +// call fail with a "control plane unreachable for Xs" error. A 404 (execution +// not yet queryable right after submit) is retried within a shorter bounded +// window and then fails with a distinct message. Other 4xx are permanent and +// abort immediately. Each poll request is bounded by callPollClient's timeout; +// the overall wait is otherwise bounded only by ctx. +func (a *Agent) awaitExecutionResult( + ctx context.Context, + base, target, executionID, runID string, + execCtx ExecutionContext, +) (map[string]any, error) { + pollURL := fmt.Sprintf("%s/api/v1/executions/%s", base, url.PathEscape(executionID)) + interval := callInitialPollInterval + + var ( + transientStreakStart time.Time + notFoundStreakStart time.Time + failCount int + failBackoff = callRetryBackoffMin + ) + + // resetFailureStreak clears the consecutive-failure bookkeeping — called + // after any successful poll, honoring "a successful poll resets the window". + resetFailureStreak := func() { + transientStreakStart = time.Time{} + notFoundStreakStart = time.Time{} + failCount = 0 + failBackoff = callRetryBackoffMin + } + + // recordTransientFailure advances the transient-failure window. It returns + // retry=true (keep polling after a backoff) until the window is exceeded, + // at which point it returns the terminal "unreachable" error. + recordTransientFailure := func(cause error) (bool, error) { + now := time.Now() + if transientStreakStart.IsZero() { + transientStreakStart = now + } + notFoundStreakStart = time.Time{} + failCount++ + elapsed := now.Sub(transientStreakStart) + if elapsed >= a.callRetryWindow { + a.logExecutionError(ctx, "call.outbound.failed", "cross-node call control plane unreachable", map[string]any{ + "target": target, + "execution_id": executionID, + "attempts": failCount, + "elapsed_ms": elapsed.Milliseconds(), + "error": cause.Error(), + }) + return false, fmt.Errorf("control plane unreachable for %s: %d consecutive failed polls of execution %s: %w", + elapsed.Round(time.Second), failCount, executionID, cause) } + a.logExecutionWarn(ctx, "call.outbound.poll_retry", "cross-node call status poll failed, retrying", map[string]any{ + "target": target, + "execution_id": executionID, + "attempt": failCount, + "elapsed_ms": elapsed.Milliseconds(), + "window_ms": a.callRetryWindow.Milliseconds(), + "error": cause.Error(), + }) + return true, nil } - if !strings.EqualFold(execResp.Status, "succeeded") { - a.logExecutionError(ctx, "call.outbound.failed", "cross-node call did not succeed", map[string]any{ - "target": target, - "status": execResp.Status, + + // recordNotFound advances the shorter 404 window (the CP may briefly lag + // between accepting a submit and making the execution row queryable). + recordNotFound := func() (bool, error) { + now := time.Now() + if notFoundStreakStart.IsZero() { + notFoundStreakStart = now + } + transientStreakStart = time.Time{} + failCount++ + window := callNotFoundRetryWindow + if a.callRetryWindow < window { + window = a.callRetryWindow + } + elapsed := now.Sub(notFoundStreakStart) + if elapsed >= window { + a.logExecutionError(ctx, "call.outbound.failed", "cross-node call execution not found", map[string]any{ + "target": target, + "execution_id": executionID, + "attempts": failCount, + "elapsed_ms": elapsed.Milliseconds(), + }) + return false, &ExecuteError{ + StatusCode: http.StatusNotFound, + Message: fmt.Sprintf("execution %s not found on control plane after %s (submit was accepted but the execution never became queryable)", + executionID, elapsed.Round(time.Second)), + } + } + a.logExecutionWarn(ctx, "call.outbound.poll_retry", "cross-node call execution not yet visible, retrying", map[string]any{ + "target": target, + "execution_id": executionID, + "attempt": failCount, + "elapsed_ms": elapsed.Milliseconds(), + "reason": "not_found", }) - return nil, &ExecuteError{ - StatusCode: resp.StatusCode, - Message: fmt.Sprintf("execute status %s", execResp.Status), - ErrorDetails: execResp.ErrorDetails, + return true, nil + } + + // backoffAfterFailure sleeps the current failure backoff (honoring ctx) and + // advances it. Returns false if ctx was cancelled during the wait. + backoffAfterFailure := func() bool { + if !sleepCtx(ctx, failBackoff) { + return false } + failBackoff = nextRetryBackoff(failBackoff) + return true } - a.logExecutionInfo(ctx, "call.outbound.complete", "cross-node call completed", map[string]any{ - "target": target, - "execution_id": execResp.ExecutionID, - "run_id": execResp.RunID, - }) - return execResp.Result, nil + for { + if err := ctx.Err(); err != nil { + a.logExecutionError(ctx, "call.outbound.failed", "cross-node call wait cancelled", map[string]any{ + "target": target, + "execution_id": executionID, + "error": err.Error(), + }) + return nil, err + } + + req, err := http.NewRequestWithContext(ctx, http.MethodGet, pollURL, nil) + if err != nil { + return nil, fmt.Errorf("build status request: %w", err) + } + a.applyCallHeaders(req, execCtx, runID) + if a.client != nil { + a.client.SignHTTPRequest(req, nil) + } + + resp, err := a.callPollClient.Do(req) + if err != nil { + // Distinguish caller cancellation from transport failures so the + // ctx-cancel contract surfaces context.Canceled/DeadlineExceeded. + if ctxErr := ctx.Err(); ctxErr != nil { + a.logExecutionError(ctx, "call.outbound.failed", "cross-node call wait cancelled", map[string]any{ + "target": target, + "execution_id": executionID, + "error": ctxErr.Error(), + }) + return nil, ctxErr + } + // A transport error/timeout is transient: retry within the window + // instead of killing a call that may have been running for a long + // time (the observed real-world failure — one timed-out poll aborted + // a 30-minute call). + retry, failErr := recordTransientFailure(fmt.Errorf("poll execution status: %w", err)) + if !retry { + return nil, failErr + } + if !backoffAfterFailure() { + return nil, ctx.Err() + } + continue + } + + bodyBytes, readErr := io.ReadAll(resp.Body) + _ = resp.Body.Close() + if readErr != nil { + return nil, fmt.Errorf("read execution status: %w", readErr) + } + + // 404: the execution row may not be queryable yet right after submit. + // Retry within a bounded (shorter) window, then fail distinctly. + if resp.StatusCode == http.StatusNotFound { + retry, failErr := recordNotFound() + if !retry { + return nil, failErr + } + if !backoffAfterFailure() { + return nil, ctx.Err() + } + continue + } + + // Transient server-side conditions (408/429/5xx): retry within the + // failure window rather than aborting the call. + if isTransientPollStatus(resp.StatusCode) { + retry, failErr := recordTransientFailure(fmt.Errorf("poll execution status %d: %s", + resp.StatusCode, strings.TrimSpace(string(bodyBytes)))) + if !retry { + return nil, failErr + } + if !backoffAfterFailure() { + return nil, ctx.Err() + } + continue + } + + // Other 4xx (auth, bad request): a persistent client error retrying + // cannot fix — abort with the status. + if resp.StatusCode >= 400 { + a.logExecutionError(ctx, "call.outbound.failed", "cross-node call status poll returned error status", map[string]any{ + "target": target, + "execution_id": executionID, + "status": resp.StatusCode, + }) + return nil, &ExecuteError{ + StatusCode: resp.StatusCode, + Message: fmt.Sprintf("execution status failed (%d): %s", resp.StatusCode, strings.TrimSpace(string(bodyBytes))), + } + } + + // A genuine 2xx answer from the CP resets the failure window. + resetFailureStreak() + + var statusResp struct { + ExecutionID string `json:"execution_id"` + RunID string `json:"run_id"` + Status string `json:"status"` + Result json.RawMessage `json:"result"` + Error *string `json:"error"` + ErrorMessage *string `json:"error_message"` + ErrorDetails interface{} `json:"error_details"` + } + if err := json.Unmarshal(bodyBytes, &statusResp); err != nil { + a.logExecutionError(ctx, "call.outbound.failed", "failed to decode execution status response", map[string]any{ + "target": target, + "execution_id": executionID, + "error": err.Error(), + }) + return nil, fmt.Errorf("decode execute response: %w", err) + } + + status := types.NormalizeStatus(statusResp.Status) + + if status == types.ExecutionStatusSucceeded { + var result map[string]any + if len(statusResp.Result) > 0 && string(statusResp.Result) != "null" { + if err := json.Unmarshal(statusResp.Result, &result); err != nil { + a.logExecutionError(ctx, "call.outbound.failed", "failed to decode cross-node call result", map[string]any{ + "target": target, + "execution_id": executionID, + "error": err.Error(), + }) + return nil, fmt.Errorf("decode execute response: %w", err) + } + } + a.logExecutionInfo(ctx, "call.outbound.complete", "cross-node call completed", map[string]any{ + "target": target, + "execution_id": executionID, + "run_id": runID, + }) + return result, nil + } + + if types.TerminalStatuses[status] { + // error_message, falling back to the status endpoint's "error" + // field — Python parity (client.py:1000-1002). + message := "" + if statusResp.ErrorMessage != nil && *statusResp.ErrorMessage != "" { + message = *statusResp.ErrorMessage + } else if statusResp.Error != nil && *statusResp.Error != "" { + message = *statusResp.Error + } + if message == "" { + message = fmt.Sprintf("execute status %s", status) + } + a.logExecutionError(ctx, "call.outbound.failed", "cross-node call did not succeed", map[string]any{ + "target": target, + "execution_id": executionID, + "status": status, + "error": message, + }) + // StatusBadGateway matches what the synchronous execute endpoint + // returns for failed executions, so ExecuteError.StatusCode keeps + // its pre-async meaning for callers that inspect it. + return nil, &ExecuteError{ + StatusCode: http.StatusBadGateway, + Message: message, + ErrorDetails: statusResp.ErrorDetails, + } + } + + // Non-terminal: sleep with jittered exponential backoff, honoring ctx. + select { + case <-ctx.Done(): + a.logExecutionError(ctx, "call.outbound.failed", "cross-node call wait cancelled", map[string]any{ + "target": target, + "execution_id": executionID, + "error": ctx.Err().Error(), + }) + return nil, ctx.Err() + case <-time.After(nextCallPollInterval(interval)): + } + interval *= 2 + if interval > callMaxPollInterval { + interval = callMaxPollInterval + } + } } // emitWorkflowEvent sends a workflow event to the control plane asynchronously. diff --git a/sdk/go/agent/agent_call_coverage_test.go b/sdk/go/agent/agent_call_coverage_test.go new file mode 100644 index 000000000..a09a816f8 --- /dev/null +++ b/sdk/go/agent/agent_call_coverage_test.go @@ -0,0 +1,709 @@ +package agent + +import ( + "context" + "encoding/json" + "errors" + "io" + "log" + "net" + "net/http" + "net/http/httptest" + "net/url" + "strings" + "sync/atomic" + "syscall" + "testing" + "time" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +// These tests close the async-submit + poll error/edge branches of Agent.Call +// that the happy-path tests in agent_test.go do not reach. Each is derived from +// the behavior contract of the async Call rewrite (submit -> poll -> terminal +// mapping, error propagation, ctx handling), not from mirroring the code. + +// callErrBody is a response body whose Read always fails, used to exercise the +// io.ReadAll error branches on both the submit response and each status poll. +type callErrBody struct{} + +func (callErrBody) Read([]byte) (int, error) { return 0, errors.New("simulated body read failure") } +func (callErrBody) Close() error { return nil } + +// newCallTestAgent builds a minimal Agent for exercising Call's helpers. The +// AgentFieldURL is a harmless placeholder — the submit/poll base URL is passed +// explicitly to the helper under test, so no real network is used unless a +// test wires a fake transport. +func newCallTestAgent(t *testing.T, agentFieldURL string) *Agent { + t.Helper() + a, err := New(Config{ + NodeID: "node-1", + Version: "1.0.0", + AgentFieldURL: agentFieldURL, + Logger: log.New(io.Discard, "", 0), + }) + require.NoError(t, err) + return a +} + +// --- nextCallPollInterval clamps ------------------------------------------- + +// Contract: the jittered poll interval is always clamped into +// [callMinPollInterval, callMaxPollInterval]. A tiny input clamps up to the +// floor; a huge input clamps down to the ceiling; an in-range input is returned +// jittered without clamping. +func TestNextCallPollInterval_Clamps(t *testing.T) { + // Jitter is uniform(0.8, 1.2)*current, so these bounds hold for every draw. + for i := 0; i < 64; i++ { + // 1ms * 1.2 = 1.2ms, always below the 50ms floor -> clamps up. + assert.Equal(t, callMinPollInterval, nextCallPollInterval(1*time.Millisecond), + "sub-floor interval must clamp up to callMinPollInterval") + + // 10s * 0.8 = 8s, always above the 4s ceiling -> clamps down. + assert.Equal(t, callMaxPollInterval, nextCallPollInterval(10*time.Second), + "super-ceiling interval must clamp down to callMaxPollInterval") + + // 1s jittered stays within [0.8s, 1.2s] -> no clamp, value returned as-is. + mid := nextCallPollInterval(1 * time.Second) + assert.GreaterOrEqual(t, mid, 800*time.Millisecond) + assert.LessOrEqual(t, mid, 1200*time.Millisecond) + } +} + +// --- submitAsyncExecution error branches ----------------------------------- + +// Contract: an unbuildable submit request surfaces a "build request" error +// before any network I/O. +func TestSubmitAsyncExecution_BuildRequestError(t *testing.T) { + a := newCallTestAgent(t, "http://placeholder.invalid") + + _, _, err := a.submitAsyncExecution( + context.Background(), "://bad", "target.node", + []byte(`{"input":{}}`), ExecutionContext{}, "run-1", + ) + require.Error(t, err) + assert.Contains(t, err.Error(), "build request") +} + +// Contract: an AMBIGUOUS submit transport failure (a plain error that does not +// prove the request never reached the server) is NOT retried — re-POSTing a +// possibly-accepted execute/async would double-run the target — and surfaces a +// "perform execute call (not retried ...)" error on the first attempt. +func TestSubmitAsyncExecution_TransportError(t *testing.T) { + a := newCallTestAgent(t, "http://placeholder.invalid") + var attempts atomic.Int32 + a.callSubmitClient = &http.Client{Transport: roundTripperFunc(func(*http.Request) (*http.Response, error) { + attempts.Add(1) + return nil, errors.New("read: connection reset by peer") + })} + + _, _, err := a.submitAsyncExecution( + context.Background(), "http://cp.example", "target.node", + []byte(`{"input":{}}`), ExecutionContext{}, "run-1", + ) + require.Error(t, err) + assert.Contains(t, err.Error(), "perform execute call") + assert.Contains(t, err.Error(), "not retried") + assert.Equal(t, int32(1), attempts.Load(), "ambiguous submit failure must not be retried") +} + +// Contract: a submit response whose body cannot be read surfaces a +// "read execute response" error. +func TestSubmitAsyncExecution_ReadBodyError(t *testing.T) { + a := newCallTestAgent(t, "http://placeholder.invalid") + a.callSubmitClient = &http.Client{Transport: roundTripperFunc(func(*http.Request) (*http.Response, error) { + return &http.Response{ + StatusCode: http.StatusAccepted, + Header: make(http.Header), + Body: callErrBody{}, + }, nil + })} + + _, _, err := a.submitAsyncExecution( + context.Background(), "http://cp.example", "target.node", + []byte(`{"input":{}}`), ExecutionContext{}, "run-1", + ) + require.Error(t, err) + assert.Contains(t, err.Error(), "read execute response") +} + +// Contract: when the control plane rejects the submit with a structured JSON +// error body, Call returns an *ExecuteError carrying the HTTP status and the +// control plane's "error" message (and error_details). +func TestCall_SubmitStructuredError(t *testing.T) { + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + assert.Equal(t, http.MethodPost, r.Method) + w.WriteHeader(http.StatusForbidden) + _ = json.NewEncoder(w).Encode(map[string]any{ + "error": "permission denied", + "error_details": map[string]any{"code": "forbidden"}, + }) + })) + defer server.Close() + + a := newCallTestAgent(t, server.URL) + + result, err := a.Call(context.Background(), "target.node", map[string]any{}) + require.Error(t, err) + assert.Nil(t, result) + + var execErr *ExecuteError + require.ErrorAs(t, err, &execErr) + assert.Equal(t, http.StatusForbidden, execErr.StatusCode) + assert.Contains(t, execErr.Message, "permission denied") + assert.NotNil(t, execErr.ErrorDetails) +} + +// Contract: a 2xx submit whose body is not valid JSON surfaces a +// "decode execute response" error. +func TestCall_SubmitDecodeError(t *testing.T) { + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + w.WriteHeader(http.StatusAccepted) + _, _ = w.Write([]byte("this is definitely not json")) + })) + defer server.Close() + + a := newCallTestAgent(t, server.URL) + + _, err := a.Call(context.Background(), "target.node", map[string]any{}) + require.Error(t, err) + assert.Contains(t, err.Error(), "decode execute response") +} + +// --- awaitExecutionResult error branches ----------------------------------- + +// Contract: a context already cancelled before the wait loop starts aborts +// immediately with the context error, without polling. +func TestAwaitExecutionResult_ContextAlreadyCancelled(t *testing.T) { + a := newCallTestAgent(t, "http://placeholder.invalid") + + ctx, cancel := context.WithCancel(context.Background()) + cancel() + + result, err := a.awaitExecutionResult( + ctx, "http://cp.example", "target.node", "exec-1", "run-1", ExecutionContext{}, + ) + require.Error(t, err) + assert.Nil(t, result) + assert.ErrorIs(t, err, context.Canceled) +} + +// Contract: an unbuildable status-poll request surfaces a "build status +// request" error. +func TestAwaitExecutionResult_BuildRequestError(t *testing.T) { + a := newCallTestAgent(t, "http://placeholder.invalid") + + result, err := a.awaitExecutionResult( + context.Background(), "://bad", "target.node", "exec-1", "run-1", ExecutionContext{}, + ) + require.Error(t, err) + assert.Nil(t, result) + assert.Contains(t, err.Error(), "build status request") +} + +// Contract: when the caller's context is cancelled while a poll request is in +// flight, the wait surfaces the context error (not a generic transport error), +// preserving the ctx-cancel contract. +func TestAwaitExecutionResult_ContextCancelDuringPoll(t *testing.T) { + a := newCallTestAgent(t, "http://placeholder.invalid") + a.callPollClient = &http.Client{Transport: roundTripperFunc(func(req *http.Request) (*http.Response, error) { + <-req.Context().Done() + return nil, req.Context().Err() + })} + + ctx, cancel := context.WithTimeout(context.Background(), 50*time.Millisecond) + defer cancel() + + result, err := a.awaitExecutionResult( + ctx, "http://cp.example", "target.node", "exec-1", "run-1", ExecutionContext{}, + ) + require.Error(t, err) + assert.Nil(t, result) + assert.ErrorIs(t, err, context.DeadlineExceeded) +} + +// Contract: a PERSISTENT poll transport failure is retried (not fatal on the +// first blip) until the retry window is exceeded, then fails with the +// "control plane unreachable for Xs" error that wraps the underlying cause — +// not the raw first error. The window is shrunk via env so the test is fast. +func TestAwaitExecutionResult_TransportError(t *testing.T) { + t.Setenv(envCallRetryWindowSeconds, "1") + a := newCallTestAgent(t, "http://placeholder.invalid") + var attempts atomic.Int32 + a.callPollClient = &http.Client{Transport: roundTripperFunc(func(*http.Request) (*http.Response, error) { + attempts.Add(1) + return nil, errors.New("connection reset by peer") + })} + + start := time.Now() + result, err := a.awaitExecutionResult( + context.Background(), "http://cp.example", "target.node", "exec-1", "run-1", ExecutionContext{}, + ) + require.Error(t, err) + assert.Nil(t, result) + assert.Contains(t, err.Error(), "control plane unreachable") + assert.Contains(t, err.Error(), "poll execution status") + assert.Greater(t, attempts.Load(), int32(1), "a transient poll failure must be retried, not fatal on the first blip") + assert.Less(t, time.Since(start), 20*time.Second, "must fail promptly once the window is exceeded") +} + +// Contract: a status-poll response whose body cannot be read surfaces a +// "read execution status" error. +func TestAwaitExecutionResult_ReadBodyError(t *testing.T) { + a := newCallTestAgent(t, "http://placeholder.invalid") + a.callPollClient = &http.Client{Transport: roundTripperFunc(func(*http.Request) (*http.Response, error) { + return &http.Response{ + StatusCode: http.StatusOK, + Header: make(http.Header), + Body: callErrBody{}, + }, nil + })} + + result, err := a.awaitExecutionResult( + context.Background(), "http://cp.example", "target.node", "exec-1", "run-1", ExecutionContext{}, + ) + require.Error(t, err) + assert.Nil(t, result) + assert.Contains(t, err.Error(), "read execution status") +} + +// Contract: a PERMANENT (non-transient) HTTP error status on a status poll — +// e.g. 403 auth — aborts the wait immediately with an *ExecuteError carrying +// that status. (Transient statuses like 5xx/429 are instead retried; see +// TestCall_PollRetriesTransient5xxThenSucceeds.) +func TestCall_PollReturnsErrorStatus(t *testing.T) { + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + switch r.Method { + case http.MethodPost: + w.WriteHeader(http.StatusAccepted) + _ = json.NewEncoder(w).Encode(map[string]any{ + "execution_id": "exec-1", + "run_id": "run-1", + "status": "queued", + }) + default: + w.WriteHeader(http.StatusForbidden) + _, _ = w.Write([]byte("permission denied")) + } + })) + defer server.Close() + + a := newCallTestAgent(t, server.URL) + + result, err := a.Call(context.Background(), "target.node", map[string]any{}) + require.Error(t, err) + assert.Nil(t, result) + + var execErr *ExecuteError + require.ErrorAs(t, err, &execErr) + assert.Equal(t, http.StatusForbidden, execErr.StatusCode) + assert.Contains(t, execErr.Message, "execution status failed") +} + +// Contract: a status-poll body that is not valid JSON surfaces a +// "decode execute response" error. +func TestCall_PollDecodeError(t *testing.T) { + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + switch r.Method { + case http.MethodPost: + w.WriteHeader(http.StatusAccepted) + _ = json.NewEncoder(w).Encode(map[string]any{ + "execution_id": "exec-1", + "run_id": "run-1", + "status": "queued", + }) + default: + _, _ = w.Write([]byte("<>")) + } + })) + defer server.Close() + + a := newCallTestAgent(t, server.URL) + + _, err := a.Call(context.Background(), "target.node", map[string]any{}) + require.Error(t, err) + assert.Contains(t, err.Error(), "decode execute response") +} + +// Contract: a succeeded execution whose "result" field is not a JSON object +// (so it cannot decode into the result map) surfaces a "decode execute +// response" error rather than a silent empty result. +func TestCall_SucceededResultDecodeError(t *testing.T) { + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + switch r.Method { + case http.MethodPost: + w.WriteHeader(http.StatusAccepted) + _ = json.NewEncoder(w).Encode(map[string]any{ + "execution_id": "exec-1", + "run_id": "run-1", + "status": "queued", + }) + default: + // result is a JSON string, not an object -> unmarshal into + // map[string]any fails. + _, _ = w.Write([]byte(`{"status":"succeeded","result":"not-an-object"}`)) + } + })) + defer server.Close() + + a := newCallTestAgent(t, server.URL) + + _, err := a.Call(context.Background(), "target.node", map[string]any{}) + require.Error(t, err) + assert.Contains(t, err.Error(), "decode execute response") +} + +// Contract: a succeeded execution with a null result yields a nil result map +// and no error (the len/"null" guard skips decoding). +func TestCall_SucceededNullResult(t *testing.T) { + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + switch r.Method { + case http.MethodPost: + w.WriteHeader(http.StatusAccepted) + _ = json.NewEncoder(w).Encode(map[string]any{ + "execution_id": "exec-1", + "run_id": "run-1", + "status": "queued", + }) + default: + _, _ = w.Write([]byte(`{"status":"succeeded","result":null}`)) + } + })) + defer server.Close() + + a := newCallTestAgent(t, server.URL) + + result, err := a.Call(context.Background(), "target.node", map[string]any{}) + require.NoError(t, err) + assert.Nil(t, result) +} + +// --- Call resilience to transient control-plane outages -------------------- +// +// These tests exercise the Part 4 behavior contract: a transient poll blip or a +// dial-only submit failure must NOT kill a long-running cross-node call, while a +// non-idempotent submit is never blindly re-sent on an ambiguous failure. + +// Contract: the fake CP serves 2 successful polls, then 3 consecutive transient +// 5xx polls (well within the retry window), then recovers and completes → the +// call SUCCEEDS with the correct result, and each transient failure is logged at +// warn with the call.outbound.poll_retry event. +func TestCall_PollRetriesTransient5xxThenSucceeds(t *testing.T) { + var polls atomic.Int32 + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + switch { + case r.Method == http.MethodPost && strings.HasPrefix(r.URL.Path, "/api/v1/execute/async/"): + w.WriteHeader(http.StatusAccepted) + _ = json.NewEncoder(w).Encode(map[string]any{ + "execution_id": "exec-1", "run_id": "run-1", "status": "queued", + }) + case r.Method == http.MethodGet: + switch n := polls.Add(1); { + case n <= 2: // two healthy "running" polls + _ = json.NewEncoder(w).Encode(map[string]any{"status": "running"}) + case n <= 5: // three consecutive transient failures + w.WriteHeader(http.StatusServiceUnavailable) + _, _ = w.Write([]byte("overloaded")) + default: // recovery + _ = json.NewEncoder(w).Encode(map[string]any{ + "status": "succeeded", + "result": map[string]any{"ok": true}, + }) + } + default: + w.WriteHeader(http.StatusNotFound) + } + })) + defer server.Close() + + a := newCallTestAgent(t, server.URL) + + var result map[string]any + stdout, _, err := captureOutput(t, func() error { + var callErr error + result, callErr = a.Call(context.Background(), "target.node", map[string]any{}) + return callErr + }) + require.NoError(t, err, "transient poll failures within the window must not fail the call") + require.NotNil(t, result) + assert.Equal(t, true, result["ok"]) + assert.GreaterOrEqual(t, polls.Load(), int32(6), "must have retried past the transient failures") + + var warnRetries int + for _, line := range strings.Split(strings.TrimSpace(stdout), "\n") { + if line == "" { + continue + } + var entry ExecutionLogEntry + if json.Unmarshal([]byte(line), &entry) != nil { + continue + } + if entry.EventType == "call.outbound.poll_retry" && entry.Level == "warn" { + warnRetries++ + assert.NotNil(t, entry.Attributes["attempt"], "warn retry log must carry the attempt count") + } + } + assert.GreaterOrEqual(t, warnRetries, 3, "each transient failure must emit a warn retry log") +} + +// Contract: when the CP stays down past the (env-shortened) retry window, the +// call fails with the "control plane unreachable for Xs" error — not the raw +// first poll error — after having retried (5xx is transient). +func TestCall_PollUnreachableAfterWindow(t *testing.T) { + t.Setenv(envCallRetryWindowSeconds, "1") + var polls atomic.Int32 + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + switch r.Method { + case http.MethodPost: + w.WriteHeader(http.StatusAccepted) + _ = json.NewEncoder(w).Encode(map[string]any{ + "execution_id": "exec-1", "run_id": "run-1", "status": "queued", + }) + default: + polls.Add(1) + w.WriteHeader(http.StatusBadGateway) + _, _ = w.Write([]byte("down")) + } + })) + defer server.Close() + + a := newCallTestAgent(t, server.URL) + start := time.Now() + result, err := a.Call(context.Background(), "target.node", map[string]any{}) + require.Error(t, err) + assert.Nil(t, result) + assert.Contains(t, err.Error(), "control plane unreachable") + assert.Greater(t, polls.Load(), int32(1), "a transient 5xx must be retried, not fatal on the first") + assert.Less(t, time.Since(start), 20*time.Second, "must fail promptly once the window is exceeded") +} + +// Contract: a 404 on a just-submitted execution is retried within a bounded +// (shorter) window, then fails with a DISTINCT not-found error (StatusCode 404) +// rather than the generic unreachable error. +func TestCall_Poll404BoundedThenDistinctError(t *testing.T) { + t.Setenv(envCallRetryWindowSeconds, "1") // also shrinks the 404 window to 1s + var gets atomic.Int32 + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + switch r.Method { + case http.MethodPost: + w.WriteHeader(http.StatusAccepted) + _ = json.NewEncoder(w).Encode(map[string]any{ + "execution_id": "exec-404", "run_id": "run-1", "status": "queued", + }) + default: + gets.Add(1) + w.WriteHeader(http.StatusNotFound) + _, _ = w.Write([]byte("not found")) + } + })) + defer server.Close() + + a := newCallTestAgent(t, server.URL) + result, err := a.Call(context.Background(), "target.node", map[string]any{}) + require.Error(t, err) + assert.Nil(t, result) + + var execErr *ExecuteError + require.ErrorAs(t, err, &execErr) + assert.Equal(t, http.StatusNotFound, execErr.StatusCode) + assert.Contains(t, execErr.Message, "not found on control plane") + assert.Greater(t, gets.Load(), int32(1), "404 right after submit must be retried within the bounded window") +} + +// Contract: a submit whose connection is REFUSED twice (proving the request +// never reached the server) is safely retried and then succeeds — creating +// EXACTLY ONE execution on the CP (a refused dial writes no request bytes, so a +// re-POST cannot double-run the target). +func TestSubmit_ConnectionRefusedRetriesToExactlyOneExecution(t *testing.T) { + var executePosts atomic.Int32 + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + switch { + case r.Method == http.MethodPost && strings.HasPrefix(r.URL.Path, "/api/v1/execute/async/"): + executePosts.Add(1) + w.WriteHeader(http.StatusAccepted) + _ = json.NewEncoder(w).Encode(map[string]any{ + "execution_id": "exec-1", "run_id": "run-1", "status": "queued", + }) + case r.Method == http.MethodGet: + _ = json.NewEncoder(w).Encode(map[string]any{ + "status": "succeeded", "result": map[string]any{"ok": true}, + }) + default: + w.WriteHeader(http.StatusNotFound) + } + })) + defer server.Close() + + a := newCallTestAgent(t, server.URL) + var dials atomic.Int32 + realTransport := http.DefaultTransport + a.callSubmitClient = &http.Client{Transport: roundTripperFunc(func(req *http.Request) (*http.Response, error) { + if dials.Add(1) <= 2 { + return nil, &net.OpError{Op: "dial", Net: "tcp", Err: syscall.ECONNREFUSED} + } + return realTransport.RoundTrip(req) + })} + + result, err := a.Call(context.Background(), "target.node", map[string]any{}) + require.NoError(t, err) + require.NotNil(t, result) + assert.Equal(t, true, result["ok"]) + assert.Equal(t, int32(1), executePosts.Load(), "connection-refused retries must create exactly one execution") + assert.Equal(t, int32(3), dials.Load(), "two refusals then one accepted submit") +} + +// Contract: an AMBIGUOUS submit failure — the server accepts the connection but +// never responds (the request MAY have been accepted) — is NOT retried. The +// generous submit timeout (shrunk here via env) bounds the wait; exactly ONE +// request reaches the server and the call fails with a clear message. +func TestSubmit_AmbiguousTimeoutNoRetry(t *testing.T) { + t.Setenv(envCallSubmitTimeoutSeconds, "1") + var executeRequests atomic.Int32 + // release lets the blocked handler return so server.Close() does not hang. + // The two defers run LIFO: close(release) first, then server.Close(). + release := make(chan struct{}) + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + if r.Method == http.MethodPost { + executeRequests.Add(1) + // Accept the connection but never respond until the test ends or the + // client disconnects — modelling a hung-but-listening CP. + select { + case <-release: + case <-r.Context().Done(): + } + return + } + w.WriteHeader(http.StatusNotFound) + })) + defer server.Close() + defer close(release) + + a := newCallTestAgent(t, server.URL) + start := time.Now() + result, err := a.Call(context.Background(), "target.node", map[string]any{}) + require.Error(t, err) + assert.Nil(t, result) + assert.Contains(t, err.Error(), "not retried") + assert.Equal(t, int32(1), executeRequests.Load(), "ambiguous submit timeout must not be retried") + assert.GreaterOrEqual(t, time.Since(start), 900*time.Millisecond, "submit must wait for its timeout") + assert.Less(t, time.Since(start), 10*time.Second) +} + +// Contract: the resilience knobs are read from the environment (once, cached on +// the agent), and invalid/unparseable values fall back to the documented +// defaults. +func TestCallResilienceEnvOverrides(t *testing.T) { + t.Setenv(envCallRetryWindowSeconds, "7") + t.Setenv(envCallSubmitTimeoutSeconds, "11") + t.Setenv(envCallPollTimeoutSeconds, "13") + ov := newCallTestAgent(t, "http://placeholder.invalid") + assert.Equal(t, 7*time.Second, ov.callRetryWindow) + assert.Equal(t, 11*time.Second, ov.callSubmitClient.Timeout) + assert.Equal(t, 13*time.Second, ov.callPollClient.Timeout) + + // Invalid / non-positive values fall back to defaults. + t.Setenv(envCallRetryWindowSeconds, "not-a-number") + t.Setenv(envCallSubmitTimeoutSeconds, "-5") + t.Setenv(envCallPollTimeoutSeconds, "0") + fb := newCallTestAgent(t, "http://placeholder.invalid") + assert.Equal(t, defaultCallRetryWindow, fb.callRetryWindow) + assert.Equal(t, defaultCallSubmitTimeout, fb.callSubmitClient.Timeout) + assert.Equal(t, defaultCallPollTimeout, fb.callPollClient.Timeout) +} + +// Contract: requestNeverReachedServer returns true ONLY for errors that prove no +// request bytes reached the server (connection-refused, DNS failure, dial-phase +// net.OpError), so a non-idempotent submit is retried only when safe. Caller +// cancellation, an ambiguous post-dial timeout, and non-dial network errors all +// return false. +func TestRequestNeverReachedServer(t *testing.T) { + tests := []struct { + name string + err error + want bool + }{ + {"nil", nil, false}, + {"caller cancelled", context.Canceled, false}, + {"caller deadline (ambiguous timeout)", context.DeadlineExceeded, false}, + {"wrapped deadline", &url.Error{Op: "Post", URL: "http://x", Err: context.DeadlineExceeded}, false}, + {"connection refused", syscall.ECONNREFUSED, true}, + {"wrapped connection refused", &url.Error{Op: "Post", URL: "http://x", Err: &net.OpError{Op: "dial", Net: "tcp", Err: syscall.ECONNREFUSED}}, true}, + {"dns failure", &net.DNSError{Err: "no such host", Name: "cp.invalid"}, true}, + {"wrapped dns failure", &url.Error{Op: "Post", URL: "http://x", Err: &net.DNSError{Err: "no such host"}}, true}, + {"dial-phase op error (non-econnrefused)", &net.OpError{Op: "dial", Net: "tcp", Err: errors.New("network is unreachable")}, true}, + {"post-dial read error", &net.OpError{Op: "read", Net: "tcp", Err: errors.New("connection reset by peer")}, false}, + {"opaque error", errors.New("something went wrong"), false}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + assert.Equal(t, tt.want, requestNeverReachedServer(tt.err)) + }) + } +} + +// Contract: isTransientPollStatus treats 408/429/5xx as retryable and every +// other status (including 404 and permanent 4xx) as non-transient. +func TestIsTransientPollStatus(t *testing.T) { + transient := []int{http.StatusRequestTimeout, http.StatusTooManyRequests, 500, 502, 503, 504} + for _, code := range transient { + assert.True(t, isTransientPollStatus(code), "status %d should be transient", code) + } + permanent := []int{200, 201, 400, http.StatusForbidden, http.StatusUnauthorized, http.StatusNotFound, 409} + for _, code := range permanent { + assert.False(t, isTransientPollStatus(code), "status %d should not be transient", code) + } +} + +// Contract: nextRetryBackoff doubles the backoff, clamps to callRetryBackoffMax, +// and floors a sub-minimum input at callRetryBackoffMin. +func TestNextRetryBackoff(t *testing.T) { + assert.Equal(t, 1*time.Second, nextRetryBackoff(callRetryBackoffMin)) // 500ms -> 1s + assert.Equal(t, callRetryBackoffMax, nextRetryBackoff(callRetryBackoffMax)) + assert.Equal(t, callRetryBackoffMax, nextRetryBackoff(callRetryBackoffMax+time.Second)) + assert.Equal(t, callRetryBackoffMin, nextRetryBackoff(1*time.Nanosecond)) // 2ns clamps up to the floor +} + +// Contract: sleepCtx returns true when the full duration elapses (or is +// non-positive on a live ctx) and false when ctx is cancelled — before or +// during the wait. +func TestSleepCtx(t *testing.T) { + assert.True(t, sleepCtx(context.Background(), 0), "non-positive duration on a live ctx returns true") + assert.True(t, sleepCtx(context.Background(), 5*time.Millisecond), "elapsed sleep returns true") + + cancelled, cancel := context.WithCancel(context.Background()) + cancel() + assert.False(t, sleepCtx(cancelled, 0), "non-positive duration on a cancelled ctx returns false") + assert.False(t, sleepCtx(cancelled, time.Hour), "already-cancelled ctx returns false immediately") + + ctx, cancel2 := context.WithCancel(context.Background()) + go func() { time.Sleep(20 * time.Millisecond); cancel2() }() + start := time.Now() + assert.False(t, sleepCtx(ctx, time.Hour), "cancellation during the wait returns false") + assert.Less(t, time.Since(start), time.Second, "must return promptly on cancellation") +} + +// Contract: when a submit's connection is refused for longer than the retry +// window, submitAsyncExecution stops retrying and fails with the "control plane +// unreachable" error (having retried more than once), never creating an +// execution. +func TestSubmit_ConnectionRefusedBeyondWindow(t *testing.T) { + t.Setenv(envCallRetryWindowSeconds, "1") + a := newCallTestAgent(t, "http://placeholder.invalid") + var attempts atomic.Int32 + a.callSubmitClient = &http.Client{Transport: roundTripperFunc(func(*http.Request) (*http.Response, error) { + attempts.Add(1) + return nil, &net.OpError{Op: "dial", Net: "tcp", Err: syscall.ECONNREFUSED} + })} + + start := time.Now() + _, _, err := a.submitAsyncExecution( + context.Background(), "http://cp.example", "target.node", + []byte(`{"input":{}}`), ExecutionContext{}, "run-1", + ) + require.Error(t, err) + assert.Contains(t, err.Error(), "control plane unreachable") + assert.Greater(t, attempts.Load(), int32(1), "connection-refused submit must be retried within the window") + assert.Less(t, time.Since(start), 20*time.Second, "must fail promptly once the window is exceeded") +} diff --git a/sdk/go/agent/agent_test.go b/sdk/go/agent/agent_test.go index 160258040..776b39d1c 100644 --- a/sdk/go/agent/agent_test.go +++ b/sdk/go/agent/agent_test.go @@ -697,27 +697,61 @@ func TestHandleReasoner_Error(t *testing.T) { assert.Contains(t, result["error"], "assert.AnError") } +// TestCall pins the async-submit + wait contract. Contract: Call POSTs to the +// ASYNC execute endpoint (/api/v1/execute/async/{target}) with the full set of +// lineage headers (the control plane reads them identically for sync and async +// via readExecutionHeaders, so DAG parentage is preserved), then polls +// GET /api/v1/executions/{id} — with the same lineage headers — until the +// execution succeeds, and returns the child's result map. func TestCall(t *testing.T) { + var polls atomic.Int32 server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { - if strings.Contains(r.URL.Path, "/execute/") { - // Verify headers + switch { + case r.Method == http.MethodPost && r.URL.Path == "/api/v1/execute/async/target.node": + // Verify lineage headers on the submit assert.Equal(t, "run-1", r.Header.Get("X-Run-ID")) assert.Equal(t, "parent-exec", r.Header.Get("X-Parent-Execution-ID")) assert.Equal(t, "session-1", r.Header.Get("X-Session-ID")) assert.Equal(t, "actor-1", r.Header.Get("X-Actor-ID")) + assert.Equal(t, "node-1", r.Header.Get("X-Caller-Agent-ID")) var reqBody map[string]any json.NewDecoder(r.Body).Decode(&reqBody) assert.Equal(t, map[string]any{"value": float64(42)}, reqBody["input"]) - resp := map[string]any{ + w.WriteHeader(http.StatusAccepted) + _ = json.NewEncoder(w).Encode(map[string]any{ + "execution_id": "exec-1", + "run_id": "run-1", + "status": "queued", + }) + case r.Method == http.MethodGet && r.URL.Path == "/api/v1/executions/exec-1": + // Lineage headers ride along on polls too (Python parity: the + // wait loop reuses the submit headers). + assert.Equal(t, "run-1", r.Header.Get("X-Run-ID")) + assert.Equal(t, "parent-exec", r.Header.Get("X-Parent-Execution-ID")) + + // First poll: still running; second poll: terminal result. + if polls.Add(1) == 1 { + _ = json.NewEncoder(w).Encode(map[string]any{ + "execution_id": "exec-1", + "run_id": "run-1", + "status": "running", + }) + return + } + _ = json.NewEncoder(w).Encode(map[string]any{ "execution_id": "exec-1", "run_id": "run-1", "status": "succeeded", "result": map[string]any{"output": "result"}, - } + }) + case strings.HasSuffix(r.URL.Path, "/logs"): + // Structured execution-log shipping — irrelevant to this contract. w.WriteHeader(http.StatusOK) - json.NewEncoder(w).Encode(resp) + default: + t.Errorf("unexpected request: %s %s", r.Method, r.URL.Path) + w.WriteHeader(http.StatusNotFound) } })) defer server.Close() @@ -744,33 +778,182 @@ func TestCall(t *testing.T) { assert.NoError(t, err) assert.NotNil(t, result) assert.Equal(t, "result", result["output"]) + assert.GreaterOrEqual(t, polls.Load(), int32(2), "expected at least two status polls") +} + +// TestCall_OutlivesCallTimeout is the regression test for the 15s +// 'context deadline exceeded' bug: a child reasoner that runs LONGER than +// Config.CallTimeout must still complete successfully, because CallTimeout +// bounds each HTTP request (submit + each poll), not the end-to-end call. +func TestCall_OutlivesCallTimeout(t *testing.T) { + const callTimeout = 300 * time.Millisecond + start := time.Now() + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + switch { + case r.Method == http.MethodPost && strings.HasPrefix(r.URL.Path, "/api/v1/execute/async/"): + w.WriteHeader(http.StatusAccepted) + _ = json.NewEncoder(w).Encode(map[string]any{ + "execution_id": "exec-slow", + "run_id": "run-slow", + "status": "queued", + }) + case r.Method == http.MethodGet && r.URL.Path == "/api/v1/executions/exec-slow": + // Child stays 'running' for 3x CallTimeout, then succeeds. Every + // individual poll response is fast — only the CHILD is slow. + if time.Since(start) < 3*callTimeout { + _ = json.NewEncoder(w).Encode(map[string]any{"status": "running"}) + return + } + _ = json.NewEncoder(w).Encode(map[string]any{ + "status": "succeeded", + "result": map[string]any{"took": "longer than CallTimeout"}, + }) + default: + t.Errorf("unexpected request: %s %s", r.Method, r.URL.Path) + w.WriteHeader(http.StatusNotFound) + } + })) + defer server.Close() + + agent, err := New(Config{ + NodeID: "node-1", + Version: "1.0.0", + AgentFieldURL: server.URL, + CallTimeout: callTimeout, + Logger: log.New(io.Discard, "", 0), + }) + require.NoError(t, err) + + result, err := agent.Call(context.Background(), "target.slow", map[string]any{}) + require.NoError(t, err, "a child running longer than CallTimeout must not kill the call") + require.NotNil(t, result) + assert.Equal(t, "longer than CallTimeout", result["took"]) + assert.GreaterOrEqual(t, time.Since(start), 3*callTimeout, + "call must actually have waited past CallTimeout") +} + +// TestCall_CtxCancelAbortsWait: cancelling the caller's ctx aborts the wait +// loop promptly (returning the context error). Note the contract: the child +// execution is NOT cancelled server-side — matching the Python SDK. +func TestCall_CtxCancelAbortsWait(t *testing.T) { + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + switch { + case r.Method == http.MethodPost && strings.HasPrefix(r.URL.Path, "/api/v1/execute/async/"): + w.WriteHeader(http.StatusAccepted) + _ = json.NewEncoder(w).Encode(map[string]any{ + "execution_id": "exec-hang", + "run_id": "run-hang", + "status": "queued", + }) + case r.Method == http.MethodGet: + // Child never finishes. + _ = json.NewEncoder(w).Encode(map[string]any{"status": "running"}) + default: + w.WriteHeader(http.StatusNotFound) + } + })) + defer server.Close() + + agent, err := New(Config{ + NodeID: "node-1", + Version: "1.0.0", + AgentFieldURL: server.URL, + Logger: log.New(io.Discard, "", 0), + }) + require.NoError(t, err) + + ctx, cancel := context.WithCancel(context.Background()) + go func() { + time.Sleep(250 * time.Millisecond) + cancel() + }() + + start := time.Now() + result, err := agent.Call(ctx, "target.hang", map[string]any{}) + require.Error(t, err) + assert.ErrorIs(t, err, context.Canceled) + assert.Nil(t, result) + assert.Less(t, time.Since(start), 5*time.Second, "cancel must abort the wait promptly") } func TestCall_ErrorHandling(t *testing.T) { tests := []struct { name string serverResponse func(w http.ResponseWriter, r *http.Request) - wantErr bool + wantErrSubstr string }{ { - name: "API error", + name: "API error on submit", serverResponse: func(w http.ResponseWriter, r *http.Request) { w.WriteHeader(http.StatusBadRequest) w.Write([]byte("bad request")) }, - wantErr: true, + wantErrSubstr: "bad request", + }, + { + name: "submission missing execution id", + serverResponse: func(w http.ResponseWriter, r *http.Request) { + w.WriteHeader(http.StatusAccepted) + _ = json.NewEncoder(w).Encode(map[string]any{"status": "queued"}) + }, + wantErrSubstr: "missing identifiers", }, { - name: "execution failed status", + name: "execution failed status via poll", serverResponse: func(w http.ResponseWriter, r *http.Request) { - resp := map[string]any{ + if r.Method == http.MethodPost { + w.WriteHeader(http.StatusAccepted) + _ = json.NewEncoder(w).Encode(map[string]any{ + "execution_id": "exec-f", + "run_id": "run-f", + "status": "queued", + }) + return + } + _ = json.NewEncoder(w).Encode(map[string]any{ "status": "failed", "error_message": "execution failed", + }) + }, + wantErrSubstr: "execution failed", + }, + { + name: "failed execution coalesces error field into message", + serverResponse: func(w http.ResponseWriter, r *http.Request) { + if r.Method == http.MethodPost { + w.WriteHeader(http.StatusAccepted) + _ = json.NewEncoder(w).Encode(map[string]any{ + "execution_id": "exec-e", + "run_id": "run-e", + "status": "queued", + }) + return } - w.WriteHeader(http.StatusOK) - json.NewEncoder(w).Encode(resp) + // The status endpoint reports failures in "error" + // (ExecutionStatusResponse), not "error_message" — Python + // coalesces it (client.py:1000-1002) and so must we. + _ = json.NewEncoder(w).Encode(map[string]any{ + "status": "failed", + "error": "boom from error field", + }) }, - wantErr: true, + wantErrSubstr: "boom from error field", + }, + { + name: "cancelled execution is terminal", + serverResponse: func(w http.ResponseWriter, r *http.Request) { + if r.Method == http.MethodPost { + w.WriteHeader(http.StatusAccepted) + _ = json.NewEncoder(w).Encode(map[string]any{ + "execution_id": "exec-c", + "run_id": "run-c", + "status": "queued", + }) + return + } + _ = json.NewEncoder(w).Encode(map[string]any{"status": "cancelled"}) + }, + wantErrSubstr: "execute status cancelled", }, } @@ -790,13 +973,9 @@ func TestCall_ErrorHandling(t *testing.T) { require.NoError(t, err) result, err := agent.Call(context.Background(), "target", map[string]any{}) - if tt.wantErr { - assert.Error(t, err) - assert.Nil(t, result) - } else { - assert.NoError(t, err) - assert.NotNil(t, result) - } + require.Error(t, err) + assert.Nil(t, result) + assert.Contains(t, err.Error(), tt.wantErrSubstr) }) } } @@ -994,8 +1173,11 @@ func TestAIWithTools(t *testing.T) { switch r.URL.Path { case "/api/v1/discovery/capabilities": _, _ = w.Write([]byte(`{"discovered_at":"2025-01-01T00:00:00Z","total_agents":1,"total_reasoners":1,"total_skills":0,"pagination":{"limit":50,"offset":0,"has_more":false},"capabilities":[{"agent_id":"agent-1","reasoners":[{"id":"lookup","invocation_target":"agent-1.lookup","input_schema":{"type":"object"}}],"skills":[]}]}`)) - case "/api/v1/execute/agent-1.lookup": - _, _ = w.Write([]byte(`{"status":"open"}`)) + case "/api/v1/execute/async/agent-1.lookup": + w.WriteHeader(http.StatusAccepted) + _, _ = w.Write([]byte(`{"execution_id":"exec-tool-1","run_id":"run-tool-1","status":"queued"}`)) + case "/api/v1/executions/exec-tool-1": + _, _ = w.Write([]byte(`{"execution_id":"exec-tool-1","run_id":"run-tool-1","status":"succeeded","result":{"status":"open"}}`)) case "/chat/completions": count := chatRequests.Add(1) if count == 1 { @@ -1037,9 +1219,10 @@ func TestAIWithTools(t *testing.T) { return } _ = json.NewEncoder(w).Encode(ai.Response{Choices: []ai.Choice{{Message: ai.Message{Content: []ai.ContentPart{{Type: "text", Text: "blocked"}}}}}}) - case "/api/v1/execute/agent-1.lookup": + case "/api/v1/execute/async/agent-1.lookup": executeCalls.Add(1) - _, _ = w.Write([]byte(`{"status":"open"}`)) + w.WriteHeader(http.StatusAccepted) + _, _ = w.Write([]byte(`{"execution_id":"exec-tool-2","run_id":"run-tool-2","status":"queued"}`)) default: t.Fatalf("unexpected path %s", r.URL.Path) } @@ -1683,10 +1866,19 @@ func TestCallLocalUnknownReasoner(t *testing.T) { } func TestCall_TargetPrefixing(t *testing.T) { - var capturedPath string + var capturedSubmitPath string server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { - capturedPath = r.URL.Path + if r.Method == http.MethodPost { + capturedSubmitPath = r.URL.Path + w.WriteHeader(http.StatusAccepted) + _ = json.NewEncoder(w).Encode(map[string]any{ + "execution_id": "exec-p", + "run_id": "run-p", + "status": "queued", + }) + return + } resp := map[string]any{ "status": "succeeded", @@ -1706,5 +1898,5 @@ func TestCall_TargetPrefixing(t *testing.T) { _, err := agent.Call(context.Background(), "lookup", nil) require.NoError(t, err) - assert.Contains(t, capturedPath, "/execute/node-1.lookup") + assert.Contains(t, capturedSubmitPath, "/execute/async/node-1.lookup") } diff --git a/sdk/go/agent/note.go b/sdk/go/agent/note.go index 2fa8801fd..8a41c8805 100644 --- a/sdk/go/agent/note.go +++ b/sdk/go/agent/note.go @@ -61,8 +61,11 @@ func (a *Agent) sendNote(ctx context.Context, message string, tags []string) { // Get execution context from the provided context execCtx := ExecutionContextFrom(ctx) - // Build note URL (canonical endpoint: /api/v1/executions/note) - noteURL := strings.TrimSuffix(baseURL, "/") + "/executions/note" + // Build note URL. AgentFieldURL is the bare control-plane base (e.g. + // http://localhost:8080); the canonical endpoint is /api/v1/executions/note, + // consistent with the other endpoint builders in agent.go and the client + // package. Omitting /api/v1 posts to an unregistered route and 404s. + noteURL := strings.TrimSuffix(baseURL, "/") + "/api/v1/executions/note" // Build payload payload := notePayload{ diff --git a/sdk/go/agent/note_test.go b/sdk/go/agent/note_test.go index c830bf0cc..059684d19 100644 --- a/sdk/go/agent/note_test.go +++ b/sdk/go/agent/note_test.go @@ -39,7 +39,7 @@ func TestNote_Basic(t *testing.T) { cfg := Config{ NodeID: "test-node", Version: "1.0.0", - AgentFieldURL: server.URL + "/api/v1", // Will be used directly + AgentFieldURL: server.URL, // bare control-plane base; /api/v1 appended by note.go Logger: log.New(io.Discard, "", 0), } @@ -97,7 +97,7 @@ func TestNotef_Formatted(t *testing.T) { cfg := Config{ NodeID: "test-node", Version: "1.0.0", - AgentFieldURL: server.URL + "/api/v1", + AgentFieldURL: server.URL, Logger: log.New(io.Discard, "", 0), } @@ -134,7 +134,7 @@ func TestNote_NoTags(t *testing.T) { cfg := Config{ NodeID: "test-node", Version: "1.0.0", - AgentFieldURL: server.URL + "/api/v1", + AgentFieldURL: server.URL, Logger: log.New(io.Discard, "", 0), } @@ -191,7 +191,7 @@ func TestNote_ServerError(t *testing.T) { cfg := Config{ NodeID: "test-node", Version: "1.0.0", - AgentFieldURL: server.URL + "/api/v1", + AgentFieldURL: server.URL, Logger: log.New(io.Discard, "", 0), } @@ -209,7 +209,14 @@ func TestNote_ServerError(t *testing.T) { time.Sleep(200 * time.Millisecond) } -func TestNote_URLConversion(t *testing.T) { +// TestNote_URLPath pins the canonical note endpoint. Contract: given the bare +// control-plane base URL as AgentFieldURL (the same value handed to +// client.New, which every other endpoint appends /api/v1/... to), a note POSTs +// to /api/v1/executions/note — the route the control plane actually registers +// (control-plane/internal/server/routes_core.go). A trailing slash on the base +// must not change the path. The previous test asserted the pre-fix path +// (/executions/note), which 404s against a real control plane. +func TestNote_URLPath(t *testing.T) { var requestPath string requestReceived := make(chan struct{}) @@ -221,19 +228,19 @@ func TestNote_URLConversion(t *testing.T) { defer server.Close() tests := []struct { - name string - agentFieldURL string - expectedPath string + name string + agentFieldURL string + expectedPath string }{ { - name: "Standard /api/v1 URL", - agentFieldURL: server.URL + "/api/v1", + name: "bare base URL", + agentFieldURL: server.URL, expectedPath: "/api/v1/executions/note", }, { - name: "URL without /api/v1", - agentFieldURL: server.URL, - expectedPath: "/executions/note", + name: "bare base URL with trailing slash", + agentFieldURL: server.URL + "/", + expectedPath: "/api/v1/executions/note", }, } @@ -282,7 +289,7 @@ func TestNote_WithToken(t *testing.T) { cfg := Config{ NodeID: "test-node", Version: "1.0.0", - AgentFieldURL: server.URL + "/api/v1", + AgentFieldURL: server.URL, Token: "test-token-123", Logger: log.New(io.Discard, "", 0), } @@ -316,7 +323,7 @@ func TestNote_FireAndForget(t *testing.T) { cfg := Config{ NodeID: "test-node", Version: "1.0.0", - AgentFieldURL: slowServer.URL + "/api/v1", + AgentFieldURL: slowServer.URL, Logger: log.New(io.Discard, "", 0), } @@ -350,7 +357,7 @@ func TestNote_MultipleNotes(t *testing.T) { cfg := Config{ NodeID: "test-node", Version: "1.0.0", - AgentFieldURL: server.URL + "/api/v1", + AgentFieldURL: server.URL, Logger: log.New(io.Discard, "", 0), } diff --git a/sdk/go/agent/reasoner_failed.go b/sdk/go/agent/reasoner_failed.go new file mode 100644 index 000000000..50bc30dcb --- /dev/null +++ b/sdk/go/agent/reasoner_failed.go @@ -0,0 +1,38 @@ +package agent + +// ReasonerFailed is returned from a reasoner handler to report that the work +// ran but failed, while preserving a structured result on the execution +// record. +// +// Returning a value from a reasoner — even one whose payload says the work did +// not succeed — makes the execution handler record the execution as +// "succeeded": it only distinguishes "returned" from "errored", never +// inspecting the value. A build that completed zero issues and merged nothing +// would therefore surface as green. +// +// Return &ReasonerFailed{...} when the reasoner has determined its own work +// failed but you still want the structured Result preserved on the execution +// record. The async handler posts status="failed" to the control plane while +// also sending Result and ErrorDetails (the control plane stores the result +// payload regardless of terminal status), so the rich outcome — debt, DAG +// state, any PR that was opened — is not lost behind a bare error string. +// +// Mirrors the Python SDK's ReasonerFailed exception (agentfield.exceptions). +type ReasonerFailed struct { + // Message is the human-readable failure summary; it becomes the execution + // error string. + Message string + + // Result is an optional structured result to preserve on the execution + // record (e.g. a BuildResult). JSON-encoded by the handler before posting. + Result any + + // ErrorDetails is optional structured error metadata mirrored onto the + // status payload's error_details field. + ErrorDetails any +} + +// Error implements the error interface, returning the failure message. +func (e *ReasonerFailed) Error() string { + return e.Message +} diff --git a/sdk/go/agent/reasoner_failed_test.go b/sdk/go/agent/reasoner_failed_test.go new file mode 100644 index 000000000..8327a4abc --- /dev/null +++ b/sdk/go/agent/reasoner_failed_test.go @@ -0,0 +1,224 @@ +package agent + +import ( + "bytes" + "context" + "encoding/json" + "errors" + "fmt" + "io" + "log" + "net/http" + "net/http/httptest" + "strings" + "sync/atomic" + "testing" + "time" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +func TestReasonerFailed_ErrorAndErrorsAs(t *testing.T) { + var err error = &ReasonerFailed{Message: "the work failed"} + assert.Equal(t, "the work failed", err.Error()) + + var rf *ReasonerFailed + require.True(t, errors.As(err, &rf)) + assert.Equal(t, "the work failed", rf.Message) +} + +// newStatusCapturingAgent returns an agent whose async status callbacks are +// delivered to statusCh, plus the server (caller must Close it). +func newStatusCapturingAgent(t *testing.T, statusCh chan map[string]any, failFirst bool, attempts *int32) (*Agent, *httptest.Server) { + t.Helper() + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + require.Contains(t, r.URL.Path, "/api/v1/executions/") + if !strings.HasSuffix(r.URL.Path, "/status") { + w.WriteHeader(http.StatusNoContent) + return + } + if attempts != nil { + n := atomic.AddInt32(attempts, 1) + if failFirst && n == 1 { + w.WriteHeader(http.StatusInternalServerError) + return + } + } + var payload map[string]any + require.NoError(t, json.NewDecoder(r.Body).Decode(&payload)) + statusCh <- payload + w.WriteHeader(http.StatusNoContent) + })) + + a, err := New(Config{ + NodeID: "node-1", + Version: "1.0.0", + AgentFieldURL: server.URL, + Logger: log.New(io.Discard, "", 0), + }) + require.NoError(t, err) + a.httpClient = server.Client() + return a, server +} + +// TestReasonerFailed_AsyncCarriesResultAndDetails maps to the contract: +// handler returns ReasonerFailed{Result: X} -> the posted failed-status body +// contains status=failed, error=Message, result=X (+ error_details). +func TestReasonerFailed_AsyncCarriesResultAndDetails(t *testing.T) { + statusCh := make(chan map[string]any, 1) + a, server := newStatusCapturingAgent(t, statusCh, false, nil) + defer server.Close() + + a.executeReasonerAsync(&Reasoner{ + Name: "fails-with-result", + Handler: func(_ context.Context, _ map[string]any) (any, error) { + return nil, &ReasonerFailed{ + Message: "build failed", + Result: map[string]any{"issues_done": 0, "pr": "none"}, + ErrorDetails: map[string]any{"reason": "no_merge"}, + } + }, + }, map[string]any{"x": 1}, ExecutionContext{ExecutionID: "exec-1", RunID: "run-1", WorkflowID: "wf-1"}) + + select { + case payload := <-statusCh: + assert.Equal(t, "failed", payload["status"]) + assert.Equal(t, "build failed", payload["error"]) + result, ok := payload["result"].(map[string]any) + require.True(t, ok, "result must be carried onto the failed status") + assert.EqualValues(t, 0, result["issues_done"]) + assert.Equal(t, "none", result["pr"]) + details, ok := payload["error_details"].(map[string]any) + require.True(t, ok, "error_details must be carried") + assert.Equal(t, "no_merge", details["reason"]) + case <-time.After(3 * time.Second): + t.Fatal("timed out waiting for status callback") + } +} + +// TestReasonerFailed_AsyncPlainErrorHasNoResultKey maps to the contract: +// a plain error -> no result key. +func TestReasonerFailed_AsyncPlainErrorHasNoResultKey(t *testing.T) { + statusCh := make(chan map[string]any, 1) + a, server := newStatusCapturingAgent(t, statusCh, false, nil) + defer server.Close() + + a.executeReasonerAsync(&Reasoner{ + Name: "plain-error", + Handler: func(_ context.Context, _ map[string]any) (any, error) { + return nil, fmt.Errorf("boom") + }, + }, map[string]any{"x": 1}, ExecutionContext{ExecutionID: "exec-2", RunID: "run-2", WorkflowID: "wf-2"}) + + select { + case payload := <-statusCh: + assert.Equal(t, "failed", payload["status"]) + assert.Equal(t, "boom", payload["error"]) + _, hasResult := payload["result"] + assert.False(t, hasResult, "plain error must not carry a result key") + _, hasDetails := payload["error_details"] + assert.False(t, hasDetails, "plain error must not carry an error_details key") + case <-time.After(3 * time.Second): + t.Fatal("timed out waiting for status callback") + } +} + +// TestReasonerFailed_AsyncRetriesOn5xxAndDeliversResult maps to the contract: +// the post is still retried (5x) on 5xx, and the result survives the retry. +func TestReasonerFailed_AsyncRetriesOn5xxAndDeliversResult(t *testing.T) { + statusCh := make(chan map[string]any, 1) + var attempts int32 + a, server := newStatusCapturingAgent(t, statusCh, true, &attempts) + defer server.Close() + + a.executeReasonerAsync(&Reasoner{ + Name: "retryable", + Handler: func(_ context.Context, _ map[string]any) (any, error) { + return nil, &ReasonerFailed{Message: "x", Result: map[string]any{"k": "v"}} + }, + }, map[string]any{"x": 1}, ExecutionContext{ExecutionID: "exec-3", RunID: "run-3", WorkflowID: "wf-3"}) + + select { + case payload := <-statusCh: + assert.Equal(t, "failed", payload["status"]) + result, ok := payload["result"].(map[string]any) + require.True(t, ok, "result must survive the retry") + assert.Equal(t, "v", result["k"]) + assert.GreaterOrEqual(t, atomic.LoadInt32(&attempts), int32(2), "a 5xx must have triggered at least one retry") + case <-time.After(5 * time.Second): + t.Fatal("timed out waiting for retried status callback") + } +} + +// TestReasonerFailed_SyncReasonerResponseCarriesResult exercises the sync +// handleReasoner path (agent has no control-plane URL, so no async dispatch). +func TestReasonerFailed_SyncReasonerResponseCarriesResult(t *testing.T) { + a, err := New(Config{NodeID: "node-1", Version: "1.0.0", Logger: log.New(io.Discard, "", 0)}) + require.NoError(t, err) + a.RegisterReasoner("failing", func(_ context.Context, _ map[string]any) (any, error) { + return nil, &ReasonerFailed{ + Message: "sync fail", + Result: map[string]any{"k": "v"}, + ErrorDetails: map[string]any{"reason": "boom"}, + } + }) + + req := httptest.NewRequest(http.MethodPost, "/reasoners/failing", bytes.NewBufferString(`{"input":{}}`)) + rec := httptest.NewRecorder() + a.Handler().ServeHTTP(rec, req) + + assert.Equal(t, http.StatusInternalServerError, rec.Code) + var resp map[string]any + require.NoError(t, json.Unmarshal(rec.Body.Bytes(), &resp)) + assert.Equal(t, "sync fail", resp["error"]) + result, ok := resp["result"].(map[string]any) + require.True(t, ok, "sync error response must carry the result") + assert.Equal(t, "v", result["k"]) + details, ok := resp["error_details"].(map[string]any) + require.True(t, ok) + assert.Equal(t, "boom", details["reason"]) +} + +// TestReasonerFailed_SyncExecuteResponseCarriesResult exercises the sync +// handleExecute path. +func TestReasonerFailed_SyncExecuteResponseCarriesResult(t *testing.T) { + a, err := New(Config{NodeID: "node-1", Version: "1.0.0", Logger: log.New(io.Discard, "", 0)}) + require.NoError(t, err) + a.RegisterReasoner("failing", func(_ context.Context, _ map[string]any) (any, error) { + return nil, &ReasonerFailed{Message: "exec fail", Result: map[string]any{"n": 2}} + }) + + req := httptest.NewRequest(http.MethodPost, "/execute/failing", bytes.NewBufferString(`{"input":{}}`)) + rec := httptest.NewRecorder() + a.Handler().ServeHTTP(rec, req) + + assert.Equal(t, http.StatusInternalServerError, rec.Code) + var resp map[string]any + require.NoError(t, json.Unmarshal(rec.Body.Bytes(), &resp)) + assert.Equal(t, "exec fail", resp["error"]) + result, ok := resp["result"].(map[string]any) + require.True(t, ok, "sync execute error response must carry the result") + assert.EqualValues(t, 2, result["n"]) +} + +// TestReasonerFailed_SyncPlainErrorHasNoResultKey guards the negative: a plain +// error on the sync path must not synthesize a result key. +func TestReasonerFailed_SyncPlainErrorHasNoResultKey(t *testing.T) { + a, err := New(Config{NodeID: "node-1", Version: "1.0.0", Logger: log.New(io.Discard, "", 0)}) + require.NoError(t, err) + a.RegisterReasoner("plain", func(_ context.Context, _ map[string]any) (any, error) { + return nil, fmt.Errorf("plain boom") + }) + + req := httptest.NewRequest(http.MethodPost, "/reasoners/plain", bytes.NewBufferString(`{"input":{}}`)) + rec := httptest.NewRecorder() + a.Handler().ServeHTTP(rec, req) + + assert.Equal(t, http.StatusInternalServerError, rec.Code) + var resp map[string]any + require.NoError(t, json.Unmarshal(rec.Body.Bytes(), &resp)) + assert.Equal(t, "plain boom", resp["error"]) + _, hasResult := resp["result"] + assert.False(t, hasResult) +} diff --git a/sdk/go/go.mod b/sdk/go/go.mod index 773e6a006..06836b5d3 100644 --- a/sdk/go/go.mod +++ b/sdk/go/go.mod @@ -3,6 +3,7 @@ module github.com/Agent-Field/agentfield/sdk/go go 1.21 require ( + github.com/santhosh-tekuri/jsonschema/v5 v5.3.1 github.com/stretchr/testify v1.11.1 gopkg.in/yaml.v3 v3.0.1 ) diff --git a/sdk/go/go.sum b/sdk/go/go.sum index c4c1710c4..5792edead 100644 --- a/sdk/go/go.sum +++ b/sdk/go/go.sum @@ -2,6 +2,8 @@ github.com/davecgh/go-spew v1.1.1 h1:vj9j/u1bqnvCEfJOwUhtlOARqs3+rkHYY13jYWTU97c github.com/davecgh/go-spew v1.1.1/go.mod h1:J7Y8YcW2NihsgmVo/mv3lAwl/skON4iLHjSsI+c5H38= github.com/pmezard/go-difflib v1.0.0 h1:4DBwDE0NGyQoBHbLQYPwSUPoCMWR5BEzIk/f1lZbAQM= github.com/pmezard/go-difflib v1.0.0/go.mod h1:iKH77koFhYxTK1pcRnkKkqfTogsbg7gZNVY4sRDYZ/4= +github.com/santhosh-tekuri/jsonschema/v5 v5.3.1 h1:lZUw3E0/J3roVtGQ+SCrUrg3ON6NgVqpn3+iol9aGu4= +github.com/santhosh-tekuri/jsonschema/v5 v5.3.1/go.mod h1:uToXkOrWAZ6/Oc07xWQrPOhJotwFIyu2bBVN41fcDUY= github.com/stretchr/testify v1.11.1 h1:7s2iGBzp5EwR7/aIZr8ao5+dra3wiQyKjjFuvgVKu7U= github.com/stretchr/testify v1.11.1/go.mod h1:wZwfW3scLgRK+23gO65QZefKpKQRnfz6sD981Nm4B6U= gopkg.in/check.v1 v0.0.0-20161208181325-20d25e280405 h1:yhCVgyC4o1eVCa2tZl7eS0r+SDo693bJlVdllGtEeKM= diff --git a/sdk/go/harness/claudecode.go b/sdk/go/harness/claudecode.go index debd5f96b..c15fda638 100644 --- a/sdk/go/harness/claudecode.go +++ b/sdk/go/harness/claudecode.go @@ -12,6 +12,10 @@ import ( // It uses the `claude` CLI with `--print` mode for non-interactive output. type ClaudeCodeProvider struct { BinPath string + + // runCLI is the subprocess runner, injectable for tests. It defaults to + // RunCLIWithStdin so the prompt can be delivered via stdin. + runCLI func(ctx context.Context, cmd []string, env map[string]string, cwd string, timeout int, stdin []byte) (*CLIResult, error) } // NewClaudeCodeProvider creates a Claude Code provider. If binPath is empty, @@ -20,7 +24,7 @@ func NewClaudeCodeProvider(binPath string) *ClaudeCodeProvider { if binPath == "" { binPath = "claude" } - return &ClaudeCodeProvider{BinPath: binPath} + return &ClaudeCodeProvider{BinPath: binPath, runCLI: RunCLIWithStdin} } // permissionMap translates common permission mode names to Claude Code flags. @@ -30,7 +34,17 @@ var permissionMap = map[string]string{ } func (p *ClaudeCodeProvider) Execute(ctx context.Context, prompt string, options Options) (*RawResult, error) { - cmd := []string{p.BinPath, "--print", "--output-format", "json"} + // stream-json (rather than plain json) keeps RunCLI's idle watchdog fed: + // with --output-format json the CLI is COMPLETELY SILENT until the run + // finishes, so any turn quieter than AGENTFIELD_HARNESS_IDLE_SECONDS + // (default 120s) was SIGKILLed mid-run. stream-json emits per-message + // JSONL events (init, thinking deltas, assistant messages, final result) + // as they happen — verified against claude CLI 2.1.191 — and the final + // "result" event carries the same fields as the json format (result, + // session_id, num_turns, total_cost_usd), so parseJSONOutput's contract + // is unchanged. In --print mode the CLI hard-requires --verbose alongside + // stream-json ("--output-format=stream-json requires --verbose"). + cmd := []string{p.BinPath, "--print", "--output-format", "stream-json", "--verbose"} if options.Model != "" { cmd = append(cmd, "--model", options.Model) @@ -64,8 +78,12 @@ func (p *ClaudeCodeProvider) Execute(ctx context.Context, prompt string, options cmd = append(cmd, "--allowedTools", tool) } - // The prompt is passed via stdin-like argument (last positional arg) - cmd = append(cmd, prompt) + // The prompt is delivered on stdin, NOT as a trailing positional argument. + // `--allowedTools` is variadic in the claude CLI (verified against 2.1.191): + // a positional prompt immediately following it is greedily absorbed into the + // tool list, so `--print` sees no prompt and exits non-zero with + // "Input must be provided ... when using --print". `claude --print` reads + // the prompt from stdin when no positional is present. env := make(map[string]string) for k, v := range options.Env { @@ -81,9 +99,14 @@ func (p *ClaudeCodeProvider) Execute(ctx context.Context, prompt string, options cwd = options.ProjectDir } + runCLI := p.runCLI + if runCLI == nil { + runCLI = RunCLIWithStdin + } + startAPI := time.Now() - cliResult, err := RunCLI(ctx, cmd, env, cwd, options.timeout()) + cliResult, err := runCLI(ctx, cmd, env, cwd, options.timeout(), []byte(prompt)) apiMS := int(time.Since(startAPI).Milliseconds()) if err != nil { @@ -109,7 +132,7 @@ func (p *ClaudeCodeProvider) Execute(ctx context.Context, prompt string, options return nil, err } - // Parse JSON output from Claude Code's --output-format json + // Parse the JSONL event stream from Claude Code's --output-format stream-json raw := &RawResult{ Metrics: Metrics{ DurationAPIMS: apiMS, @@ -144,9 +167,16 @@ func (p *ClaudeCodeProvider) Execute(ctx context.Context, prompt string, options return raw, nil } -// parseJSONOutput attempts to extract structured data from Claude Code's JSON output. +// parseJSONOutput extracts structured data from Claude Code's JSONL output. +// It consumes both the stream-json event stream (system/init, thinking +// deltas, assistant messages, terminal "result" event) and the legacy +// single-line json format: every line is parsed as an event, all events are +// collected into Messages (mirroring the Python provider, which appends every +// SDK stream message), and the LAST "result" event supplies Result, +// SessionID, CostUSD and NumTurns — its fields are identical across both +// output formats. Assistant-message text is only a fallback when no result +// event ever arrives (e.g. a stream truncated by a crash). func (p *ClaudeCodeProvider) parseJSONOutput(stdout string, raw *RawResult) { - // Claude Code with --output-format json outputs JSONL var messages []map[string]any var resultText string var sessionID string diff --git a/sdk/go/harness/claudecode_defaultrunner_test.go b/sdk/go/harness/claudecode_defaultrunner_test.go new file mode 100644 index 000000000..3bbcc242b --- /dev/null +++ b/sdk/go/harness/claudecode_defaultrunner_test.go @@ -0,0 +1,38 @@ +package harness + +import ( + "context" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +// TestClaudeCodeProvider_NilRunCLIUsesDefaultRunner covers the runCLI == nil +// fallback in Execute. NewClaudeCodeProvider always wires runCLI to +// RunCLIWithStdin, so a provider built via the constructor never exercises the +// fallback. A provider constructed as a struct literal (e.g. zero-valued +// runCLI) must still run: Execute defaults runCLI to RunCLIWithStdin and drives +// the real subprocess. +// +// Contract: with runCLI unset, Execute delivers the prompt to the default +// runner, runs the binary, and parses its stream-json result. +func TestClaudeCodeProvider_NilRunCLIUsesDefaultRunner(t *testing.T) { + dir := t.TempDir() + script := writeTestScript(t, dir, "claude", + `#!/bin/sh +echo '{"type":"result","result":"defaulted","session_id":"s-default","num_turns":1}' +`) + + // Struct literal (not NewClaudeCodeProvider) leaves runCLI nil, forcing the + // default-runner path in Execute. + p := &ClaudeCodeProvider{BinPath: script} + require.Nil(t, p.runCLI, "precondition: runCLI must be nil to exercise the fallback") + + raw, err := p.Execute(context.Background(), "hello", Options{}) + require.NoError(t, err) + require.False(t, raw.IsError, "unexpected error: %s", raw.ErrorMessage) + assert.Equal(t, "defaulted", raw.Result) + assert.Equal(t, "s-default", raw.Metrics.SessionID) + assert.Equal(t, 1, raw.Metrics.NumTurns) +} diff --git a/sdk/go/harness/claudecode_integration_test.go b/sdk/go/harness/claudecode_integration_test.go new file mode 100644 index 000000000..04de4e832 --- /dev/null +++ b/sdk/go/harness/claudecode_integration_test.go @@ -0,0 +1,71 @@ +//go:build integration + +package harness + +import ( + "context" + "os" + "testing" + "time" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +// TestClaudeCodeProvider_Integration_StdinWithTools drives the REAL claude CLI +// with an allowed-tools list set, which places the variadic --allowedTools flag +// last in the arg vector. Before the stdin fix the trailing positional prompt +// was swallowed by --allowedTools and the CLI exited non-zero with +// "Input must be provided ... when using --print". Delivering the prompt on +// stdin (no trailing positional) resolves it. +// +// It also proves the stream-json output path end-to-end: the provider invokes +// the CLI with `--output-format stream-json --verbose` (so the idle watchdog +// sees per-message progress instead of total silence), and the assertions on +// SessionID/CostUSD/NumTurns only pass if the terminal "result" event of the +// real stream was parsed. +// +// Requires an authenticated claude CLI. Its path is taken from +// AGENTFIELD_CLAUDE_BIN (the package TestMain shadows a bare "claude" on PATH +// with a stub, so an explicit path is required to reach the real binary). Opt-in: +// +// AGENTFIELD_CLAUDE_BIN="$(command -v claude)" \ +// go test -tags integration -run TestClaudeCodeProvider_Integration ./harness/ +func TestClaudeCodeProvider_Integration_StdinWithTools(t *testing.T) { + binPath := os.Getenv("AGENTFIELD_CLAUDE_BIN") + if binPath == "" { + t.Skip("set AGENTFIELD_CLAUDE_BIN to the real claude binary to run this test") + } + + ctx, cancel := context.WithTimeout(context.Background(), 120*time.Second) + defer cancel() + + p := NewClaudeCodeProvider(binPath) + raw, err := p.Execute(ctx, "Reply with exactly: HELLO_AGENTFIELD", Options{ + Model: "haiku", + Tools: []string{"Read", "Write"}, + Timeout: 120, + }) + require.NoError(t, err) + + t.Logf("IsError: %v", raw.IsError) + t.Logf("ErrorMessage: %s", raw.ErrorMessage) + t.Logf("Result: %s", raw.Result) + t.Logf("ReturnCode: %d", raw.ReturnCode) + t.Logf("SessionID: %s", raw.Metrics.SessionID) + t.Logf("NumTurns: %d", raw.Metrics.NumTurns) + if raw.Metrics.CostUSD != nil { + t.Logf("CostUSD: %f", *raw.Metrics.CostUSD) + } + + assert.False(t, raw.IsError, "expected no error, got: %s", raw.ErrorMessage) + assert.Contains(t, raw.Result, "HELLO_AGENTFIELD") + + // Stream-json parsing proof: these fields only exist on the terminal + // "result" event of the JSONL stream. + assert.NotEmpty(t, raw.Metrics.SessionID, "session_id must be parsed from the stream's result event") + require.NotNil(t, raw.Metrics.CostUSD, "total_cost_usd must be parsed from the stream's result event") + assert.Greater(t, *raw.Metrics.CostUSD, 0.0) + assert.GreaterOrEqual(t, raw.Metrics.NumTurns, 1) + assert.Greater(t, len(raw.Messages), 1, "stream-json must yield multiple events, not one json blob") +} diff --git a/sdk/go/harness/claudecode_test.go b/sdk/go/harness/claudecode_test.go new file mode 100644 index 000000000..dad45b271 --- /dev/null +++ b/sdk/go/harness/claudecode_test.go @@ -0,0 +1,147 @@ +package harness + +import ( + "context" + "os" + "path/filepath" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +// TestClaudeCodeProvider_PromptDeliveredViaStdin pins the fix for the variadic +// --allowedTools bug in the claude CLI (verified against 2.1.191). +// +// Contract: +// - The prompt must be handed to the subprocess on stdin. +// - The prompt must NOT appear as an argument, in particular not as a trailing +// positional after --allowedTools — claude's --allowedTools is variadic and +// greedily absorbs a following positional, leaving `--print` with no prompt +// and a non-zero exit ("Input must be provided ... when using --print"). +func TestClaudeCodeProvider_PromptDeliveredViaStdin(t *testing.T) { + p := NewClaudeCodeProvider("claude") + + var gotCmd []string + var gotStdin []byte + p.runCLI = func(_ context.Context, cmd []string, _ map[string]string, _ string, _ int, stdin []byte) (*CLIResult, error) { + gotCmd = append([]string(nil), cmd...) + gotStdin = append([]byte(nil), stdin...) + return &CLIResult{ + Stdout: `{"type":"result","result":"OK","session_id":"s1","num_turns":1}`, + ReturnCode: 0, + }, nil + } + + const prompt = "please reply with exactly OK" + raw, err := p.Execute(context.Background(), prompt, Options{ + Model: "haiku", + Tools: []string{"Read", "Write"}, + }) + require.NoError(t, err) + require.False(t, raw.IsError, "unexpected error: %s", raw.ErrorMessage) + + // 1. Prompt delivered via stdin. + assert.Equal(t, prompt, string(gotStdin), "prompt must be piped to the CLI on stdin") + + // 2. Prompt must not appear anywhere in the arg vector. + assert.NotContains(t, gotCmd, prompt, "prompt must not be passed as a CLI argument") + + // 3. The arg vector must end at the last --allowedTools value, never at a + // positional prompt — this is the exact regression being guarded. + require.NotEmpty(t, gotCmd) + assert.Equal(t, "Write", gotCmd[len(gotCmd)-1], + "arg vector must end at the last --allowedTools value, not a trailing positional prompt") + + // Sanity: the flags we expect are still present. + assert.Contains(t, gotCmd, "--allowedTools") + assert.Contains(t, gotCmd, "--print") + + // 4. Watchdog contract: the CLI must stream per-message events so RunCLI's + // idle watchdog sees progress — plain `--output-format json` is silent + // until completion and any turn quieter than the idle window was + // SIGKILLed mid-run. In --print mode the claude CLI hard-requires + // --verbose alongside stream-json. + assert.Contains(t, gotCmd, "--verbose") + for i, arg := range gotCmd { + if arg == "--output-format" { + require.Greater(t, len(gotCmd), i+1) + assert.Equal(t, "stream-json", gotCmd[i+1], + "output format must be stream-json to keep the idle watchdog fed") + } + } +} + +// TestClaudeCodeProvider_EmptyToolsStillStdin ensures the prompt is delivered on +// stdin even when no tools are set (no trailing positional in any case). +func TestClaudeCodeProvider_EmptyToolsStillStdin(t *testing.T) { + p := NewClaudeCodeProvider("claude") + + var gotCmd []string + var gotStdin []byte + p.runCLI = func(_ context.Context, cmd []string, _ map[string]string, _ string, _ int, stdin []byte) (*CLIResult, error) { + gotCmd = append([]string(nil), cmd...) + gotStdin = append([]byte(nil), stdin...) + return &CLIResult{Stdout: `{"type":"result","result":"OK"}`, ReturnCode: 0}, nil + } + + const prompt = "hello there" + _, err := p.Execute(context.Background(), prompt, Options{}) + require.NoError(t, err) + + assert.Equal(t, prompt, string(gotStdin)) + assert.NotContains(t, gotCmd, prompt) + require.NotEmpty(t, gotCmd) + assert.Equal(t, "--verbose", gotCmd[len(gotCmd)-1], + "vector ends at the base flags (--output-format stream-json --verbose), no positional prompt") +} + +// TestClaudeCodeProvider_ParsesStreamJSONFixture asserts the parser against a +// REAL captured stream: testdata/claude_stream_json_2.1.191.jsonl was produced +// by `claude --print --output-format stream-json --verbose --model haiku +// --allowedTools Read --allowedTools Write` (claude CLI 2.1.191) with the +// prompt "reply with just OK" delivered on stdin. Only environment-identifying +// inventories in the system/init event were sanitized; the event sequence and +// the terminal result event are byte-real. +// +// Contract (unchanged from the plain-json output format): +// - Result is the final result event's "result" text — not the raw stream. +// - SessionID / CostUSD / NumTurns come from the final result event. +// - Every stream event is retained in Messages (Python-provider parity: the +// Python claude provider appends every SDK stream message). +func TestClaudeCodeProvider_ParsesStreamJSONFixture(t *testing.T) { + fixture, err := os.ReadFile(filepath.Join("testdata", "claude_stream_json_2.1.191.jsonl")) + require.NoError(t, err) + + t.Run("parser consumes the raw capture", func(t *testing.T) { + raw := &RawResult{} + NewClaudeCodeProvider("").parseJSONOutput(string(fixture), raw) + + assert.Equal(t, "OK", raw.Result, "Result must be the final result event's text") + assert.Equal(t, "48a64026-56f9-4203-8220-1c099dd8378a", raw.Metrics.SessionID) + require.NotNil(t, raw.Metrics.CostUSD) + assert.InDelta(t, 0.0326441, *raw.Metrics.CostUSD, 1e-9) + assert.Equal(t, 1, raw.Metrics.NumTurns) + assert.Len(t, raw.Messages, 11, + "all stream events retained (init, rate_limit, thinking deltas, assistant, result)") + }) + + t.Run("Execute end-to-end over a subprocess emitting the capture", func(t *testing.T) { + dir := t.TempDir() + streamPath := filepath.Join(dir, "stream.jsonl") + require.NoError(t, os.WriteFile(streamPath, fixture, 0o644)) + script := writeTestScript(t, dir, "claude-stream", + "#!/bin/sh\ncat "+streamPath+"\n") + + raw, err := NewClaudeCodeProvider(script).Execute(context.Background(), "reply with just OK", Options{ + Tools: []string{"Read", "Write"}, + }) + require.NoError(t, err) + require.False(t, raw.IsError, "unexpected error: %s", raw.ErrorMessage) + assert.Equal(t, "OK", raw.Result) + assert.Equal(t, "48a64026-56f9-4203-8220-1c099dd8378a", raw.Metrics.SessionID) + require.NotNil(t, raw.Metrics.CostUSD) + assert.InDelta(t, 0.0326441, *raw.Metrics.CostUSD, 1e-9) + assert.Equal(t, 1, raw.Metrics.NumTurns) + }) +} diff --git a/sdk/go/harness/cli.go b/sdk/go/harness/cli.go index 3c5f864a5..d24ba2251 100644 --- a/sdk/go/harness/cli.go +++ b/sdk/go/harness/cli.go @@ -46,13 +46,27 @@ type CLIResult struct { ReturnCode int } -// RunCLI runs a CLI command and returns its output. The context controls -// cancellation; timeout is in seconds (0 means no timeout beyond ctx). +// RunCLI runs a CLI command with no standard input: the child sees an immediate +// EOF on stdin. It is a thin wrapper over RunCLIWithStdin; see that function for +// the full contract. +func RunCLI(ctx context.Context, cmd []string, env map[string]string, cwd string, timeout int) (*CLIResult, error) { + return RunCLIWithStdin(ctx, cmd, env, cwd, timeout, nil) +} + +// RunCLIWithStdin runs a CLI command and returns its output. The context +// controls cancellation; timeout is in seconds (0 means no timeout beyond ctx). +// +// stdin is fed to the child process's standard input. A nil (or empty) slice +// yields an immediate EOF — identical to RunCLI. Providers whose CLI reads the +// prompt from stdin (e.g. `claude --print`) pass the prompt bytes here instead +// of appending them as a trailing positional argument, which a preceding +// variadic flag (e.g. claude's `--allowedTools`) would otherwise greedily +// consume — leaving the CLI with no prompt and exiting non-zero. // // Environment merging: entries in env are merged with os.Environ(). An empty // string value ("") causes that variable to be removed from the environment // rather than set to empty — use this to unset inherited variables. -func RunCLI(ctx context.Context, cmd []string, env map[string]string, cwd string, timeout int) (*CLIResult, error) { +func RunCLIWithStdin(ctx context.Context, cmd []string, env map[string]string, cwd string, timeout int, stdin []byte) (*CLIResult, error) { if timeout > 0 { var cancel context.CancelFunc ctx, cancel = context.WithTimeout(ctx, time.Duration(timeout)*time.Second) @@ -94,14 +108,26 @@ func RunCLI(ctx context.Context, cmd []string, env map[string]string, cwd string c.Dir = cwd } - // Explicit null stdin so the child gets an immediate EOF instead of - // inheriting the parent's stdin (a hang risk if the child reads stdin). - c.Stdin = bytes.NewReader(nil) + // Feed the provided stdin to the child. A nil/empty slice yields an + // immediate EOF, so the child never inherits the parent's stdin (a hang + // risk if the child blocks reading stdin). When the child exits without + // draining stdin, the copy fails with EPIPE, which os/exec ignores. + c.Stdin = bytes.NewReader(stdin) // Run the child in its own process group so the idle watchdog can kill // the whole tree, not just the leader. setProcessGroup is platform-guarded. setProcessGroup(c) + // On ctx cancellation/timeout, kill the whole process group too. + // exec.CommandContext's default Cancel only kills the group LEADER, which + // orphans grandchildren (e.g. MCP servers spawned by the claude CLI) when + // a caller times out or cancels. On Windows killProcessGroup degrades to + // killing just the leader, matching the previous behavior. + c.Cancel = func() error { + killProcessGroup(c) + return nil + } + stdoutPipe, err := c.StdoutPipe() if err != nil { return nil, err diff --git a/sdk/go/harness/codex.go b/sdk/go/harness/codex.go index 37c6f665a..7c7efd7a5 100644 --- a/sdk/go/harness/codex.go +++ b/sdk/go/harness/codex.go @@ -4,6 +4,7 @@ import ( "context" "encoding/json" "fmt" + "os" "strings" "time" ) @@ -12,6 +13,19 @@ import ( // It uses `codex exec --json` for structured JSONL output. type CodexProvider struct { BinPath string + + // runCLI is the subprocess runner, injectable for tests. It defaults to + // RunCLIWithStdin so the prompt can be delivered via stdin (mirroring the + // claude provider). A nil value falls back to RunCLIWithStdin at call time. + runCLI func(ctx context.Context, cmd []string, env map[string]string, cwd string, timeout int, stdin []byte) (*CLIResult, error) + + // schemaPath / outputPath are set by the runner (via SetSchema) when a JSON + // schema is in effect. codex consumes the schema natively through + // --output-schema and persists its final message to --output-last-message, + // which the runner then reads — instead of relying on the Write-tool file + // protocol used by the claude/opencode providers. + schemaPath string + outputPath string } // NewCodexProvider creates a Codex provider. If binPath is empty, @@ -20,38 +34,83 @@ func NewCodexProvider(binPath string) *CodexProvider { if binPath == "" { binPath = "codex" } - return &CodexProvider{BinPath: binPath} + return &CodexProvider{BinPath: binPath, runCLI: RunCLIWithStdin} +} + +// SetSchema tells the codex provider that a JSON schema is in effect, giving it +// the deterministic paths where the runner wrote the strict schema and expects +// codex to persist its final JSON answer. The runner owns strict-schema +// computation and file writing; the provider only needs the paths so it can +// point codex's native --output-schema / --output-last-message flags at them. +// +// This implements the schemaAware interface the runner detects (see runner.go). +func (p *CodexProvider) SetSchema(schemaPath, outputPath string) { + p.schemaPath = schemaPath + p.outputPath = outputPath } func (p *CodexProvider) Execute(ctx context.Context, prompt string, options Options) (*RawResult, error) { - cmd := []string{p.BinPath, "exec", "--json"} + // --skip-git-repo-check lets the harness run in arbitrary working dirs + // (temp dirs, non-repo project roots); codex exec otherwise refuses to + // start outside a git repo. + cmd := []string{p.BinPath, "exec", "--json", "--skip-git-repo-check"} - if options.Cwd != "" { - cmd = append(cmd, "-C", options.Cwd) - } else if options.ProjectDir != "" { - cmd = append(cmd, "-C", options.ProjectDir) + cwd := options.Cwd + if cwd == "" { + cwd = options.ProjectDir + } + if cwd != "" { + cmd = append(cmd, "-C", cwd) } - if options.PermissionMode == "auto" { - cmd = append(cmd, "--full-auto") + // Pass the model through with -m. SWE-AF resolves gpt-5.5 vs gpt-5.3-codex by + // auth mode and relies on the value reaching the CLI; the old provider + // ignored options.Model entirely. + if options.Model != "" { + cmd = append(cmd, "-m", options.Model) } - // Prompt is the final positional argument. - cmd = append(cmd, prompt) + // permission_mode → sandbox policy (port of codex_harness_patch.py:165-170). + // codex exec never prompts (approval policy is always Never); the sandbox + // controls what it may write. --full-auto is deprecated in favour of the + // bypass flag / --sandbox. + switch options.PermissionMode { + case "auto": + cmd = append(cmd, "--dangerously-bypass-approvals-and-sandbox") + case "read-only", "workspace-write", "danger-full-access": + cmd = append(cmd, "--sandbox", options.PermissionMode) + default: + cmd = append(cmd, "--sandbox", "workspace-write") + } + + // Native structured output: when the runner has set a schema, point codex at + // the strict schema file and the last-message output file (patch lines + // 176-178). codex writes its final message to the last-message file, which + // the runner reads back. + if p.schemaPath != "" && fileExists(p.schemaPath) { + cmd = append(cmd, "--output-schema", p.schemaPath) + if p.outputPath != "" { + cmd = append(cmd, "--output-last-message", p.outputPath) + } + } env := make(map[string]string) for k, v := range options.Env { env[k] = v } - cwd := options.Cwd - if cwd == "" { - cwd = options.ProjectDir + runCLI := p.runCLI + if runCLI == nil { + runCLI = RunCLIWithStdin } startAPI := time.Now() - cliResult, err := RunCLI(ctx, cmd, env, cwd, options.timeout()) + // The prompt is delivered on stdin, NOT as a trailing positional argument + // (patch lines 84-102, mirroring the claude provider). codex exec reads the + // prompt from stdin, and delivering it there keeps large prompts off the + // argv and out of process listings. + cliResult, err := runCLI(ctx, cmd, env, cwd, options.timeout(), []byte(prompt)) apiMS := int(time.Since(startAPI).Milliseconds()) if err != nil { @@ -88,10 +147,21 @@ func (p *CodexProvider) Execute(ctx context.Context, prompt string, options Opti cleanStderr := StripANSI(strings.TrimSpace(cliResult.Stderr)) if stdout != "" { - raw.Result = stdout + // parseJSONLOutput sets raw.Result to the extracted final message only + // when one is present, so an empty Result below reliably signals "no + // parseable final text" and triggers the last-message fallback. p.parseJSONLOutput(stdout, raw) } + // Native last-message fallback: when stdout parsing yielded no usable final + // text but codex persisted its answer to the --output-last-message file, + // read it (patch lines 236-243). + if raw.Result == "" && p.outputPath != "" && fileExists(p.outputPath) { + if data, readErr := os.ReadFile(p.outputPath); readErr == nil { + raw.Result = strings.TrimSpace(string(data)) + } + } + if cliResult.ReturnCode != 0 && raw.Result == "" { raw.IsError = true raw.FailureType = FailureCrash diff --git a/sdk/go/harness/codex_test.go b/sdk/go/harness/codex_test.go new file mode 100644 index 000000000..1a4f82893 --- /dev/null +++ b/sdk/go/harness/codex_test.go @@ -0,0 +1,308 @@ +package harness + +import ( + "context" + "encoding/json" + "fmt" + "os" + "path/filepath" + "strings" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +// TestCodexStrictJSONSchema exercises the strict rewrite against a nested +// fixture: objects inside array items, an anyOf branch, and $defs. On every +// object node defaults must be stripped, required must list all property keys, +// and additionalProperties must be false. +func TestCodexStrictJSONSchema(t *testing.T) { + schema := map[string]any{ + "type": "object", + "properties": map[string]any{ + "name": map[string]any{"type": "string", "default": "anon"}, + // A boolean-schema property value (non-map) is passed through as-is. + "flag": true, + "items": map[string]any{ + "type": "array", + "items": map[string]any{ + "type": "object", + "properties": map[string]any{ + "id": map[string]any{"type": "integer", "default": 0}, + "label": map[string]any{"type": "string"}, + }, + }, + }, + "choice": map[string]any{ + "anyOf": []any{ + map[string]any{ + "type": "object", + "properties": map[string]any{ + "a": map[string]any{"type": "string", "default": "z"}, + }, + }, + // A non-map branch entry is passed through untouched. + true, + }, + }, + }, + "$defs": map[string]any{ + "Sub": map[string]any{ + "type": "object", + "properties": map[string]any{ + "v": map[string]any{"type": "string", "default": "d"}, + }, + }, + }, + // definitions with a non-map value exercises the pass-through branch. + "definitions": map[string]any{ + "Legacy": false, + }, + } + + strict := codexStrictJSONSchema(schema) + + // Top-level object: additionalProperties false, all keys required, no default. + assert.Equal(t, false, strict["additionalProperties"]) + assert.ElementsMatch(t, []string{"name", "flag", "items", "choice"}, strict["required"].([]string)) + // Non-map property values are passed through untouched. + assert.Equal(t, true, strict["properties"].(map[string]any)["flag"]) + + props := strict["properties"].(map[string]any) + name := props["name"].(map[string]any) + _, nameHasDefault := name["default"] + assert.False(t, nameHasDefault, "top-level default must be stripped") + + // Array items object. + items := props["items"].(map[string]any) + itemObj := items["items"].(map[string]any) + assert.Equal(t, false, itemObj["additionalProperties"]) + assert.ElementsMatch(t, []string{"id", "label"}, itemObj["required"].([]string)) + idProp := itemObj["properties"].(map[string]any)["id"].(map[string]any) + _, idHasDefault := idProp["default"] + assert.False(t, idHasDefault, "array-item default must be stripped") + + // anyOf branch object. + anyOf := props["choice"].(map[string]any)["anyOf"].([]any) + branch0 := anyOf[0].(map[string]any) + assert.Equal(t, false, branch0["additionalProperties"]) + assert.ElementsMatch(t, []string{"a"}, branch0["required"].([]string)) + aProp := branch0["properties"].(map[string]any)["a"].(map[string]any) + _, aHasDefault := aProp["default"] + assert.False(t, aHasDefault, "anyOf-branch default must be stripped") + // The non-map branch entry is passed through untouched. + assert.Equal(t, true, anyOf[1]) + + // $defs recursion. + sub := strict["$defs"].(map[string]any)["Sub"].(map[string]any) + assert.Equal(t, false, sub["additionalProperties"]) + assert.ElementsMatch(t, []string{"v"}, sub["required"].([]string)) + // definitions with a non-map value is passed through untouched. + assert.Equal(t, false, strict["definitions"].(map[string]any)["Legacy"]) + + // The source schema must be left unmutated (no additionalProperties added). + _, srcMutated := schema["additionalProperties"] + assert.False(t, srcMutated, "input schema must not be mutated") +} + +// TestCodexProvider_SandboxMapping asserts the permission_mode → sandbox flag +// mapping from codex_harness_patch.py:165-170. +func TestCodexProvider_SandboxMapping(t *testing.T) { + cases := []struct { + mode string + want []string + }{ + {"auto", []string{"--dangerously-bypass-approvals-and-sandbox"}}, + {"read-only", []string{"--sandbox", "read-only"}}, + {"workspace-write", []string{"--sandbox", "workspace-write"}}, + {"danger-full-access", []string{"--sandbox", "danger-full-access"}}, + {"", []string{"--sandbox", "workspace-write"}}, + {"something-else", []string{"--sandbox", "workspace-write"}}, + } + + for _, tc := range cases { + t.Run("mode="+tc.mode, func(t *testing.T) { + var gotCmd []string + p := NewCodexProvider("codex") + p.runCLI = func(_ context.Context, cmd []string, _ map[string]string, _ string, _ int, _ []byte) (*CLIResult, error) { + gotCmd = cmd + return &CLIResult{Stdout: `{"type":"result","result":"ok"}`, ReturnCode: 0}, nil + } + + _, err := p.Execute(context.Background(), "prompt", Options{PermissionMode: tc.mode}) + require.NoError(t, err) + + joined := strings.Join(gotCmd, " ") + assert.Contains(t, joined, strings.Join(tc.want, " ")) + assert.NotContains(t, gotCmd, "--full-auto") + }) + } +} + +// TestCodexProvider_NativeSchemaArgv verifies that when a schema file is set, +// the argv points --output-schema at it and --output-last-message at the +// output file, and that the schema file on disk is the strict rewrite. +func TestCodexProvider_NativeSchemaArgv(t *testing.T) { + dir := t.TempDir() + schemaPath := SchemaPath(dir) + outputPath := OutputPath(dir) + + // Emulate what the runner writes: the strict rewrite of the source schema. + source := map[string]any{ + "type": "object", + "properties": map[string]any{ + "status": map[string]any{"type": "string", "default": "unknown"}, + }, + } + strictJSON, err := json.MarshalIndent(codexStrictJSONSchema(source), "", " ") + require.NoError(t, err) + require.NoError(t, os.WriteFile(schemaPath, strictJSON, 0o600)) + + var gotCmd []string + p := NewCodexProvider("codex") + p.SetSchema(schemaPath, outputPath) + p.runCLI = func(_ context.Context, cmd []string, _ map[string]string, _ string, _ int, _ []byte) (*CLIResult, error) { + gotCmd = cmd + return &CLIResult{Stdout: `{"type":"result","result":"{\"status\":\"ok\"}"}`, ReturnCode: 0}, nil + } + + _, err = p.Execute(context.Background(), "prompt", Options{Model: "gpt-5.5"}) + require.NoError(t, err) + + joined := strings.Join(gotCmd, " ") + assert.Contains(t, joined, "--output-schema "+schemaPath) + assert.Contains(t, joined, "--output-last-message "+outputPath) + + // The schema file on disk must be the strict rewrite. + var written map[string]any + raw, err := os.ReadFile(schemaPath) + require.NoError(t, err) + require.NoError(t, json.Unmarshal(raw, &written)) + assert.Equal(t, false, written["additionalProperties"]) + assert.ElementsMatch(t, []string{"status"}, toStringSlice(written["required"])) + statusProp := written["properties"].(map[string]any)["status"].(map[string]any) + _, hasDefault := statusProp["default"] + assert.False(t, hasDefault) +} + +// TestCodexProvider_LastMessageFallback verifies that when stdout carries no +// parseable final text but codex persisted its answer to the output file, the +// result is read from the file (codex_harness_patch.py:236-243). +func TestCodexProvider_LastMessageFallback(t *testing.T) { + dir := t.TempDir() + schemaPath := SchemaPath(dir) + outputPath := OutputPath(dir) + require.NoError(t, os.WriteFile(schemaPath, []byte(`{"type":"object"}`), 0o600)) + + p := NewCodexProvider("codex") + p.SetSchema(schemaPath, outputPath) + p.runCLI = func(_ context.Context, _ []string, _ map[string]string, _ string, _ int, _ []byte) (*CLIResult, error) { + // codex writes its final message to the last-message file; stdout has + // only non-final events, so parsing yields no result text. + require.NoError(t, os.WriteFile(outputPath, []byte(`{"status":"ok"}`), 0o600)) + return &CLIResult{Stdout: `{"type":"turn.started"}`, ReturnCode: 0}, nil + } + + raw, err := p.Execute(context.Background(), "prompt", Options{}) + require.NoError(t, err) + assert.False(t, raw.IsError) + assert.Equal(t, `{"status":"ok"}`, raw.Result) +} + +// TestCodexProvider_TimeoutSurfacesFailureTimeout verifies a timed-out run is +// classified as FailureTimeout, not a generic crash. +func TestCodexProvider_TimeoutSurfacesFailureTimeout(t *testing.T) { + p := NewCodexProvider("codex") + p.runCLI = func(_ context.Context, _ []string, _ map[string]string, _ string, _ int, _ []byte) (*CLIResult, error) { + return nil, fmt.Errorf("CLI command timed out after 1s: codex exec") + } + + raw, err := p.Execute(context.Background(), "prompt", Options{}) + require.NoError(t, err) + assert.True(t, raw.IsError) + assert.Equal(t, FailureTimeout, raw.FailureType) +} + +// TestCodexProvider_StdoutPreferredOverFile verifies the fallback does NOT fire +// when stdout already yields a final message. +func TestCodexProvider_StdoutPreferredOverFile(t *testing.T) { + dir := t.TempDir() + schemaPath := SchemaPath(dir) + outputPath := OutputPath(dir) + require.NoError(t, os.WriteFile(schemaPath, []byte(`{"type":"object"}`), 0o600)) + + p := NewCodexProvider("codex") + p.SetSchema(schemaPath, outputPath) + p.runCLI = func(_ context.Context, _ []string, _ map[string]string, _ string, _ int, _ []byte) (*CLIResult, error) { + require.NoError(t, os.WriteFile(outputPath, []byte(`{"from":"file"}`), 0o600)) + return &CLIResult{Stdout: `{"type":"result","result":"from-stdout"}`, ReturnCode: 0}, nil + } + + raw, err := p.Execute(context.Background(), "prompt", Options{}) + require.NoError(t, err) + assert.Equal(t, "from-stdout", raw.Result) +} + +// TestRunner_Run_CodexNativeSchemaEndToEnd drives the runner with a fake codex +// binary and asserts the native path end-to-end: the runner writes the strict +// schema and hands codex a codex-native suffix (not the Write-tool one), codex +// persists its answer to --output-last-message, and the runner reads it back. +func TestRunner_Run_CodexNativeSchemaEndToEnd(t *testing.T) { + dir := t.TempDir() + promptFile := filepath.Join(dir, "captured-prompt.txt") + + // The fake codex reads the prompt from stdin (capturing it), locates the + // --output-last-message path in its args, writes valid JSON there, and emits + // an empty result event so the runner must use the file, not stdout. + script := writeTestScript(t, dir, "codex", + "#!/bin/sh\n"+ + "cat > \""+promptFile+"\"\n"+ + "out=\"\"\n"+ + "prev=\"\"\n"+ + "for a in \"$@\"; do\n"+ + " if [ \"$prev\" = \"--output-last-message\" ]; then out=\"$a\"; fi\n"+ + " prev=\"$a\"\n"+ + "done\n"+ + "if [ -n \"$out\" ]; then printf '%s' '{\"status\":\"ok\"}' > \"$out\"; fi\n"+ + "printf '%s\\n' '{\"type\":\"result\",\"result\":\"\"}'\n") + + runner := NewRunner(Options{Provider: ProviderCodex, BinPath: script}) + + var dest struct { + Status string `json:"status"` + } + result, err := runner.Run(context.Background(), "do the work", map[string]any{ + "type": "object", + "properties": map[string]any{ + "status": map[string]any{"type": "string"}, + }, + }, &dest, Options{ProjectDir: dir}) + require.NoError(t, err) + require.NotNil(t, result) + assert.False(t, result.IsError, "expected success, got: %s", result.ErrorMessage) + assert.Equal(t, "ok", dest.Status) + + captured, err := os.ReadFile(promptFile) + require.NoError(t, err) + prompt := string(captured) + assert.Contains(t, prompt, "CRITICAL CODEX STRUCTURED OUTPUT REQUIREMENTS") + assert.NotContains(t, prompt, "use your Write tool") +} + +func toStringSlice(v any) []string { + switch vv := v.(type) { + case []string: + return vv + case []any: + out := make([]string, 0, len(vv)) + for _, item := range vv { + if s, ok := item.(string); ok { + out = append(out, s) + } + } + return out + default: + return nil + } +} diff --git a/sdk/go/harness/opencode.go b/sdk/go/harness/opencode.go index 940e0d6b3..2fbcc8dbb 100644 --- a/sdk/go/harness/opencode.go +++ b/sdk/go/harness/opencode.go @@ -5,6 +5,7 @@ import ( "encoding/json" "fmt" "os" + "regexp" "strconv" "strings" "sync" @@ -57,7 +58,11 @@ func (p *OpenCodeProvider) Execute(ctx context.Context, prompt string, options O // -p → --password (provider password) // so the old invocation made the binary print help and exit without // running, leaving callers with empty trajectories. See issue #517. - cmd := []string{p.BinPath, "run"} + // + // --format json emits a JSONL event stream (step_start / text / step_finish + // / tool_use / error) instead of plain text, which lets us recover the final + // message, per-step cost, and turn count, and surface in-band error events. + cmd := []string{p.BinPath, "run", "--format", "json"} // OpenCode uses --dir for the project directory the agent operates on. // ProjectDir is the canonical caller-facing field; fall back to Cwd if @@ -169,21 +174,32 @@ func (p *OpenCodeProvider) Execute(ctx context.Context, prompt string, options O return nil, err } - resultText := strings.TrimSpace(cliResult.Stdout) cleanStderr := StripANSI(strings.TrimSpace(cliResult.Stderr)) + // Parse the JSON event stream. When opencode emitted no parseable events + // (older versions, or a hard failure before any output), fall back to the + // trimmed raw stdout so plain-text output is still surfaced. + events := parseOpenCodeEvents(cliResult.Stdout) + var resultText string + if len(events) > 0 { + resultText = extractOpenCodeFinalText(events) + } else { + resultText = strings.TrimSpace(cliResult.Stdout) + } + eventError := extractOpenCodeEventError(events) + raw := &RawResult{ Result: resultText, - Messages: nil, + Messages: events, Metrics: Metrics{ DurationAPIMS: apiMS, - NumTurns: 1, SessionID: "", }, ReturnCode: cliResult.ReturnCode, } - if cliResult.ReturnCode < 0 { + switch { + case cliResult.ReturnCode < 0: raw.FailureType = FailureCrash raw.IsError = true if cleanStderr != "" { @@ -192,23 +208,243 @@ func (p *OpenCodeProvider) Execute(ctx context.Context, prompt string, options O } else { raw.ErrorMessage = fmt.Sprintf("Process killed by signal %d.", -cliResult.ReturnCode) } - } else if cliResult.ReturnCode != 0 && resultText == "" { + case cliResult.ReturnCode != 0 && resultText == "": raw.FailureType = FailureCrash raw.IsError = true if cleanStderr != "" { - raw.ErrorMessage = truncate(cleanStderr, 1000) + raw.ErrorMessage = extractOpenCodeError(cleanStderr) } else { raw.ErrorMessage = fmt.Sprintf("Process exited with code %d and produced no output.", cliResult.ReturnCode) } + case eventError != "" && resultText == "": + raw.FailureType = FailureCrash + raw.IsError = true + raw.ErrorMessage = eventError + case resultText == "" && cleanStderr != "" && matchesOpenCodeError(cleanStderr): + // opencode sometimes exits 0 even on hard failures like "Model not + // found" or auth errors — surface the real error from stderr instead + // of silently returning empty output that downstream callers would + // interpret as "the agent produced no valid result". + raw.FailureType = FailureCrash + raw.IsError = true + raw.ErrorMessage = extractOpenCodeError(cleanStderr) } - if resultText == "" { - raw.Metrics.NumTurns = 0 + // Turn count: prefer the event-derived count, else 1 when a result exists. + numTurns := countTurnsFromEvents(events) + if numTurns == 0 && resultText != "" { + numTurns = 1 } + raw.Metrics.NumTurns = numTurns + raw.Metrics.CostUSD = costFromEvents(events) return raw, nil } +// opencode CLI sometimes prints a hard error to stderr but exits 0 (notably +// "Model not found", auth errors, schema-validation failures). These patterns +// mark stderr as carrying a real failure rather than noise like the one-time +// SQLite migration prelude. Ported from providers/opencode.py:29-35. +var openCodeStderrErrorPatterns = []*regexp.Regexp{ + regexp.MustCompile(`(?m)^Error:`), + regexp.MustCompile(`\bModel not found\b`), + regexp.MustCompile(`\bAuthenticationError\b`), + regexp.MustCompile(`\bUnauthorized\b`), + regexp.MustCompile(`\bAPIError\b`), +} + +func matchesOpenCodeError(stderr string) bool { + for _, pat := range openCodeStderrErrorPatterns { + if pat.MatchString(stderr) { + return true + } + } + return false +} + +// parseOpenCodeEvents parses opencode's JSONL event stream, skipping any line +// that is not valid JSON (e.g. interleaved plain-text log lines). +func parseOpenCodeEvents(stdout string) []map[string]any { + var events []map[string]any + for _, line := range strings.Split(strings.TrimSpace(stdout), "\n") { + line = strings.TrimSpace(line) + if line == "" { + continue + } + var event map[string]any + if err := json.Unmarshal([]byte(line), &event); err != nil { + continue + } + events = append(events, event) + } + return events +} + +// extractOpenCodeFinalText reconstructs the final assistant text from the event +// stream. Ported from _cli.extract_final_text (the branches opencode emits): +// step_start resets the accumulated text; "text" events append part/text +// content; result/message/assistant/turn.completed/item.completed carry a final +// message directly. +func extractOpenCodeFinalText(events []map[string]any) string { + var resultText string + var currentParts []string + + for _, event := range events { + eventType, _ := event["type"].(string) + switch eventType { + case "step_start": + currentParts = nil + case "item.completed": + if item, ok := event["item"].(map[string]any); ok { + if it, _ := item["type"].(string); it == "agent_message" { + if text, ok := item["text"].(string); ok && text != "" { + resultText = text + } + } + } + case "result": + if r, ok := event["result"].(string); ok { + resultText = r + } else if r, ok := event["text"].(string); ok { + resultText = r + } + case "turn.completed": + if text, ok := event["text"].(string); ok && text != "" { + resultText = text + } + case "message", "assistant": + if content, ok := event["content"].(string); ok && content != "" { + resultText = content + } else if content, ok := event["text"].(string); ok && content != "" { + resultText = content + } + case "text": + content := stringField(event, "text") + if content == "" { + content = stringField(event, "content") + } + if content == "" { + if part, ok := event["part"].(map[string]any); ok { + content = stringField(part, "text") + } + } + if content != "" { + currentParts = append(currentParts, content) + resultText = strings.Join(currentParts, "") + } + } + } + return resultText +} + +// stringField returns m[key] when it is a non-empty string, else "". +func stringField(m map[string]any, key string) string { + if v, ok := m[key].(string); ok { + return v + } + return "" +} + +// countTurnsFromEvents counts opencode turns: one per step_start event, or — +// when the stream has no step markers — one per tool_use event. Ported from +// providers/opencode.py:_count_turns_from_events. +func countTurnsFromEvents(events []map[string]any) int { + stepStarts := 0 + toolUses := 0 + for _, event := range events { + switch t, _ := event["type"].(string); t { + case "step_start": + stepStarts++ + case "tool_use": + toolUses++ + } + } + if stepStarts > 0 { + return stepStarts + } + return toolUses +} + +// costFromEvents sums opencode per-step costs from step_finish events. Returns +// nil when no step carried a cost, so callers distinguish "unknown" from +// "$0.00". Ported from providers/opencode.py:_cost_from_events. +func costFromEvents(events []map[string]any) *float64 { + total := 0.0 + found := false + for _, event := range events { + if t, _ := event["type"].(string); t != "step_finish" { + continue + } + part, ok := event["part"].(map[string]any) + if !ok { + continue + } + // JSON numbers decode to float64; bool cost values never match here, + // matching the Python guard against isinstance(cost, bool). + if cost, ok := part["cost"].(float64); ok { + total += cost + found = true + } + } + if !found { + return nil + } + return &total +} + +// extractOpenCodeEventError pulls a meaningful failure message from an in-band +// JSON "error" event. Ported from providers/opencode.py:_extract_opencode_event_error. +func extractOpenCodeEventError(events []map[string]any) string { + for _, event := range events { + if t, _ := event["type"].(string); t != "error" { + continue + } + for _, key := range []string{"message", "error", "text"} { + if v := strings.TrimSpace(stringField(event, key)); v != "" { + return truncate(v, 1000) + } + } + if part, ok := event["part"].(map[string]any); ok { + for _, key := range []string{"message", "error", "text"} { + if v := strings.TrimSpace(stringField(part, key)); v != "" { + return truncate(v, 1000) + } + } + } + if b, err := json.Marshal(event); err == nil { + return truncate(string(b), 1000) + } + return "" + } + return "" +} + +// extractOpenCodeError pulls the meaningful failure line(s) out of opencode +// stderr. opencode's stderr typically opens with the SQLite migration prelude +// followed by the real error, so prefer the line carrying an error marker plus +// a small window of context. Ported from +// providers/opencode.py:_extract_opencode_error. +func extractOpenCodeError(stderr string) string { + lines := strings.Split(stderr, "\n") + for i, line := range lines { + for _, pat := range openCodeStderrErrorPatterns { + if pat.MatchString(line) { + start := i - 1 + if start < 0 { + start = 0 + } + end := i + 5 + if end > len(lines) { + end = len(lines) + } + window := strings.Join(lines[start:end], "\n") + return truncate(strings.TrimSpace(window), 1000) + } + } + } + return truncate(stderr, 1000) +} + func mergedProcessEnv(overrides map[string]string) map[string]string { merged := make(map[string]string) for _, entry := range os.Environ() { diff --git a/sdk/go/harness/opencode_json_test.go b/sdk/go/harness/opencode_json_test.go new file mode 100644 index 000000000..00485eaa1 --- /dev/null +++ b/sdk/go/harness/opencode_json_test.go @@ -0,0 +1,225 @@ +package harness + +import ( + "context" + "strings" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +// fakeOpenCode returns an OpenCodeProvider whose subprocess is replaced by a +// runCLI that yields the given stdout/stderr/returncode and captures the argv. +func fakeOpenCode(stdout, stderr string, code int, gotCmd *[]string) *OpenCodeProvider { + p := NewOpenCodeProvider("opencode", "") + p.runCLI = func(_ context.Context, cmd []string, _ map[string]string, _ string, _ int) (*CLIResult, error) { + if gotCmd != nil { + *gotCmd = cmd + } + return &CLIResult{Stdout: stdout, Stderr: stderr, ReturnCode: code}, nil + } + return p +} + +// TestOpenCode_JSONEventsParsed maps to the contract: a fake CLI emitting JSON +// events yields a parsed result plus event-derived cost and turn count, and the +// argv requests --format json. +func TestOpenCode_JSONEventsParsed(t *testing.T) { + stream := strings.Join([]string{ + `{"type":"step_start"}`, + `{"type":"text","part":{"text":"first "}}`, + `{"type":"step_finish","part":{"cost":0.01}}`, + `{"type":"step_start"}`, + `{"type":"text","part":{"text":"final answer"}}`, + `{"type":"step_finish","part":{"cost":0.02}}`, + }, "\n") + + var gotCmd []string + p := fakeOpenCode(stream, "", 0, &gotCmd) + raw, err := p.Execute(context.Background(), "prompt", Options{}) + require.NoError(t, err) + + assert.False(t, raw.IsError) + assert.Equal(t, "final answer", raw.Result, "step_start resets accumulated text") + assert.Equal(t, 2, raw.Metrics.NumTurns, "one turn per step_start") + require.NotNil(t, raw.Metrics.CostUSD) + assert.InDelta(t, 0.03, *raw.Metrics.CostUSD, 1e-9) + assert.Len(t, raw.Messages, 6) + + joined := strings.Join(gotCmd, " ") + assert.Contains(t, joined, "run --format json") +} + +// TestOpenCode_ToolUseTurnFallback verifies the turn count falls back to +// tool_use events when there are no step markers, and that cost is nil when no +// step_finish carried a cost. +func TestOpenCode_ToolUseTurnFallback(t *testing.T) { + stream := strings.Join([]string{ + `{"type":"tool_use"}`, + `{"type":"tool_use"}`, + `{"type":"result","result":"done"}`, + }, "\n") + + p := fakeOpenCode(stream, "", 0, nil) + raw, err := p.Execute(context.Background(), "prompt", Options{}) + require.NoError(t, err) + + assert.False(t, raw.IsError) + assert.Equal(t, "done", raw.Result) + assert.Equal(t, 2, raw.Metrics.NumTurns) + assert.Nil(t, raw.Metrics.CostUSD, "no step_finish cost -> unknown, not zero") +} + +// TestOpenCode_Exit0AuthErrorSurfaced maps to the contract: exit 0 + stderr +// "AuthenticationError..." + empty result must surface IsError carrying the +// matched message rather than silently returning empty output. +func TestOpenCode_Exit0AuthErrorSurfaced(t *testing.T) { + stderr := "Performing one time database migration...\nError: AuthenticationError: invalid api key\nmore context\n" + + p := fakeOpenCode("", stderr, 0, nil) + raw, err := p.Execute(context.Background(), "prompt", Options{}) + require.NoError(t, err) + + assert.True(t, raw.IsError, "auth error on exit-0 must be surfaced") + assert.Equal(t, FailureCrash, raw.FailureType) + assert.Contains(t, raw.ErrorMessage, "AuthenticationError") + // The migration prelude should not be all that surfaces. + assert.NotEqual(t, "Performing one time database migration...", raw.ErrorMessage) +} + +// TestOpenCode_Exit0ModelNotFoundSurfaced covers the "Model not found" pattern. +func TestOpenCode_Exit0ModelNotFoundSurfaced(t *testing.T) { + p := fakeOpenCode("", "Model not found: gpt-nonexistent", 0, nil) + raw, err := p.Execute(context.Background(), "prompt", Options{}) + require.NoError(t, err) + + assert.True(t, raw.IsError) + assert.Contains(t, raw.ErrorMessage, "Model not found") +} + +// TestOpenCode_InBandErrorEvent verifies an in-band JSON error event is +// surfaced when there is no final result text. +func TestOpenCode_InBandErrorEvent(t *testing.T) { + stream := `{"type":"error","message":"provider APIError: 500"}` + p := fakeOpenCode(stream, "", 0, nil) + raw, err := p.Execute(context.Background(), "prompt", Options{}) + require.NoError(t, err) + + assert.True(t, raw.IsError) + assert.Equal(t, FailureCrash, raw.FailureType) + assert.Contains(t, raw.ErrorMessage, "APIError: 500") +} + +// TestOpenCode_HealthyRunUnaffected maps to the contract: a healthy run is +// unaffected. Benign stderr noise (no error markers) with a valid result does +// not flip IsError. +func TestOpenCode_HealthyRunUnaffected(t *testing.T) { + stream := strings.Join([]string{ + `{"type":"step_start"}`, + `{"type":"text","part":{"text":"all good"}}`, + }, "\n") + + p := fakeOpenCode(stream, "Performing one time database migration...", 0, nil) + raw, err := p.Execute(context.Background(), "prompt", Options{}) + require.NoError(t, err) + + assert.False(t, raw.IsError, "benign stderr + valid result must stay healthy") + assert.Equal(t, "all good", raw.Result) + assert.Equal(t, 1, raw.Metrics.NumTurns) +} + +// TestExtractOpenCodeFinalText covers each final-text event shape. +func TestExtractOpenCodeFinalText(t *testing.T) { + cases := []struct { + name string + events []map[string]any + want string + }{ + { + name: "item.completed agent_message", + events: []map[string]any{ + {"type": "item.completed", "item": map[string]any{"type": "agent_message", "text": "hi there"}}, + }, + want: "hi there", + }, + { + name: "result field", + events: []map[string]any{{"type": "result", "result": "R"}}, + want: "R", + }, + { + name: "result via text field", + events: []map[string]any{{"type": "result", "text": "RT"}}, + want: "RT", + }, + { + name: "turn.completed text", + events: []map[string]any{{"type": "turn.completed", "text": "turn done"}}, + want: "turn done", + }, + { + name: "assistant content", + events: []map[string]any{{"type": "assistant", "content": "assistant says"}}, + want: "assistant says", + }, + { + name: "message via text field", + events: []map[string]any{{"type": "message", "text": "msg text"}}, + want: "msg text", + }, + { + name: "text direct field accumulates", + events: []map[string]any{{"type": "text", "text": "a"}, {"type": "text", "text": "b"}}, + want: "ab", + }, + { + name: "text content field", + events: []map[string]any{{"type": "text", "content": "cc"}}, + want: "cc", + }, + { + name: "no final text", + events: []map[string]any{{"type": "step_start"}, {"type": "tool_use"}}, + want: "", + }, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + assert.Equal(t, tc.want, extractOpenCodeFinalText(tc.events)) + }) + } +} + +// TestExtractOpenCodeEventError covers the error-event extraction shapes. +func TestExtractOpenCodeEventError(t *testing.T) { + t.Run("top-level error key", func(t *testing.T) { + got := extractOpenCodeEventError([]map[string]any{{"type": "error", "error": "boom"}}) + assert.Equal(t, "boom", got) + }) + t.Run("part-nested message", func(t *testing.T) { + got := extractOpenCodeEventError([]map[string]any{ + {"type": "error", "part": map[string]any{"message": "nested failure"}}, + }) + assert.Equal(t, "nested failure", got) + }) + t.Run("marshal fallback when no known key", func(t *testing.T) { + got := extractOpenCodeEventError([]map[string]any{{"type": "error", "code": 500.0}}) + assert.Contains(t, got, "500") + }) + t.Run("no error event", func(t *testing.T) { + assert.Equal(t, "", extractOpenCodeEventError([]map[string]any{{"type": "text", "text": "x"}})) + }) +} + +// TestOpenCode_PlainTextFallback verifies non-JSON stdout still surfaces as the +// result (older opencode versions / degraded output). +func TestOpenCode_PlainTextFallback(t *testing.T) { + p := fakeOpenCode("just plain text\n", "", 0, nil) + raw, err := p.Execute(context.Background(), "prompt", Options{}) + require.NoError(t, err) + + assert.False(t, raw.IsError) + assert.Equal(t, "just plain text", raw.Result) + assert.Equal(t, 1, raw.Metrics.NumTurns) +} diff --git a/sdk/go/harness/runner.go b/sdk/go/harness/runner.go index 9afa3b322..812f5a969 100644 --- a/sdk/go/harness/runner.go +++ b/sdk/go/harness/runner.go @@ -9,6 +9,7 @@ import ( "math" "math/rand" "os" + "path/filepath" "reflect" "slices" "strings" @@ -31,6 +32,18 @@ func NewRunner(defaults Options) *Runner { } } +// schemaAware is implemented by providers that consume a JSON schema natively +// (codex's --output-schema / --output-last-message) instead of the Write-tool +// file protocol used by the claude/opencode providers. When a schema is +// present the runner writes the STRICT schema rewrite, hands the provider the +// deterministic schema/output paths, and uses a codex-native prompt suffix +// rather than BuildPromptSuffix — matching the provider-dispatching suffix in +// codex_harness_patch.py. Providers that do not implement this keep the +// original file-write protocol, so this is fully back-compatible. +type schemaAware interface { + SetSchema(schemaPath, outputPath string) +} + // Run dispatches a prompt to a coding agent and returns the result. // If schema is non-nil, the runner instructs the agent to write structured // JSON output and validates it, retrying on failure. @@ -73,11 +86,46 @@ func (r *Runner) Run(ctx context.Context, prompt string, schema map[string]any, // "auto" — incremental only when the schema is large, else single useIncremental := resolveIncremental(schema, opts) + // Native-schema providers (codex) consume the schema through CLI flags + // instead of the Write-tool file protocol. Detect and prepare before the + // prompt suffix is built so codex gets a codex-native instruction while + // claude/opencode keep the Write-tool suffix. + var nativeSchemaPath, nativeOutputPath string + useNativeSchema := false + if schema != nil { + if sa, ok := provider.(schemaAware); ok { + absDir, absErr := filepath.Abs(outputDir) + if absErr != nil { + absDir = outputDir + } + nativeSchemaPath = SchemaPath(absDir) + nativeOutputPath = OutputPath(absDir) + if strictJSON, mErr := json.MarshalIndent(codexStrictJSONSchema(schema), "", " "); mErr == nil { + if mkErr := os.MkdirAll(filepath.Dir(nativeSchemaPath), 0o700); mkErr == nil { + if wErr := os.WriteFile(nativeSchemaPath, strictJSON, 0o600); wErr == nil { + sa.SetSchema(nativeSchemaPath, nativeOutputPath) + useNativeSchema = true + // Clean up unconditionally: CleanupTempFiles no-ops when + // outputDir is "." so the strict schema / native output + // file would otherwise leak into the working directory. + defer func() { + _ = os.Remove(nativeSchemaPath) + _ = os.Remove(nativeOutputPath) + }() + } + } + } + } + } + effectivePrompt := prompt if schema != nil { - if useIncremental { + switch { + case useNativeSchema: + effectivePrompt = prompt + BuildCodexNativeSuffix(nativeSchemaPath, nativeOutputPath) + case useIncremental: effectivePrompt = prompt + BuildIncrementalPromptSuffix(schema, outputDir) - } else { + default: effectivePrompt = prompt + BuildPromptSuffix(schema, outputDir) } } @@ -319,6 +367,16 @@ func (r *Runner) handleSchemaWithRetry( r.Logger.Println("Stdout fallback succeeded") } } + // Enforce the schema: unmarshal-only parsing (ParseAndValidate / + // TryParseFromText) accepts missing required fields, invalid enums and + // extra fields silently, so validate here to drive the retry loop below. + if err == nil { + if verr := runSchemaValidation(data, schema, dest); verr != nil { + r.Logger.Printf("Initial output failed schema validation: %v", verr) + err = verr + data = nil + } + } if err == nil && data != nil { elapsed := int(time.Since(startTime).Milliseconds()) @@ -441,6 +499,13 @@ func (r *Runner) handleSchemaWithRetry( r.Logger.Printf("Schema retry %d succeeded via stdout fallback", retryNum+1) } } + if err == nil { + if verr := runSchemaValidation(data, schema, dest); verr != nil { + r.Logger.Printf("Schema retry %d failed validation: %v", retryNum+1, verr) + err = verr + data = nil + } + } if err == nil && data != nil { elapsed := int(time.Since(startTime).Milliseconds()) @@ -505,6 +570,18 @@ func fileExists(path string) bool { return err == nil } +// runSchemaValidation enforces the JSON Schema on parsed output when the normal +// harness contract holds — both a destination struct and a schema were +// provided. It returns nil (validation not applicable) when either is absent or +// the data is nil, preserving the previous unmarshal-only behavior for +// schema-only or dest-less callers. +func runSchemaValidation(data map[string]any, schema map[string]any, dest any) error { + if data == nil || schema == nil || dest == nil { + return nil + } + return validateAgainstSchema(data, schema) +} + // StructToJSONSchema converts a Go struct (or pointer to struct) to a basic // JSON Schema map by inspecting its JSON tags. This is a convenience for // callers who don't want to construct schemas manually. diff --git a/sdk/go/harness/runner_test.go b/sdk/go/harness/runner_test.go index e17469039..c5a6594d5 100644 --- a/sdk/go/harness/runner_test.go +++ b/sdk/go/harness/runner_test.go @@ -6,6 +6,7 @@ import ( "fmt" "os" "path/filepath" + "strings" "testing" "time" @@ -467,18 +468,36 @@ func TestCodexProvider_NonZeroExit(t *testing.T) { assert.Equal(t, FailureCrash, raw.FailureType) } -func TestCodexProvider_WithFullAuto(t *testing.T) { - dir := t.TempDir() - // Script that echoes its arguments so we can verify --full-auto is passed - script := writeTestScript(t, dir, "codex", "#!/bin/sh\necho \"args: $@\"\n") +// TestCodexProvider_AutoPermissionUsesBypassNotFullAuto verifies the deprecated +// --full-auto flag is gone and the "auto" permission mode maps to the bypass +// flag (codex_harness_patch.py:165-166). Argv is captured via an injected +// runCLI rather than echoed to stdout, because the provider no longer surfaces +// raw stdout as the result. +func TestCodexProvider_AutoPermissionUsesBypassNotFullAuto(t *testing.T) { + var gotCmd []string + var gotStdin []byte + p := NewCodexProvider("codex") + p.runCLI = func(_ context.Context, cmd []string, _ map[string]string, _ string, _ int, stdin []byte) (*CLIResult, error) { + gotCmd = cmd + gotStdin = stdin + return &CLIResult{Stdout: `{"type":"result","result":"done"}`, ReturnCode: 0}, nil + } - p := NewCodexProvider(script) raw, err := p.Execute(context.Background(), "test prompt", Options{ + Model: "gpt-5.5", PermissionMode: "auto", }) assert.NoError(t, err) assert.False(t, raw.IsError) - assert.Contains(t, raw.Result, "--full-auto") + + joined := strings.Join(gotCmd, " ") + assert.Contains(t, joined, "-m gpt-5.5") + assert.Contains(t, gotCmd, "--dangerously-bypass-approvals-and-sandbox") + assert.Contains(t, gotCmd, "--skip-git-repo-check") + assert.NotContains(t, gotCmd, "--full-auto") + // The prompt rides on stdin, never as a positional argv entry. + assert.Equal(t, "test prompt", string(gotStdin)) + assert.NotContains(t, gotCmd, "test prompt") } // --- Gemini provider tests --- diff --git a/sdk/go/harness/schema.go b/sdk/go/harness/schema.go index 2c0340c56..8a143bceb 100644 --- a/sdk/go/harness/schema.go +++ b/sdk/go/harness/schema.go @@ -1,6 +1,7 @@ package harness import ( + "bytes" "encoding/json" "fmt" "os" @@ -9,6 +10,8 @@ import ( "regexp" "sort" "strings" + + "github.com/santhosh-tekuri/jsonschema/v5" ) const ( @@ -32,6 +35,103 @@ func estimateTokens(text string) int { return len(text) / 4 } +// codexStrictJSONSchema rewrites a JSON schema into the strict form codex's +// --output-schema (OpenAI structured output) requires. On every object node it +// drops each property's "default", marks ALL properties required, and sets +// additionalProperties:false; it recurses into properties, array items, +// allOf/anyOf/oneOf branches, and $defs/definitions. Port of +// _codex_strict_json_schema (codex_harness_patch.py:23-65). +// +// The input schema is not mutated — every level is copied before edits. +func codexStrictJSONSchema(schema map[string]any) map[string]any { + strict := make(map[string]any, len(schema)) + for k, v := range schema { + strict[k] = v + } + + schemaType, _ := strict["type"].(string) + + if schemaType == "object" { + if props, ok := strict["properties"].(map[string]any); ok { + cleaned := make(map[string]any, len(props)) + keys := make([]string, 0, len(props)) + for key, value := range props { + keys = append(keys, key) + if child, ok := value.(map[string]any); ok { + childCopy := make(map[string]any, len(child)) + for ck, cv := range child { + if ck == "default" { + continue + } + childCopy[ck] = cv + } + cleaned[key] = codexStrictJSONSchema(childCopy) + } else { + cleaned[key] = value + } + } + // Sort for deterministic output (Go maps randomize iteration order; + // Python preserves dict insertion order). codex validation is + // order-independent, so this only stabilizes the emitted file. + sort.Strings(keys) + strict["properties"] = cleaned + strict["required"] = keys + strict["additionalProperties"] = false + } + } + + if schemaType == "array" { + if items, ok := strict["items"].(map[string]any); ok { + strict["items"] = codexStrictJSONSchema(items) + } + } + + for _, key := range []string{"allOf", "anyOf", "oneOf"} { + if branch, ok := strict[key].([]any); ok { + newBranch := make([]any, len(branch)) + for i, item := range branch { + if m, ok := item.(map[string]any); ok { + newBranch[i] = codexStrictJSONSchema(m) + } else { + newBranch[i] = item + } + } + strict[key] = newBranch + } + } + + for _, defKey := range []string{"$defs", "definitions"} { + if defs, ok := strict[defKey].(map[string]any); ok { + newDefs := make(map[string]any, len(defs)) + for key, value := range defs { + if m, ok := value.(map[string]any); ok { + newDefs[key] = codexStrictJSONSchema(m) + } else { + newDefs[key] = value + } + } + strict[defKey] = newDefs + } + } + + return strict +} + +// BuildCodexNativeSuffix constructs the prompt suffix for codex's native +// structured output. Unlike BuildPromptSuffix (which asks the model to Write a +// file), this tells the model to emit its final JSON answer directly — the +// codex CLI persists it to outputPath via --output-last-message and validates +// it against schemaPath via --output-schema. Port of the codex-native suffix in +// codex_harness_patch.py:141-148 + 179-186. +func BuildCodexNativeSuffix(schemaPath, outputPath string) string { + return "\n\n---\n" + + "CRITICAL CODEX STRUCTURED OUTPUT REQUIREMENTS:\n" + + fmt.Sprintf("Return a single final JSON object conforming to the schema at: %s\n", schemaPath) + + fmt.Sprintf("The Codex CLI will persist your final response to: %s\n", outputPath) + + "Return the JSON object as your final answer. Do not use markdown fences, comments, or surrounding prose.\n" + + "Do not try to create .agentfield_output.json yourself; the Codex CLI will persist your final JSON response for AgentField." +} + // BuildPromptSuffix constructs the OUTPUT REQUIREMENTS instruction that tells // the coding agent to write JSON to a deterministic file path. func BuildPromptSuffix(jsonSchema map[string]any, dir string) string { @@ -262,6 +362,79 @@ func unmarshalInto(data map[string]any, dest any) error { return json.Unmarshal(b, dest) } +// validateAgainstSchema validates a parsed JSON object against a JSON Schema +// map and returns a concise, prompt-friendly error when it does not conform. +// +// A JSON round-trip into a Go struct (unmarshalInto) accepts missing required +// fields, invalid enum values, and extra fields silently — so on its own it +// lets malformed output pass the harness. This compiles the schema and runs +// real JSON Schema validation so the runner's schema-retry loop fires for those +// cases. When the schema cannot be serialized or compiled, it returns nil (no +// regression versus the previous unmarshal-only behavior — validation is simply +// skipped rather than blocking). +func validateAgainstSchema(data map[string]any, schema map[string]any) error { + schemaBytes, err := json.Marshal(schema) + if err != nil { + return nil + } + compiler := jsonschema.NewCompiler() + if err := compiler.AddResource("mem://agentfield/schema.json", bytes.NewReader(schemaBytes)); err != nil { + return nil + } + compiled, err := compiler.Compile("mem://agentfield/schema.json") + if err != nil { + return nil + } + // Normalize the data through JSON so the validator sees canonical types + // (e.g. float64 for numbers, []any for arrays) regardless of how it was + // produced upstream. + dataBytes, err := json.Marshal(data) + if err != nil { + return nil + } + var normalized any + if err := json.Unmarshal(dataBytes, &normalized); err != nil { + return nil + } + if verr := compiled.Validate(normalized); verr != nil { + return fmt.Errorf("schema validation failed: %s", conciseSchemaError(verr)) + } + return nil +} + +// conciseSchemaError flattens a jsonschema ValidationError tree into a short, +// prompt-friendly string listing the most specific (leaf) failures with their +// instance locations. +func conciseSchemaError(err error) string { + ve, ok := err.(*jsonschema.ValidationError) + if !ok { + return truncate(err.Error(), 300) + } + + var leaves []string + var walk func(e *jsonschema.ValidationError) + walk = func(e *jsonschema.ValidationError) { + if len(e.Causes) == 0 { + loc := e.InstanceLocation + if loc == "" { + loc = "/" + } + leaves = append(leaves, fmt.Sprintf("%s: %s", loc, e.Message)) + return + } + for _, c := range e.Causes { + walk(c) + } + } + walk(ve) + + if len(leaves) == 0 { + return truncate(ve.Message, 300) + } + sort.Strings(leaves) + return truncate(strings.Join(leaves, "; "), 400) +} + // CleanupTempFiles removes harness temp files. // // For safety, this is a no-op when dir is empty or ".". diff --git a/sdk/go/harness/testdata/claude_stream_json_2.1.191.jsonl b/sdk/go/harness/testdata/claude_stream_json_2.1.191.jsonl new file mode 100644 index 000000000..508cec1c2 --- /dev/null +++ b/sdk/go/harness/testdata/claude_stream_json_2.1.191.jsonl @@ -0,0 +1,11 @@ +{"type":"system","subtype":"init","cwd":"/tmp/claude-stream-capture","session_id":"48a64026-56f9-4203-8220-1c099dd8378a","tools":["Task","Bash","Edit","Read","Write","WebFetch","WebSearch"],"mcp_servers":[],"model":"claude-haiku-4-5-20251001","permissionMode":"default","slash_commands":["compact","config","init","review"],"apiKeySource":"none","claude_code_version":"2.1.191","output_style":"default","agents":["general-purpose"],"skills":[],"plugins":[],"analytics_disabled":false,"product_feedback_disabled":false,"uuid":"093be5bc-c3c1-4c15-a987-32931c15cf8c","memory_paths":{"auto":"/home/user/.claude/projects/-tmp-claude-stream-capture/memory/"},"fast_mode_state":"off"} +{"type":"rate_limit_event","rate_limit_info":{"status":"allowed","resetsAt":1783637400,"rateLimitType":"five_hour","overageStatus":"rejected","overageDisabledReason":"org_level_disabled","isUsingOverage":false},"uuid":"fd5057fe-eddf-4c86-894e-c94ffcbb5cac","session_id":"48a64026-56f9-4203-8220-1c099dd8378a"} +{"type":"system","subtype":"thinking_tokens","estimated_tokens":3,"estimated_tokens_delta":3,"uuid":"b780f966-907e-49ef-91a6-e219d7b6b14a","session_id":"48a64026-56f9-4203-8220-1c099dd8378a"} +{"type":"system","subtype":"thinking_tokens","estimated_tokens":34,"estimated_tokens_delta":31,"uuid":"689c2271-a580-4511-9358-221f1760a104","session_id":"48a64026-56f9-4203-8220-1c099dd8378a"} +{"type":"system","subtype":"thinking_tokens","estimated_tokens":47,"estimated_tokens_delta":13,"uuid":"a33e7b09-abd3-481e-98e4-780e1df4177a","session_id":"48a64026-56f9-4203-8220-1c099dd8378a"} +{"type":"system","subtype":"thinking_tokens","estimated_tokens":66,"estimated_tokens_delta":19,"uuid":"e1635c75-4610-4ac7-bee9-f8c5516dc3e4","session_id":"48a64026-56f9-4203-8220-1c099dd8378a"} +{"type":"system","subtype":"thinking_tokens","estimated_tokens":71,"estimated_tokens_delta":5,"uuid":"aee5414e-4bc0-445b-b035-c76cee1da9b4","session_id":"48a64026-56f9-4203-8220-1c099dd8378a"} +{"type":"system","subtype":"thinking_tokens","estimated_tokens":133,"estimated_tokens_delta":62,"uuid":"cd60e718-28dc-460f-b539-37d2af06a1d5","session_id":"48a64026-56f9-4203-8220-1c099dd8378a"} +{"type":"assistant","message":{"model":"claude-haiku-4-5-20251001","id":"msg_011CcsFkPnSTKPbPn3BwpPgx","type":"message","role":"assistant","content":[{"type":"thinking","thinking":"The user is asking me to reply with just \"OK\". This is a simple acknowledgment message. They've provided a lot of context about their instructions, rules, and settings, and now they're asking me to confirm I've read/understood it.\n\nI should simply respond with \"OK\" as requested.","signature":"Eo4ECpMBCA8YAipA30vWEZusHgXOU3c1q0XdLMtLPWzSbl5slFx+mjaiQLlscaSO6pucWWgwoOc0IonZFxKqxwu3tRt3stnxQZVzgjIZY2xhdWRlLWhhaWt1LTQtNS0yMDI1MTAwMTgAQgh0aGlua2luZ1okZWMwNGY0YzAtZTg1Ny00NGUyLWJhYTctNTY0YmM3ZDMxMjA3EgzjsundiqL02SWA3RQaDACiFjgKdgpHE0on0SIwSeS/uMZr34q3eZNbmpuk8NNLX0eVRJSpJQETSednE9VSO1qSNruN91bd/ez/GODMKqcCaUJDu7MAV+DVAh+5rUMSQesLeKTbOZn0tY3Dql2WxiL3JuMce6vPfsAGvvgLje+iLQP7Li3LtBbSyW5HdcEq1ih4i07Cq5N22nUBVdMx8GfXJE8eTHptJgJIrT68O3IgPXIXyTaeQ+rZBxriS5kYJZ8ATlShcO0REheNjxMpYg+L6Vfq3KGFz6CPmkKIBoKgr8ffRCniJ22h7YEsO7HsQXflk6c5wvLUHfpwlcHHtPaM/EsGglKECuGixrpXn3eNp+Kqm1VX7k/QEBZVmaRlix1n1atrKYeWRlmIo/E5j77foRA4g2fhH9oZye4BY8U1x2Z9Tu+2gghx5oCi80Qsmidvmh8PMF61bZGwTa719RiILVoOyDE5Mq+U0XlooiW28LhSUbTe6xgB"}],"stop_reason":null,"stop_sequence":null,"stop_details":null,"usage":{"input_tokens":10,"cache_creation_input_tokens":15023,"cache_read_input_tokens":16491,"cache_creation":{"ephemeral_5m_input_tokens":0,"ephemeral_1h_input_tokens":15023},"output_tokens":3,"service_tier":"standard","inference_geo":"not_available"},"diagnostics":null,"context_management":null},"parent_tool_use_id":null,"session_id":"48a64026-56f9-4203-8220-1c099dd8378a","uuid":"5abc9419-7bda-49d9-ad12-a134d4d4d69e","request_id":"req_011CcsFkNqPavEi1KdjEMJRw"} +{"type":"assistant","message":{"model":"claude-haiku-4-5-20251001","id":"msg_011CcsFkPnSTKPbPn3BwpPgx","type":"message","role":"assistant","content":[{"type":"text","text":"OK"}],"stop_reason":null,"stop_sequence":null,"stop_details":null,"usage":{"input_tokens":10,"cache_creation_input_tokens":15023,"cache_read_input_tokens":16491,"cache_creation":{"ephemeral_5m_input_tokens":0,"ephemeral_1h_input_tokens":15023},"output_tokens":3,"service_tier":"standard","inference_geo":"not_available"},"diagnostics":null,"context_management":null},"parent_tool_use_id":null,"session_id":"48a64026-56f9-4203-8220-1c099dd8378a","uuid":"40791e35-0359-433d-80b5-03175d5a88c6","request_id":"req_011CcsFkNqPavEi1KdjEMJRw"} +{"type":"result","subtype":"success","is_error":false,"api_error_status":null,"duration_ms":2002,"duration_api_ms":2725,"ttft_ms":1929,"ttft_stream_ms":1071,"time_to_request_ms":76,"num_turns":1,"result":"OK","stop_reason":"end_turn","session_id":"48a64026-56f9-4203-8220-1c099dd8378a","total_cost_usd":0.0326441,"usage":{"input_tokens":10,"cache_creation_input_tokens":15023,"cache_read_input_tokens":16491,"output_tokens":75,"server_tool_use":{"web_search_requests":0,"web_fetch_requests":0},"service_tier":"standard","cache_creation":{"ephemeral_1h_input_tokens":15023,"ephemeral_5m_input_tokens":0},"inference_geo":"not_available","iterations":[{"input_tokens":10,"output_tokens":75,"cache_read_input_tokens":16491,"cache_creation_input_tokens":15023,"cache_creation":{"ephemeral_5m_input_tokens":0,"ephemeral_1h_input_tokens":15023},"type":"message"}],"speed":"standard"},"modelUsage":{"claude-haiku-4-5-20251001":{"inputTokens":514,"outputTokens":87,"cacheReadInputTokens":16491,"cacheCreationInputTokens":15023,"webSearchRequests":0,"costUSD":0.0326441,"contextWindow":200000,"maxOutputTokens":32000}},"permission_denials":[],"terminal_reason":"completed","fast_mode_state":"off","uuid":"de7a0e6f-3173-4bf9-a79e-7f5ff9c2193c"} diff --git a/sdk/go/harness/validation_test.go b/sdk/go/harness/validation_test.go new file mode 100644 index 000000000..6d6eabb3e --- /dev/null +++ b/sdk/go/harness/validation_test.go @@ -0,0 +1,255 @@ +package harness + +import ( + "context" + "os" + "testing" + "time" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +// sequenceWriterProvider writes a different output-file body on each Execute +// call (simulating a coding agent that fixes its output across retries). +type sequenceWriterProvider struct { + outputPath string + contents [][]byte + results []*RawResult + calls int +} + +func (m *sequenceWriterProvider) Execute(_ context.Context, _ string, _ Options) (*RawResult, error) { + idx := m.calls + m.calls++ + if idx < len(m.contents) && m.contents[idx] != nil { + _ = os.WriteFile(m.outputPath, m.contents[idx], 0o644) + } + if idx < len(m.results) { + return m.results[idx], nil + } + return &RawResult{Result: "done", Metrics: Metrics{NumTurns: 1}}, nil +} + +// TestValidation_MissingRequiredFieldTriggersRetry maps to the contract: +// "output missing a required field -> validateAgainstSchema error -> retry +// fires -> second attempt valid -> success". Without real validation the +// initial (missing-field) output would unmarshal cleanly and no retry would +// fire, so mock.calls==0 would be the (buggy) old behavior. +func TestValidation_MissingRequiredFieldTriggersRetry(t *testing.T) { + dir := t.TempDir() + outputPath := OutputPath(dir) + + // Initial attempt: valid JSON but missing the required "value" field. + require.NoError(t, os.WriteFile(outputPath, []byte(`{}`), 0o644)) + + schema := map[string]any{ + "type": "object", + "required": []any{"value"}, + "properties": map[string]any{ + "value": map[string]any{"type": "string"}, + }, + } + + provider := &sequenceWriterProvider{ + outputPath: outputPath, + // Retry writes a valid object. + contents: [][]byte{[]byte(`{"value":"fixed"}`)}, + results: []*RawResult{{Result: "retry", Metrics: Metrics{NumTurns: 1}}}, + } + + var dest struct { + Value string `json:"value"` + } + runner := NewRunner(Options{Provider: "opencode"}) + result := runner.handleSchemaWithRetry( + context.Background(), + &RawResult{Result: "initial", Metrics: Metrics{NumTurns: 1}}, + schema, &dest, dir, time.Now(), provider, + Options{Provider: "opencode", SchemaMaxRetries: 2}, "prompt", false, + ) + + assert.False(t, result.IsError, "expected success after retry, got: %s", result.ErrorMessage) + assert.Equal(t, "fixed", dest.Value) + assert.Equal(t, 1, provider.calls, "exactly one retry should have fired") +} + +// TestValidation_InvalidEnumTriggersRetry maps to the contract's invalid-enum +// case. +func TestValidation_InvalidEnumTriggersRetry(t *testing.T) { + dir := t.TempDir() + outputPath := OutputPath(dir) + + // Initial attempt: "severity" present but not one of the allowed values. + require.NoError(t, os.WriteFile(outputPath, []byte(`{"severity":"bogus"}`), 0o644)) + + schema := map[string]any{ + "type": "object", + "required": []any{"severity"}, + "properties": map[string]any{ + "severity": map[string]any{ + "type": "string", + "enum": []any{"low", "high"}, + }, + }, + } + + provider := &sequenceWriterProvider{ + outputPath: outputPath, + contents: [][]byte{[]byte(`{"severity":"high"}`)}, + results: []*RawResult{{Result: "retry", Metrics: Metrics{NumTurns: 1}}}, + } + + var dest struct { + Severity string `json:"severity"` + } + runner := NewRunner(Options{Provider: "opencode"}) + result := runner.handleSchemaWithRetry( + context.Background(), + &RawResult{Result: "initial", Metrics: Metrics{NumTurns: 1}}, + schema, &dest, dir, time.Now(), provider, + Options{Provider: "opencode", SchemaMaxRetries: 2}, "prompt", false, + ) + + assert.False(t, result.IsError, "expected success after enum retry, got: %s", result.ErrorMessage) + assert.Equal(t, "high", dest.Severity) + assert.Equal(t, 1, provider.calls) +} + +// TestValidation_ExtraFieldsAllowedWhenAdditionalPropertiesUnset maps to the +// contract: "valid-but-extra-fields output passes when schema allows +// additionalProperties". No retry should fire. +func TestValidation_ExtraFieldsAllowedWhenAdditionalPropertiesUnset(t *testing.T) { + dir := t.TempDir() + outputPath := OutputPath(dir) + + require.NoError(t, os.WriteFile(outputPath, []byte(`{"a":"x","extra":"y"}`), 0o644)) + + schema := map[string]any{ + "type": "object", + "properties": map[string]any{ + "a": map[string]any{"type": "string"}, + }, + } + + provider := &sequenceWriterProvider{outputPath: outputPath} + + var dest struct { + A string `json:"a"` + } + runner := NewRunner(Options{Provider: "opencode"}) + result := runner.handleSchemaWithRetry( + context.Background(), + &RawResult{Result: "initial", Metrics: Metrics{NumTurns: 1}}, + schema, &dest, dir, time.Now(), provider, + Options{Provider: "opencode", SchemaMaxRetries: 2}, "prompt", false, + ) + + assert.False(t, result.IsError) + assert.Equal(t, "x", dest.A) + assert.Equal(t, 0, provider.calls, "valid output must not trigger a retry") +} + +// TestValidation_RetriesExhaustedSurfacesError maps to the contract: "retries +// exhausted -> error surfaces as before". +func TestValidation_RetriesExhaustedSurfacesError(t *testing.T) { + dir := t.TempDir() + outputPath := OutputPath(dir) + + require.NoError(t, os.WriteFile(outputPath, []byte(`{}`), 0o644)) + + schema := map[string]any{ + "type": "object", + "required": []any{"value"}, + "properties": map[string]any{ + "value": map[string]any{"type": "string"}, + }, + } + + // Every retry keeps writing an invalid (missing-field) object. + provider := &sequenceWriterProvider{ + outputPath: outputPath, + contents: [][]byte{[]byte(`{}`), []byte(`{}`)}, + results: []*RawResult{ + {Result: "r1", Metrics: Metrics{NumTurns: 1}}, + {Result: "r2", Metrics: Metrics{NumTurns: 1}}, + }, + } + + var dest struct { + Value string `json:"value"` + } + runner := NewRunner(Options{Provider: "opencode"}) + result := runner.handleSchemaWithRetry( + context.Background(), + &RawResult{Result: "initial", Metrics: Metrics{NumTurns: 1}}, + schema, &dest, dir, time.Now(), provider, + Options{Provider: "opencode", SchemaMaxRetries: 2}, "prompt", false, + ) + + assert.True(t, result.IsError) + assert.Equal(t, FailureSchema, result.FailureType) + assert.Contains(t, result.ErrorMessage, "Schema validation failed") + assert.Equal(t, 2, provider.calls, "both retries should have been attempted") +} + +// TestValidateAgainstSchema unit-tests the validator directly. +func TestValidateAgainstSchema(t *testing.T) { + schema := map[string]any{ + "type": "object", + "required": []any{"name", "level"}, + "properties": map[string]any{ + "name": map[string]any{"type": "string"}, + "level": map[string]any{"type": "string", "enum": []any{"low", "high"}}, + }, + "additionalProperties": false, + } + + t.Run("valid", func(t *testing.T) { + assert.NoError(t, validateAgainstSchema(map[string]any{"name": "x", "level": "high"}, schema)) + }) + t.Run("missing required", func(t *testing.T) { + err := validateAgainstSchema(map[string]any{"name": "x"}, schema) + require.Error(t, err) + assert.Contains(t, err.Error(), "schema validation failed") + }) + t.Run("invalid enum", func(t *testing.T) { + err := validateAgainstSchema(map[string]any{"name": "x", "level": "nope"}, schema) + require.Error(t, err) + }) + t.Run("extra field rejected when additionalProperties false", func(t *testing.T) { + err := validateAgainstSchema(map[string]any{"name": "x", "level": "low", "junk": 1}, schema) + require.Error(t, err) + }) + t.Run("uncompilable schema skips validation", func(t *testing.T) { + // A schema whose "type" is not a valid keyword value fails to compile; + // validation is skipped (nil) rather than blocking — no regression. + bad := map[string]any{"type": 123} + assert.NoError(t, validateAgainstSchema(map[string]any{"any": "thing"}, bad)) + }) +} + +// TestRunSchemaValidation_Gating verifies validation only applies when both a +// destination and a schema are present. +func TestRunSchemaValidation_Gating(t *testing.T) { + schema := map[string]any{ + "type": "object", + "required": []any{"value"}, + "properties": map[string]any{ + "value": map[string]any{"type": "string"}, + }, + } + invalid := map[string]any{} // missing required "value" + + var dest struct { + Value string `json:"value"` + } + // Both present -> validation runs and fails. + assert.Error(t, runSchemaValidation(invalid, schema, &dest)) + // No dest -> skipped. + assert.NoError(t, runSchemaValidation(invalid, schema, nil)) + // No schema -> skipped. + assert.NoError(t, runSchemaValidation(invalid, nil, &dest)) + // No data -> skipped. + assert.NoError(t, runSchemaValidation(nil, schema, &dest)) +}