From d72f89bed5e899c1a5c72c1e099a57320800a6ec Mon Sep 17 00:00:00 2001 From: Abir Abbas Date: Fri, 10 Jul 2026 09:44:49 -0400 Subject: [PATCH 01/13] feat(control-plane): af install --path subdirectory selector Add a --path flag to `af install` so a single repository can ship more than one installable agent node (e.g. a Python node at the root and a Go port under go/). By default `af install` finds the first agentfield-package.yaml root-first; with --path it installs the node whose manifest lives at //agentfield-package.yaml, and that subtree becomes the package root that is copied to ~/.agentfield/packages/. - domain.InstallOptions gains Path (top-level source only; never propagated to recursively-installed node dependencies). - packages.ResolvePackageSubdir / ValidateSubdirSelector: shared, reusable resolution+validation. Reject absolute paths and paths that escape the source root via ".." (validated before any clone/copy work); a missing manifest is reported with the full expected path. - GitInstaller gains a Subdir field and resolvePackageRoot: no selector keeps the historical root-first walk; a selector resolves+validates the subdir post-clone. Composes with the existing @ref URL pin (parsed independently). - Local-directory installs honor --path with identical semantics (/), resolved before any copy/registry mutation. - CLI help/examples document --path. Co-Authored-By: Claude Fable 5 --- .../internal/cli/commands/install.go | 18 +++++- control-plane/internal/core/domain/models.go | 6 ++ .../core/services/coverage_targeted_test.go | 4 +- .../internal/core/services/package_service.go | 26 ++++++-- control-plane/internal/packages/git.go | 34 ++++++++++- control-plane/internal/packages/installer.go | 60 +++++++++++++++++++ 6 files changed, 139 insertions(+), 9 deletions(-) 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/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/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") From e83bcfbbf98a21bf8a1aa7bf6e81c9f9ea0c9c26 Mon Sep 17 00:00:00 2001 From: Abir Abbas Date: Fri, 10 Jul 2026 09:45:02 -0400 Subject: [PATCH 02/13] test(control-plane): cover af install --path selector MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Behavior-driven tests derived from the --path validation contract: - packages: ResolvePackageSubdir / ValidateSubdirSelector (root vs subdir selection, in-tree traversal allowed, absolute + escaping paths rejected, missing manifest names the expected path); GitInstaller.resolvePackageRoot against a faked clone (root install unchanged, --path go selects the go subtree, missing/absolute/escaping paths rejected — the last also proving InstallFromGit rejects a bad selector before any clone); @ref composes with --path; a Go subdir package builds relative to the subdir-as-root. - services: local-directory `af install --path ` selects the subdir node, bare install installs the root node, a missing/absolute/escaping --path errors and leaves the registry unmutated. Co-Authored-By: Claude Fable 5 --- .../core/services/install_path_test.go | 116 +++++++++ .../internal/packages/install_path_test.go | 233 ++++++++++++++++++ 2 files changed, 349 insertions(+) create mode 100644 control-plane/internal/core/services/install_path_test.go create mode 100644 control-plane/internal/packages/install_path_test.go 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/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") + } +} From 1f0609cfe52811f2fd550f0705151244eb9d9ef6 Mon Sep 17 00:00:00 2001 From: Abir Abbas Date: Fri, 10 Jul 2026 09:45:07 -0400 Subject: [PATCH 03/13] docs: document af install --path for multi-node repositories Add a "One repo, many nodes: --path" section to the agent-node install guide covering git and local sources, @ref composition, the relative-path/escape rules, and that a bare install is unchanged. Add a --path row to the lifecycle reference table. Co-Authored-By: Claude Fable 5 --- docs/installing-agent-nodes.md | 28 ++++++++++++++++++++++++++++ 1 file changed, 28 insertions(+) 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 | From 3105330c33f2677616b4fc50670b87a0be6b1cd7 Mon Sep 17 00:00:00 2001 From: Abir Abbas Date: Thu, 9 Jul 2026 18:05:13 -0400 Subject: [PATCH 04/13] fix(sdk/go): deliver claude prompt via stdin; correct note endpoint path The claude provider appended the prompt as a trailing positional after repeated --allowedTools flags; claude CLI 2.1.x treats --allowedTools as variadic and swallows the positional, so every tool-passing call exited 1 with 'Input must be provided'. The prompt now flows through stdin (RunCLIWithStdin), which --print reads natively; codex/opencode/ gemini were inspected and are unaffected. Verified against the real CLI via a gated integration test. Note() posted to {base}/executions/note without the /api/v1 prefix, 404ing on the control plane (route registered under the /api/v1 group) and silently dropping every progress note; existing tests masked it by baking /api/v1 into the base URL, contrary to the SDK-wide bare-base contract. Co-Authored-By: Claude Fable 5 (cherry picked from commit 7c6f11e1c89a55e4a9959901b6b866d482519c3a) --- sdk/go/agent/note.go | 7 +- sdk/go/agent/note_test.go | 39 +++++---- sdk/go/harness/claudecode.go | 21 ++++- sdk/go/harness/claudecode_integration_test.go | 52 ++++++++++++ sdk/go/harness/claudecode_test.go | 80 +++++++++++++++++++ sdk/go/harness/cli.go | 28 +++++-- 6 files changed, 199 insertions(+), 28 deletions(-) create mode 100644 sdk/go/harness/claudecode_integration_test.go create mode 100644 sdk/go/harness/claudecode_test.go 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/harness/claudecode.go b/sdk/go/harness/claudecode.go index debd5f96b..198cb927b 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. @@ -64,8 +68,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 +89,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 { diff --git a/sdk/go/harness/claudecode_integration_test.go b/sdk/go/harness/claudecode_integration_test.go new file mode 100644 index 000000000..7c547ec95 --- /dev/null +++ b/sdk/go/harness/claudecode_integration_test.go @@ -0,0 +1,52 @@ +//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. +// +// 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) + + assert.False(t, raw.IsError, "expected no error, got: %s", raw.ErrorMessage) + assert.Contains(t, raw.Result, "HELLO_AGENTFIELD") +} diff --git a/sdk/go/harness/claudecode_test.go b/sdk/go/harness/claudecode_test.go new file mode 100644 index 000000000..95f760262 --- /dev/null +++ b/sdk/go/harness/claudecode_test.go @@ -0,0 +1,80 @@ +package harness + +import ( + "context" + "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") +} + +// 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, "json", gotCmd[len(gotCmd)-1], "vector ends at --output-format json, no positional prompt") +} diff --git a/sdk/go/harness/cli.go b/sdk/go/harness/cli.go index 3c5f864a5..00d3bdd5f 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,9 +108,11 @@ 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. From 70511be4f77811cb4314ac184877bca981737dbc Mon Sep 17 00:00:00 2001 From: Abir Abbas Date: Thu, 9 Jul 2026 18:32:58 -0400 Subject: [PATCH 05/13] fix(sdk/go): async-submit Agent.Call and stream-json claude output Agent.Call was a synchronous POST /execute/{target} bounded by the 15s CallTimeout http.Client default, killing any caller whose child reasoner worked longer. It now mirrors the Python SDK: submit via /execute/async/{target}, then poll the execution record with Python-parity pacing (0.25s doubling to 4s, jittered), CallTimeout bounding each request rather than the overall wait, ctx-cancellable, lineage headers on submit and polls so DAG parentage is unchanged. The claude provider now uses --output-format stream-json --verbose: per-event output keeps the CLI idle watchdog fed during long silent turns (plain json emitted nothing until completion, so >120s turns were SIGKILLed), and the terminal result event carries the same fields including total_cost_usd. Real 2.1.191 capture committed as a fixture; ctx cancellation now kills the whole CLI process group. Co-Authored-By: Claude Fable 5 (cherry picked from commit 18c3f3eaf5aab18667c08e23c5836b38ff739e36) --- sdk/go/agent/agent.go | 313 +++++++++++++++--- sdk/go/agent/agent_test.go | 246 ++++++++++++-- sdk/go/harness/claudecode.go | 25 +- sdk/go/harness/claudecode_integration_test.go | 19 ++ sdk/go/harness/claudecode_test.go | 69 +++- sdk/go/harness/cli.go | 10 + .../testdata/claude_stream_json_2.1.191.jsonl | 11 + 7 files changed, 609 insertions(+), 84 deletions(-) create mode 100644 sdk/go/harness/testdata/claude_stream_json_2.1.191.jsonl diff --git a/sdk/go/agent/agent.go b/sdk/go/agent/agent.go index 872b64aa8..5e116c308 100644 --- a/sdk/go/agent/agent.go +++ b/sdk/go/agent/agent.go @@ -365,11 +365,13 @@ 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 as a client - memory backend requests, and for cross-agent + // Call()s the async submit plus each status poll. It does NOT bound a + // Call() end-to-end: Call submits to the async execute endpoint and + // polls until the child execution finishes, so a child reasoner may run + // arbitrarily longer than CallTimeout (bound the overall wait with the + // ctx passed to Call instead). Optional. Default: 15s. CallTimeout time.Duration // DisableLeaseLoop disables automatic periodic lease refreshes. @@ -1565,7 +1567,45 @@ 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 +) + +// 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: Config.CallTimeout bounds each individual HTTP request +// (the submit and every poll), NOT the end-to-end 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 +1631,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,6 +1678,30 @@ 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) } +} + +// 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, "/")) + req, err := http.NewRequestWithContext(ctx, http.MethodPost, url, bytes.NewReader(body)) + 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 "", "", fmt.Errorf("build request: %w", err) + } + req.Header.Set("Content-Type", "application/json") + a.applyCallHeaders(req, execCtx, runID) // Sign request with DID auth headers if configured if a.client != nil { @@ -1640,16 +1714,16 @@ func (a *Agent) Call(ctx context.Context, target string, input map[string]any) ( "target": target, "error": err.Error(), }) - return nil, fmt.Errorf("perform execute call: %w", err) + return "", "", fmt.Errorf("perform execute call: %w", err) } 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 +1735,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 +1745,192 @@ 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") } - 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, - }) - return nil, &ExecuteError{ - StatusCode: resp.StatusCode, - Message: fmt.Sprintf("execute status %s", execResp.Status), - ErrorDetails: execResp.ErrorDetails, + + return submitResp.ExecutionID, submitResp.RunID, nil +} + +// awaitExecutionResult polls GET /api/v1/executions/{execution_id} until the +// execution reaches a terminal status. Mirrors Python _await_execution_sync +// (client.py:970-1011): statuses are normalized, "succeeded" returns the +// result, {failed, cancelled, timeout} surface the execution error (with the +// status endpoint's "error" field coalesced into the error message), and any +// other status keeps polling with jittered exponential backoff. The overall +// wait is unbounded — only ctx cancellation stops it — while each poll request +// is individually bounded by the HTTP client's CallTimeout. +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 + + 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 } - } - 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 + 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.httpClient.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.logExecutionError(ctx, "call.outbound.failed", "cross-node call status poll failed", map[string]any{ + "target": target, + "execution_id": executionID, + "error": err.Error(), + }) + return nil, fmt.Errorf("poll execution status: %w", err) + } + + bodyBytes, readErr := io.ReadAll(resp.Body) + resp.Body.Close() + if readErr != nil { + return nil, fmt.Errorf("read execution status: %w", readErr) + } + + // Python's poll uses raise_for_status(): any HTTP error aborts the wait. + 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))), + } + } + + 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_test.go b/sdk/go/agent/agent_test.go index 160258040..403c88b08 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/harness/claudecode.go b/sdk/go/harness/claudecode.go index 198cb927b..c15fda638 100644 --- a/sdk/go/harness/claudecode.go +++ b/sdk/go/harness/claudecode.go @@ -34,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) @@ -122,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, @@ -157,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_integration_test.go b/sdk/go/harness/claudecode_integration_test.go index 7c547ec95..04de4e832 100644 --- a/sdk/go/harness/claudecode_integration_test.go +++ b/sdk/go/harness/claudecode_integration_test.go @@ -19,6 +19,12 @@ import ( // "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: @@ -46,7 +52,20 @@ func TestClaudeCodeProvider_Integration_StdinWithTools(t *testing.T) { 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 index 95f760262..dad45b271 100644 --- a/sdk/go/harness/claudecode_test.go +++ b/sdk/go/harness/claudecode_test.go @@ -2,6 +2,8 @@ package harness import ( "context" + "os" + "path/filepath" "testing" "github.com/stretchr/testify/assert" @@ -54,6 +56,20 @@ func TestClaudeCodeProvider_PromptDeliveredViaStdin(t *testing.T) { // 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 @@ -76,5 +92,56 @@ func TestClaudeCodeProvider_EmptyToolsStillStdin(t *testing.T) { assert.Equal(t, prompt, string(gotStdin)) assert.NotContains(t, gotCmd, prompt) require.NotEmpty(t, gotCmd) - assert.Equal(t, "json", gotCmd[len(gotCmd)-1], "vector ends at --output-format json, no positional prompt") + 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 00d3bdd5f..d24ba2251 100644 --- a/sdk/go/harness/cli.go +++ b/sdk/go/harness/cli.go @@ -118,6 +118,16 @@ func RunCLIWithStdin(ctx context.Context, cmd []string, env map[string]string, c // 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/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"} From 431f0bcd41b13f6919d81463f1c587387e3c2180 Mon Sep 17 00:00:00 2001 From: Abir Abbas Date: Fri, 10 Jul 2026 09:37:44 -0400 Subject: [PATCH 06/13] test(sdk/go): cover async Call submit/poll paths and claudecode default-runner fallback Adds tests closing the patch-coverage gap on PR #746's async-submit Agent.Call rewrite and the claudecode stdin/stream-json changes. agent_call_coverage_test.go (package agent) exercises the error/edge branches of Call's submit + poll loop that the happy-path tests miss: - nextCallPollInterval clamps (below floor / above ceiling) - submitAsyncExecution: build-request, transport, read-body, structured 4xx, and JSON-decode errors - awaitExecutionResult: ctx-already-cancelled, GET build error, ctx-cancel-during-poll, transport error, read-body error, poll HTTP-error status, poll decode error, succeeded-result decode error, and succeeded-null-result claudecode_defaultrunner_test.go covers the runCLI == nil fallback in ClaudeCodeProvider.Execute via a struct-literal provider (bypassing NewClaudeCodeProvider) driving the real default runner. Lifts sdk-go patch coverage from 61% to 98% (agent.go 98.29%, claudecode.go 100%), clearing the 80% min_patch gate. Tests only; no production code changed. Co-Authored-By: Claude Fable 5 (cherry picked from commit 5b763d872751309c0b72ebd90f48626cfbdf9692) --- sdk/go/agent/agent_call_coverage_test.go | 359 ++++++++++++++++++ .../harness/claudecode_defaultrunner_test.go | 38 ++ 2 files changed, 397 insertions(+) create mode 100644 sdk/go/agent/agent_call_coverage_test.go create mode 100644 sdk/go/harness/claudecode_defaultrunner_test.go 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..7065d31ef --- /dev/null +++ b/sdk/go/agent/agent_call_coverage_test.go @@ -0,0 +1,359 @@ +package agent + +import ( + "context" + "encoding/json" + "errors" + "io" + "log" + "net/http" + "net/http/httptest" + "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: a transport failure on the async submit surfaces a +// "perform execute call" error. +func TestSubmitAsyncExecution_TransportError(t *testing.T) { + a := newCallTestAgent(t, "http://placeholder.invalid") + a.httpClient = &http.Client{Transport: roundTripperFunc(func(*http.Request) (*http.Response, error) { + return nil, errors.New("dial tcp: connection refused") + })} + + _, _, 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") +} + +// 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.httpClient = &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.httpClient = &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 transport failure on a status poll (with a live context) +// surfaces a "poll execution status" error. +func TestAwaitExecutionResult_TransportError(t *testing.T) { + a := newCallTestAgent(t, "http://placeholder.invalid") + a.httpClient = &http.Client{Transport: roundTripperFunc(func(*http.Request) (*http.Response, error) { + return nil, errors.New("connection reset by peer") + })} + + 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(), "poll execution status") +} + +// 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.httpClient = &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: an HTTP error status on a status poll aborts the wait with an +// *ExecuteError carrying that status (Python raise_for_status parity). +func TestCall_PollReturnsErrorStatus(t *testing.T) { + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + switch { + case r.Method == 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.StatusInternalServerError) + _, _ = w.Write([]byte("upstream boom")) + } + })) + 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.StatusInternalServerError, 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 { + case r.Method == 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 { + case r.Method == 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 { + case r.Method == 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) +} 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) +} From b4027777a77e86d012bbc1ea9d396609dc5c7344 Mon Sep 17 00:00:00 2001 From: Abir Abbas Date: Fri, 10 Jul 2026 11:05:49 -0400 Subject: [PATCH 07/13] fix(sdk/go): codex native structured output + model/sandbox parity Port the codex harness patch behaviors into the Go codex provider so it matches the Python reference (runtime/codex_harness_patch.py): - Pass `-m ` (was ignored entirely; SWE-AF resolves gpt-5.5 vs gpt-5.3-codex by auth mode and the value must reach the CLI). - Add `--skip-git-repo-check` so the harness runs outside a git repo. - Replace deprecated `--full-auto` with the permission->sandbox mapping: "auto" -> --dangerously-bypass-approvals-and-sandbox; read-only / workspace-write / danger-full-access -> --sandbox ; else --sandbox workspace-write. - Deliver the prompt via stdin (not argv), mirroring the claude provider. - Native structured output: when a schema is set, pass --output-schema (pointing at the STRICT rewrite) + --output-last-message, and read the last-message file when stdout yields no parseable final text. Schema plumbing uses a schemaAware interface the runner detects: the runner owns the strict-schema rewrite (codexStrictJSONSchema, ported from _codex_strict_json_schema), writes it, hands the provider the deterministic paths, and swaps in a codex-native prompt suffix while claude/opencode keep the Write-tool suffix. Options gains no schema field. Co-Authored-By: Claude Fable 5 --- sdk/go/harness/codex.go | 100 +++++++++-- sdk/go/harness/codex_test.go | 308 ++++++++++++++++++++++++++++++++++ sdk/go/harness/runner.go | 52 +++++- sdk/go/harness/runner_test.go | 31 +++- sdk/go/harness/schema.go | 97 +++++++++++ 5 files changed, 565 insertions(+), 23 deletions(-) create mode 100644 sdk/go/harness/codex_test.go 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/runner.go b/sdk/go/harness/runner.go index 9afa3b322..fe45ab13b 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) } } 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..1dc339a07 100644 --- a/sdk/go/harness/schema.go +++ b/sdk/go/harness/schema.go @@ -32,6 +32,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 { From 76b555d76c8ac7a6bf808cc4b55ce322f7b0dcca Mon Sep 17 00:00:00 2001 From: Abir Abbas Date: Fri, 10 Jul 2026 11:10:04 -0400 Subject: [PATCH 08/13] fix(sdk/go): enforce JSON Schema in the harness retry loop MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ParseAndValidate was unmarshal-only, so output missing required fields, carrying invalid enum values, or (with additionalProperties:false) extra fields passed silently and the schema-retry loop never fired. Add validateAgainstSchema (github.com/santhosh-tekuri/jsonschema/v5) and run it at every parse-success point in handleSchemaWithRetry — after the initial ParseAndValidate/TryParseFromText and inside each retry iteration — so a validation failure sets err and the EXISTING retry branch fires (schemaMaxRetries default 2 preserved). Validation applies only when both a destination struct and a schema are provided; uncompilable schemas skip validation (return nil) so there is no regression versus unmarshal-only. This makes map-schema harness calls stricter than before; callers control strictness through the schema they pass. Co-Authored-By: Claude Fable 5 --- sdk/go/go.mod | 1 + sdk/go/go.sum | 2 + sdk/go/harness/runner.go | 29 ++++ sdk/go/harness/schema.go | 76 +++++++++ sdk/go/harness/validation_test.go | 255 ++++++++++++++++++++++++++++++ 5 files changed, 363 insertions(+) create mode 100644 sdk/go/harness/validation_test.go 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/runner.go b/sdk/go/harness/runner.go index fe45ab13b..812f5a969 100644 --- a/sdk/go/harness/runner.go +++ b/sdk/go/harness/runner.go @@ -367,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()) @@ -489,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()) @@ -553,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/schema.go b/sdk/go/harness/schema.go index 1dc339a07..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 ( @@ -359,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/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)) +} From b8514d3aacdffd13cc3d0f15c1c5a6838d19c0bf Mon Sep 17 00:00:00 2001 From: Abir Abbas Date: Fri, 10 Jul 2026 11:15:18 -0400 Subject: [PATCH 09/13] fix(sdk/go): opencode JSON event stream + exit-0 error surfacing MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Port the Python opencode provider behaviors (providers/opencode.py): - Add `--format json` and parse the JSONL event stream: recover the final assistant text (extract_final_text parity), sum per-step cost from step_finish events (nil when none report a cost, distinguishing "unknown" from "$0.00"), and count turns as one per step_start, falling back to tool_use events. - Surface in-band JSON "error" events and — critically — hard failures on which opencode exits 0 (Model not found, AuthenticationError, Unauthorized, APIError): when stderr matches one of those markers and no result was produced, mark IsError with the extracted error window instead of returning empty output that reads downstream as "no result". Non-JSON stdout still falls back to trimmed raw text, so older opencode versions keep working. Co-Authored-By: Claude Fable 5 --- sdk/go/harness/opencode.go | 254 ++++++++++++++++++++++++++- sdk/go/harness/opencode_json_test.go | 225 ++++++++++++++++++++++++ 2 files changed, 470 insertions(+), 9 deletions(-) create mode 100644 sdk/go/harness/opencode_json_test.go 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) +} From 45311737c29881017523068d97e102988647f252 Mon Sep 17 00:00:00 2001 From: Abir Abbas Date: Fri, 10 Jul 2026 11:20:01 -0400 Subject: [PATCH 10/13] fix(sdk/go): preserve ReasonerFailed result on failed executions MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Add a native ReasonerFailed error type (Message / Result / ErrorDetails) so a reasoner that ran but failed can report status=failed WITHOUT discarding its structured outcome — mirroring the Python SDK's ReasonerFailed exception. In executeReasonerAsync's error branch, detect it via errors.As and attach payload["result"] / payload["error_details"] (only when non-nil) so the single 5x-retried status post carries the result atomically; the control plane stores the result regardless of terminal status. The two sync HTTP paths (handleExecute / handleReasoner) mirror this by carrying the result/details onto their error response. A plain error synthesizes neither key. Co-Authored-By: Claude Fable 5 --- sdk/go/agent/agent.go | 44 ++++++ sdk/go/agent/reasoner_failed.go | 38 +++++ sdk/go/agent/reasoner_failed_test.go | 224 +++++++++++++++++++++++++++ 3 files changed, 306 insertions(+) create mode 100644 sdk/go/agent/reasoner_failed.go create mode 100644 sdk/go/agent/reasoner_failed_test.go diff --git a/sdk/go/agent/agent.go b/sdk/go/agent/agent.go index 5e116c308..5ef3f3301 100644 --- a/sdk/go/agent/agent.go +++ b/sdk/go/agent/agent.go @@ -1171,6 +1171,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 } @@ -1374,6 +1389,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(), @@ -1486,6 +1515,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", 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) +} From 0ce29614fa7cdcf4dbb8188c7931ecbce7c05018 Mon Sep 17 00:00:00 2001 From: Abir Abbas Date: Fri, 10 Jul 2026 15:49:32 -0400 Subject: [PATCH 11/13] fix(sdk/go): make Agent.Call resilient to transient control-plane outages MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The async submit+poll Agent.Call path (introduced earlier on this branch) failed a call the instant a single HTTP request to the control plane failed. Real-world testing exposed the impact: two ~hour-long SWE-AF builds died at ~55 minutes when the CP briefly went slow under system load — one timed-out status poll ("context deadline exceeded while awaiting headers") aborted calls that had been running for 30+ minutes, and a slow submit POST likewise killed a call before the request could be confirmed accepted. Poll resilience: a transient poll failure (transport error/timeout, 408/429, or 5xx) no longer fails the call. Consecutive failures are retried with exponential backoff (capped at 15s) for a configurable window of UNBROKEN failure — default 5 minutes — after which the call fails with a clear "control plane unreachable for Xs" error instead of the raw first error. A single successful poll resets the window. A 404 on a just-submitted execution (the CP can lag before the row is queryable) is retried within a shorter bounded window, then fails with a distinct not-found message. Permanent 4xx (auth, bad request) still abort immediately. Submit safety: re-POSTing execute/async is NOT idempotent (a duplicate would double-run a coder), so the submit is retried ONLY on errors that prove the request never reached the server — dial/DNS/connection-refused, detected by inspecting the error chain (syscall.ECONNREFUSED, net.DNSError, dial-phase net.OpError). Ambiguous failures (a timeout awaiting headers — the request may already have been accepted) are never blind-retried; instead the submit client timeout is raised substantially (default 120s) so a slow-but-healthy CP does not abort an accepted request, and the call fails with a clear message if it still times out. Config: three env knobs following the AGENTFIELD_* integer-seconds convention, read once and cached on the agent — AGENTFIELD_CALL_POLL_TIMEOUT_SECONDS (60), AGENTFIELD_CALL_RETRY_WINDOW_SECONDS (300), AGENTFIELD_CALL_SUBMIT_TIMEOUT_SECONDS (120) — with dedicated submit/poll HTTP clients so these timeouts don't affect other client traffic. Each retried failure is logged at warn via the existing structured-log seam (call.outbound.submit_retry / call.outbound.poll_retry) with attempt count and elapsed window so operators see degradation without the call dying. The Python SDK's async Call path (_submit_execution_sync / _await_execution_sync) uses bare requests with raise_for_status() and no transient retry, so it shares this fragility; this change improves on it. Co-Authored-By: Claude Fable 5 --- sdk/go/agent/agent.go | 440 ++++++++++++++++++++++++++++++++++++++---- 1 file changed, 399 insertions(+), 41 deletions(-) diff --git a/sdk/go/agent/agent.go b/sdk/go/agent/agent.go index 5ef3f3301..43695e8a7 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" @@ -476,11 +479,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 @@ -561,6 +577,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 @@ -574,6 +601,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), @@ -1623,6 +1653,135 @@ const ( 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 { @@ -1736,29 +1895,85 @@ func (a *Agent) submitAsyncExecution( runID string, ) (executionID, submittedRunID string, err error) { url := fmt.Sprintf("%s/api/v1/execute/async/%s", base, strings.TrimPrefix(target, "/")) - req, err := http.NewRequestWithContext(ctx, http.MethodPost, url, bytes.NewReader(body)) - 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 "", "", fmt.Errorf("build request: %w", err) - } - req.Header.Set("Content-Type", "application/json") - a.applyCallHeaders(req, execCtx, runID) - // Sign request with DID auth headers if configured - if a.client != nil { - a.client.SignHTTPRequest(req, body) - } + 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 "", "", fmt.Errorf("perform execute call: %w", err) + if !sleepCtx(ctx, backoff) { + return "", "", ctx.Err() + } + backoff = nextRetryBackoff(backoff) } defer resp.Body.Close() @@ -1818,13 +2033,23 @@ func (a *Agent) submitAsyncExecution( } // awaitExecutionResult polls GET /api/v1/executions/{execution_id} until the -// execution reaches a terminal status. Mirrors Python _await_execution_sync -// (client.py:970-1011): statuses are normalized, "succeeded" returns the -// result, {failed, cancelled, timeout} surface the execution error (with the -// status endpoint's "error" field coalesced into the error message), and any -// other status keeps polling with jittered exponential backoff. The overall -// wait is unbounded — only ctx cancellation stops it — while each poll request -// is individually bounded by the HTTP client's CallTimeout. +// 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, @@ -1833,6 +2058,102 @@ func (a *Agent) awaitExecutionResult( 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 + } + + // 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 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 + } + for { if err := ctx.Err(); err != nil { a.logExecutionError(ctx, "call.outbound.failed", "cross-node call wait cancelled", map[string]any{ @@ -1852,7 +2173,7 @@ func (a *Agent) awaitExecutionResult( a.client.SignHTTPRequest(req, nil) } - resp, err := a.httpClient.Do(req) + resp, err := a.callPollClient.Do(req) if err != nil { // Distinguish caller cancellation from transport failures so the // ctx-cancel contract surfaces context.Canceled/DeadlineExceeded. @@ -1864,21 +2185,55 @@ func (a *Agent) awaitExecutionResult( }) return nil, ctxErr } - a.logExecutionError(ctx, "call.outbound.failed", "cross-node call status poll failed", map[string]any{ - "target": target, - "execution_id": executionID, - "error": err.Error(), - }) - return nil, fmt.Errorf("poll execution status: %w", err) + // 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() + _ = resp.Body.Close() if readErr != nil { return nil, fmt.Errorf("read execution status: %w", readErr) } - // Python's poll uses raise_for_status(): any HTTP error aborts the wait. + // 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, @@ -1891,6 +2246,9 @@ func (a *Agent) awaitExecutionResult( } } + // 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"` From 560f6bcacd51602836948ccb5057b98706a17bb5 Mon Sep 17 00:00:00 2001 From: Abir Abbas Date: Fri, 10 Jul 2026 15:49:45 -0400 Subject: [PATCH 12/13] test(sdk/go): cover Agent.Call resilience to transient CP outages MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Extends the async-Call test suite for the submit/poll resilience change: - Poll retries: 2 healthy polls, then 3 transient 5xx, then recovery — the call succeeds and each transient failure emits a warn call.outbound.poll_retry log with an attempt count. - Poll unreachable: a CP down past the (env-shortened) window fails with the "control plane unreachable" error, having retried rather than dying on the first blip. - 404 after submit: retried within the bounded window, then a distinct 404 not-found error. - Submit connection-refused: refused twice then accepted creates EXACTLY ONE execution (asserted server-side); refused beyond the window fails unreachable. - Submit ambiguous timeout: a hung-but-listening CP is NOT retried (exactly one request reaches the server) and fails with a clear message. - Env overrides read once and cached; invalid values fall back to defaults. - Direct unit tests for requestNeverReachedServer (dial/DNS/econnrefused vs ambiguous/opaque), isTransientPollStatus, nextRetryBackoff, and sleepCtx. Existing branch tests updated for the new semantics: submit/poll now use the dedicated call clients; a persistent poll transport error is retried to the unreachable-window error; a permanent poll status uses 403 (5xx is now retried). Also checks previously-unchecked json.Encode returns and switches to tagged switches so golangci-lint is clean on the branch delta. Co-Authored-By: Claude Fable 5 --- sdk/go/agent/agent_call_coverage_test.go | 396 +++++++++++++++++++++-- sdk/go/agent/agent_test.go | 30 +- 2 files changed, 388 insertions(+), 38 deletions(-) diff --git a/sdk/go/agent/agent_call_coverage_test.go b/sdk/go/agent/agent_call_coverage_test.go index 7065d31ef..a09a816f8 100644 --- a/sdk/go/agent/agent_call_coverage_test.go +++ b/sdk/go/agent/agent_call_coverage_test.go @@ -6,8 +6,13 @@ import ( "errors" "io" "log" + "net" "net/http" "net/http/httptest" + "net/url" + "strings" + "sync/atomic" + "syscall" "testing" "time" @@ -82,12 +87,16 @@ func TestSubmitAsyncExecution_BuildRequestError(t *testing.T) { assert.Contains(t, err.Error(), "build request") } -// Contract: a transport failure on the async submit surfaces a -// "perform execute call" error. +// 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") - a.httpClient = &http.Client{Transport: roundTripperFunc(func(*http.Request) (*http.Response, error) { - return nil, errors.New("dial tcp: connection refused") + 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( @@ -96,13 +105,15 @@ func TestSubmitAsyncExecution_TransportError(t *testing.T) { ) 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.httpClient = &http.Client{Transport: roundTripperFunc(func(*http.Request) (*http.Response, error) { + a.callSubmitClient = &http.Client{Transport: roundTripperFunc(func(*http.Request) (*http.Response, error) { return &http.Response{ StatusCode: http.StatusAccepted, Header: make(http.Header), @@ -197,7 +208,7 @@ func TestAwaitExecutionResult_BuildRequestError(t *testing.T) { // preserving the ctx-cancel contract. func TestAwaitExecutionResult_ContextCancelDuringPoll(t *testing.T) { a := newCallTestAgent(t, "http://placeholder.invalid") - a.httpClient = &http.Client{Transport: roundTripperFunc(func(req *http.Request) (*http.Response, error) { + a.callPollClient = &http.Client{Transport: roundTripperFunc(func(req *http.Request) (*http.Response, error) { <-req.Context().Done() return nil, req.Context().Err() })} @@ -213,27 +224,36 @@ func TestAwaitExecutionResult_ContextCancelDuringPoll(t *testing.T) { assert.ErrorIs(t, err, context.DeadlineExceeded) } -// Contract: a transport failure on a status poll (with a live context) -// surfaces a "poll execution status" error. +// 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") - a.httpClient = &http.Client{Transport: roundTripperFunc(func(*http.Request) (*http.Response, error) { + 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.httpClient = &http.Client{Transport: roundTripperFunc(func(*http.Request) (*http.Response, error) { + a.callPollClient = &http.Client{Transport: roundTripperFunc(func(*http.Request) (*http.Response, error) { return &http.Response{ StatusCode: http.StatusOK, Header: make(http.Header), @@ -249,12 +269,14 @@ func TestAwaitExecutionResult_ReadBodyError(t *testing.T) { assert.Contains(t, err.Error(), "read execution status") } -// Contract: an HTTP error status on a status poll aborts the wait with an -// *ExecuteError carrying that status (Python raise_for_status parity). +// 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 { - case r.Method == http.MethodPost: + switch r.Method { + case http.MethodPost: w.WriteHeader(http.StatusAccepted) _ = json.NewEncoder(w).Encode(map[string]any{ "execution_id": "exec-1", @@ -262,8 +284,8 @@ func TestCall_PollReturnsErrorStatus(t *testing.T) { "status": "queued", }) default: - w.WriteHeader(http.StatusInternalServerError) - _, _ = w.Write([]byte("upstream boom")) + w.WriteHeader(http.StatusForbidden) + _, _ = w.Write([]byte("permission denied")) } })) defer server.Close() @@ -276,7 +298,7 @@ func TestCall_PollReturnsErrorStatus(t *testing.T) { var execErr *ExecuteError require.ErrorAs(t, err, &execErr) - assert.Equal(t, http.StatusInternalServerError, execErr.StatusCode) + assert.Equal(t, http.StatusForbidden, execErr.StatusCode) assert.Contains(t, execErr.Message, "execution status failed") } @@ -284,8 +306,8 @@ func TestCall_PollReturnsErrorStatus(t *testing.T) { // "decode execute response" error. func TestCall_PollDecodeError(t *testing.T) { server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { - switch { - case r.Method == http.MethodPost: + switch r.Method { + case http.MethodPost: w.WriteHeader(http.StatusAccepted) _ = json.NewEncoder(w).Encode(map[string]any{ "execution_id": "exec-1", @@ -310,8 +332,8 @@ func TestCall_PollDecodeError(t *testing.T) { // 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 { - case r.Method == http.MethodPost: + switch r.Method { + case http.MethodPost: w.WriteHeader(http.StatusAccepted) _ = json.NewEncoder(w).Encode(map[string]any{ "execution_id": "exec-1", @@ -337,8 +359,8 @@ func TestCall_SucceededResultDecodeError(t *testing.T) { // 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 { - case r.Method == http.MethodPost: + switch r.Method { + case http.MethodPost: w.WriteHeader(http.StatusAccepted) _ = json.NewEncoder(w).Encode(map[string]any{ "execution_id": "exec-1", @@ -357,3 +379,331 @@ func TestCall_SucceededNullResult(t *testing.T) { 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 403c88b08..776b39d1c 100644 --- a/sdk/go/agent/agent_test.go +++ b/sdk/go/agent/agent_test.go @@ -720,7 +720,7 @@ func TestCall(t *testing.T) { assert.Equal(t, map[string]any{"value": float64(42)}, reqBody["input"]) w.WriteHeader(http.StatusAccepted) - json.NewEncoder(w).Encode(map[string]any{ + _ = json.NewEncoder(w).Encode(map[string]any{ "execution_id": "exec-1", "run_id": "run-1", "status": "queued", @@ -733,14 +733,14 @@ func TestCall(t *testing.T) { // First poll: still running; second poll: terminal result. if polls.Add(1) == 1 { - json.NewEncoder(w).Encode(map[string]any{ + _ = 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{ + _ = json.NewEncoder(w).Encode(map[string]any{ "execution_id": "exec-1", "run_id": "run-1", "status": "succeeded", @@ -792,7 +792,7 @@ func TestCall_OutlivesCallTimeout(t *testing.T) { 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{ + _ = json.NewEncoder(w).Encode(map[string]any{ "execution_id": "exec-slow", "run_id": "run-slow", "status": "queued", @@ -801,10 +801,10 @@ func TestCall_OutlivesCallTimeout(t *testing.T) { // 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"}) + _ = json.NewEncoder(w).Encode(map[string]any{"status": "running"}) return } - json.NewEncoder(w).Encode(map[string]any{ + _ = json.NewEncoder(w).Encode(map[string]any{ "status": "succeeded", "result": map[string]any{"took": "longer than CallTimeout"}, }) @@ -840,14 +840,14 @@ func TestCall_CtxCancelAbortsWait(t *testing.T) { 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{ + _ = 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"}) + _ = json.NewEncoder(w).Encode(map[string]any{"status": "running"}) default: w.WriteHeader(http.StatusNotFound) } @@ -894,7 +894,7 @@ func TestCall_ErrorHandling(t *testing.T) { 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"}) + _ = json.NewEncoder(w).Encode(map[string]any{"status": "queued"}) }, wantErrSubstr: "missing identifiers", }, @@ -903,14 +903,14 @@ func TestCall_ErrorHandling(t *testing.T) { serverResponse: func(w http.ResponseWriter, r *http.Request) { if r.Method == http.MethodPost { w.WriteHeader(http.StatusAccepted) - json.NewEncoder(w).Encode(map[string]any{ + _ = 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{ + _ = json.NewEncoder(w).Encode(map[string]any{ "status": "failed", "error_message": "execution failed", }) @@ -922,7 +922,7 @@ func TestCall_ErrorHandling(t *testing.T) { serverResponse: func(w http.ResponseWriter, r *http.Request) { if r.Method == http.MethodPost { w.WriteHeader(http.StatusAccepted) - json.NewEncoder(w).Encode(map[string]any{ + _ = json.NewEncoder(w).Encode(map[string]any{ "execution_id": "exec-e", "run_id": "run-e", "status": "queued", @@ -932,7 +932,7 @@ func TestCall_ErrorHandling(t *testing.T) { // 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{ + _ = json.NewEncoder(w).Encode(map[string]any{ "status": "failed", "error": "boom from error field", }) @@ -944,14 +944,14 @@ func TestCall_ErrorHandling(t *testing.T) { serverResponse: func(w http.ResponseWriter, r *http.Request) { if r.Method == http.MethodPost { w.WriteHeader(http.StatusAccepted) - json.NewEncoder(w).Encode(map[string]any{ + _ = 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"}) + _ = json.NewEncoder(w).Encode(map[string]any{"status": "cancelled"}) }, wantErrSubstr: "execute status cancelled", }, From 55ad55f3159234bcc354a4bc37abeaf616b252ca Mon Sep 17 00:00:00 2001 From: Abir Abbas Date: Fri, 10 Jul 2026 15:53:13 -0400 Subject: [PATCH 13/13] docs(sdk/go): correct Call timeout docs for the dedicated call clients The Call() and Config.CallTimeout doc comments still claimed CallTimeout bounds the async submit and each status poll. That is no longer true: the Call path now uses dedicated submit/poll HTTP clients bounded by AGENTFIELD_CALL_SUBMIT_TIMEOUT_SECONDS / AGENTFIELD_CALL_POLL_TIMEOUT_SECONDS, with transient failures retried within AGENTFIELD_CALL_RETRY_WINDOW_SECONDS. Update both comments so the documented timeout semantics match the code. Co-Authored-By: Claude Fable 5 --- sdk/go/agent/agent.go | 32 ++++++++++++++++++++------------ 1 file changed, 20 insertions(+), 12 deletions(-) diff --git a/sdk/go/agent/agent.go b/sdk/go/agent/agent.go index 43695e8a7..89b02ec4a 100644 --- a/sdk/go/agent/agent.go +++ b/sdk/go/agent/agent.go @@ -369,12 +369,15 @@ type Config struct { LeaseRefreshInterval time.Duration // CallTimeout bounds every INDIVIDUAL outbound HTTP request this agent - // makes as a client - memory backend requests, and for cross-agent - // Call()s the async submit plus each status poll. It does NOT bound a - // Call() end-to-end: Call submits to the async execute endpoint and - // polls until the child execution finishes, so a child reasoner may run - // arbitrarily longer than CallTimeout (bound the overall wait with the - // ctx passed to Call instead). Optional. Default: 15s. + // 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. @@ -1803,12 +1806,17 @@ func nextCallPollInterval(current time.Duration) time.Duration { // status — mirroring the Python SDK's app.call // (sdk/python/agentfield/client.py _submit_execution_sync/_await_execution_sync). // -// Timeout semantics: Config.CallTimeout bounds each individual HTTP request -// (the submit and every poll), NOT the end-to-end 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). +// 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")