Consolidate the SWE and PR-AF nodes in the app, and make installs superseded-aware

control-plane/internal/cli/catalog.go

@@ -15,9 +15,24 @@ import (
 // offline and gives a harness a curated set of nodes to install before any
 // registry search lands. It is seeded from the desktop app's curated list
 // (desktop/src/shared/catalog.ts) — keep the two in sync when adding nodes.
-// `name` MUST equal the node's agentfield-package.yaml `name:` (the registry
-// key after install), which is often not the repo name (SWE-AF//go →
-// swe-planner).
+//
+// One row per product, sourced at the bare repo URL. A repo that ships more
+// than one implementation of the same node says which one it wants installed
+// with `superseded_by:` in its root manifest — the redirect that makes
+// `af install <repo>` land on the maintained node (SWE-AF and pr-af both point
+// their root at `//go`). Naming `//go` here would install that same node, but
+// it would skip the redirect, and the redirect is what carries a user who
+// already has the superseded node across: it installs the successor first,
+// migrates node-scoped secrets, and only then retires the old package. So the
+// catalog names the repo and lets the manifest decide.
+//
+// `name` MUST equal the name the package ends up registered under once the
+// install settles — the registry key a harness then passes to `af run`. Note
+// that is the name after any `superseded_by:` redirect resolves, which need
+// not be the `name:` in the manifest at the source: a successor may
+// deliberately take its predecessor's name (an in-place rename), and it may
+// live in a subdirectory this list never names. It is often not the repo name
+// either (SWE-AF → swe-planner).
 type nodeCatalogEntry struct {
 	Name        string `json:"name"`
 	Description string `json:"description"`
@@ -33,22 +48,15 @@ var nodeCatalog = []nodeCatalogEntry{
 	{
 		Name:        "swe-planner",
 		Description: "Autonomous software-engineering fleet: plan, code, test, and ship production-grade PRs — one static binary",
-		Source:      "https://github.com/Agent-Field/SWE-AF//go",
+		Source:      "https://github.com/Agent-Field/SWE-AF",
 		Docs:        "https://github.com/Agent-Field/SWE-AF",
 		Language:    "go",
 	},
 	{
 		Name:        "pr-af",
-		Description: "Turns a plain task description into a draft pull request on GitHub",
+		Description: "Deep, evidence-backed review of any GitHub pull request — one static binary",
 		Source:      "https://github.com/Agent-Field/pr-af",
 		Docs:        "https://github.com/Agent-Field/pr-af",
-		Language:    "python",
-	},
-	{
-		Name:        "pr-af-go",
-		Description: "Go port of the PR review agent: same reasoners, one static binary",
-		Source:      "https://github.com/Agent-Field/pr-af//go",
-		Docs:        "https://github.com/Agent-Field/pr-af",
 		Language:    "go",
 	},
 	{

control-plane/internal/cli/catalog_test.go

@@ -15,7 +15,7 @@ func TestRunCatalogJSON(t *testing.T) {
 
 	var entries []map[string]interface{}
 	require.NoError(t, json.Unmarshal(stdout.Bytes(), &entries))
-	require.GreaterOrEqual(t, len(entries), 5, "catalog must list at least five installable nodes")
+	require.GreaterOrEqual(t, len(entries), 4, "catalog must list at least four installable nodes")
 
 	for _, e := range entries {
 		require.NotEmpty(t, e["name"], "entry missing name: %v", e)
@@ -32,23 +32,43 @@ func TestRunCatalogPrettyEndsWithInstallHint(t *testing.T) {
 	require.Contains(t, out, "swe-planner")
 }
 
-// The SWE fleet ships as exactly one catalog row, named for the product rather
-// than the implementation and installed from the `//go` source selector. A
-// second entry — a re-added root/Python row, or the old implementation-suffixed
+// A repo that ships both a Python node and its Go counterpart is offered as
+// exactly one row, named for the product rather than the implementation, and
+// installed from the bare repo URL so the root manifest's `superseded_by:`
+// redirect decides which node lands (and carries an existing install across).
+// A second row — a re-added Python entry, or the old implementation-suffixed
 // name creeping back — must fail here rather than reappear in `af catalog`.
-func TestCatalogHasSingleGoSWEEntry(t *testing.T) {
-	var sweEntries []nodeCatalogEntry
-	for _, e := range nodeCatalog {
-		if strings.Contains(e.Source, "Agent-Field/SWE-AF") {
-			sweEntries = append(sweEntries, e)
-		}
-	}
+func TestCatalogOffersConsolidatedNodesOnce(t *testing.T) {
+	for _, tc := range []struct {
+		repo    string
+		want    string
+		retired string
+	}{
+		{repo: "Agent-Field/SWE-AF", want: "swe-planner", retired: "swe-planner-go"},
+		{repo: "Agent-Field/pr-af", want: "pr-af", retired: "pr-af-go"},
+	} {
+		t.Run(tc.want, func(t *testing.T) {
+			var entries []nodeCatalogEntry
+			for _, e := range nodeCatalog {
+				if strings.Contains(e.Source, tc.repo) {
+					entries = append(entries, e)
+				}
+			}
+
+			require.Len(t, entries, 1, "exactly one catalog entry may install from %s", tc.repo)
+			require.Equal(t, tc.want, entries[0].Name,
+				"the entry is named for the product, not the implementation")
+			require.Equal(t, "https://github.com/"+tc.repo, entries[0].Source,
+				"source must be the bare repo URL so superseded_by picks the node")
+			require.Equal(t, "go", entries[0].Language,
+				"the redirect lands on the Go node, so that is what the row advertises")
 
-	require.Len(t, sweEntries, 1, "exactly one catalog entry may install from Agent-Field/SWE-AF")
-	require.Equal(t, "swe-planner", sweEntries[0].Name,
-		"the SWE entry is named for the product, not the implementation")
-	require.True(t, strings.HasSuffix(sweEntries[0].Source, "//go"),
-		"SWE entry source must select the go subdirectory, got %q", sweEntries[0].Source)
+			for _, e := range nodeCatalog {
+				require.NotEqual(t, tc.retired, e.Name,
+					"%q is the pre-consolidation name and must not reappear", tc.retired)
+			}
+		})
+	}
 }
 
 func TestRunCatalogRejectsUnknownFormat(t *testing.T) {
@@ -65,5 +85,5 @@ func TestNewCatalogCommandExecute(t *testing.T) {
 	})
 	var entries []map[string]interface{}
 	require.NoError(t, json.Unmarshal([]byte(out), &entries))
-	require.GreaterOrEqual(t, len(entries), 5)
+	require.GreaterOrEqual(t, len(entries), 4)
 }

control-plane/internal/core/services/install_result_test.go

@@ -0,0 +1,212 @@
+package services
+
+import (
+	"os"
+	"os/exec"
+	"path/filepath"
+	"testing"
+	"time"
+
+	"github.com/Agent-Field/agentfield/control-plane/internal/core/domain"
+	"github.com/stretchr/testify/require"
+)
+
+// Validation contract for InstallPackageWithResult — the seam that lets an
+// install job report the package that actually landed rather than inferring it
+// from a registry diff:
+//
+//  1. A local install reports the manifest's name.
+//  2. A git install reports the name the git installer recorded.
+//  3. A git install of a package whose manifest declares `superseded_by:`
+//     reports the SUCCESSOR's name — including when the successor takes the
+//     predecessor's own name, where a registry diff sees no change at all.
+//  4. A failed install reports no name.
+//  5. A declared node dependency that cannot be installed does not fail the
+//     parent install, which is already in place and usable.
+
+// runGit runs a git command in dir, with a fixed identity so the fixture does
+// not depend on the host's git config.
+func runGit(t *testing.T, dir string, args ...string) {
+	t.Helper()
+	cmd := exec.Command("git", args...)
+	cmd.Dir = dir
+	cmd.Env = append(os.Environ(),
+		"GIT_AUTHOR_NAME=t", "GIT_AUTHOR_EMAIL=t@t",
+		"GIT_COMMITTER_NAME=t", "GIT_COMMITTER_EMAIL=t@t")
+	out, err := cmd.CombinedOutput()
+	require.NoError(t, err, "git %v: %s", args, out)
+}
+
+// bareRepoAt commits everything in dir and bare-clones it to bare, which is the
+// source `af install` routes to the git installer (the `.git` suffix is what
+// IsGitURL keys on for a plain path).
+func bareRepoAt(t *testing.T, dir, bare string) string {
+	t.Helper()
+	runGit(t, dir, "init", "-q")
+	runGit(t, dir, "add", "-A")
+	runGit(t, dir, "commit", "-qm", "fixture")
+	out, err := exec.Command("git", "clone", "-q", "--bare", dir, bare).CombinedOutput()
+	require.NoError(t, err, "git clone --bare: %s", out)
+	return bare
+}
+
+// Contract 1: a local install reports the name in the manifest.
+func TestInstallPackageWithResult_LocalReportsManifestName(t *testing.T) {
+	home := t.TempDir()
+	src := filepath.Join(t.TempDir(), "repo")
+	writeNode(t, src, "local-node")
+
+	name, err := newLocalPackageService(t, home).InstallPackageWithResult(src, domain.InstallOptions{})
+	require.NoError(t, err)
+	require.Equal(t, "local-node", name)
+}
+
+// Contract 2: a git install reports what the git installer recorded.
+func TestInstallPackageWithResult_GitReportsInstalledName(t *testing.T) {
+	home := t.TempDir()
+	src := filepath.Join(t.TempDir(), "repo")
+	writeNode(t, src, "git-node")
+
+	bare := bareRepoAt(t, src, filepath.Join(t.TempDir(), "fixture.git"))
+	name, err := newLocalPackageService(t, home).
+		InstallPackageWithResult(bare, domain.InstallOptions{})
+	require.NoError(t, err)
+	require.Equal(t, "git-node", name)
+	require.True(t, installedNamesFromRegistry(t, home)["git-node"])
+}
+
+// Contract 3: a redirect reports the successor. The same-name case is the one a
+// registry diff cannot see — the set of installed names is identical before and
+// after — so it is the reason this seam exists at all.
+func TestInstallPackageWithResult_ReportsSupersededSuccessor(t *testing.T) {
+	for _, tc := range []struct {
+		name          string
+		successorName string
+	}{
+		{name: "successor takes a new name", successorName: "successor-node"},
+		{name: "successor takes the same name", successorName: "redirected-node"},
+	} {
+		t.Run(tc.name, func(t *testing.T) {
+			home := t.TempDir()
+			src := filepath.Join(t.TempDir(), "repo")
+			writeNode(t, filepath.Join(src, "v2"), tc.successorName)
+
+			// The root redirects into this same repo's v2/ subdirectory. Written
+			// after the bare clone location is known, so point at it directly.
+			bare := filepath.Join(t.TempDir(), "fixture.git")
+			require.NoError(t, os.MkdirAll(src, 0o755))
+			require.NoError(t, os.WriteFile(
+				filepath.Join(src, "agentfield-package.yaml"),
+				[]byte("name: redirected-node\nversion: 1.0.0\nmain: main.py\nsuperseded_by: "+bare+"//v2\n"),
+				0o644))
+			require.NoError(t, os.WriteFile(filepath.Join(src, "main.py"), []byte("print('ok')\n"), 0o644))
+
+			bareRepoAt(t, src, bare)
+
+			name, err := newLocalPackageService(t, home).
+				InstallPackageWithResult(bare, domain.InstallOptions{})
+			require.NoError(t, err)
+			require.Equal(t, tc.successorName, name,
+				"the successor's name is what landed, so it is what must be reported")
+			require.True(t, installedNamesFromRegistry(t, home)[tc.successorName])
+		})
+	}
+}
+
+// Contract 4: a failed install reports no name, whatever stage it failed at.
+func TestInstallPackageWithResult_FailuresReportNoName(t *testing.T) {
+	t.Run("invalid package structure", func(t *testing.T) {
+		home := t.TempDir()
+		src := filepath.Join(t.TempDir(), "repo")
+		// Declares an entrypoint it does not ship, so validation rejects it.
+		require.NoError(t, os.MkdirAll(src, 0o755))
+		require.NoError(t, os.WriteFile(filepath.Join(src, "agentfield-package.yaml"),
+			[]byte("name: broken-node\nversion: 1.0.0\nmain: missing.py\n"), 0o644))
+
+		name, err := newLocalPackageService(t, home).InstallPackageWithResult(src, domain.InstallOptions{})
+		require.Error(t, err)
+		require.Empty(t, name)
+		require.Empty(t, installedNamesFromRegistry(t, home),
+			"a rejected package must not reach the registry")
+	})
+
+	t.Run("dependency build fails", func(t *testing.T) {
+		home := t.TempDir()
+		src := filepath.Join(t.TempDir(), "repo")
+		require.NoError(t, os.MkdirAll(src, 0o755))
+		// A Go node whose build cannot succeed: the manifest promises a build
+		// entrypoint, and there is no Go module behind it.
+		require.NoError(t, os.WriteFile(filepath.Join(src, "agentfield-package.yaml"),
+			[]byte("name: broken-go-node\nversion: 1.0.0\nlanguage: go\nentrypoint:\n  build: ./cmd/nope\n  start: bin/nope\n"), 0o644))
+
+		name, err := newLocalPackageService(t, home).InstallPackageWithResult(src, domain.InstallOptions{})
+		require.Error(t, err)
+		require.Empty(t, name)
+	})
+
+	t.Run("registry cannot be written", func(t *testing.T) {
+		home := t.TempDir()
+		src := filepath.Join(t.TempDir(), "repo")
+		writeNode(t, src, "unwritable-node")
+		// A directory where the registry file belongs: the write fails, and the
+		// install must surface that rather than claim a name.
+		require.NoError(t, os.Mkdir(filepath.Join(home, "installed.yaml"), 0o755))
+
+		name, err := newLocalPackageService(t, home).InstallPackageWithResult(src, domain.InstallOptions{})
+		require.Error(t, err)
+		require.Empty(t, name)
+	})
+}
+
+// Contract: a dependency cycle terminates. Two packages that declare each other
+// as node dependencies, by bare path — the form `resolveNodeRef` cannot name, so
+// the already-installed check never fires — and with Force set, which is what
+// every update uses, so each install succeeds rather than being refused. Without
+// a walk-tracking guard this recurses until the process dies, taking the package
+// job manager's `active` latch with it and blocking every later install.
+func TestInstallPackageWithResult_DependencyCycleTerminates(t *testing.T) {
+	home := t.TempDir()
+	dir := t.TempDir()
+	a, b := filepath.Join(dir, "a"), filepath.Join(dir, "b")
+	for _, n := range []struct{ path, name, dep string }{{a, "cycle-a", b}, {b, "cycle-b", a}} {
+		require.NoError(t, os.MkdirAll(n.path, 0o755))
+		require.NoError(t, os.WriteFile(filepath.Join(n.path, "agentfield-package.yaml"),
+			[]byte("name: "+n.name+"\nversion: 1.0.0\nmain: main.py\ndependencies:\n  nodes:\n    - "+n.dep+"\n"), 0o644))
+		require.NoError(t, os.WriteFile(filepath.Join(n.path, "main.py"), []byte("print('ok')\n"), 0o644))
+	}
+
+	done := make(chan struct{})
+	var name string
+	var err error
+	go func() {
+		defer close(done)
+		name, err = newLocalPackageService(t, home).
+			InstallPackageWithResult(a, domain.InstallOptions{Force: true})
+	}()
+	select {
+	case <-done:
+	case <-time.After(30 * time.Second):
+		t.Fatal("mutually-dependent packages recursed without terminating")
+	}
+	require.NoError(t, err)
+	require.Equal(t, "cycle-a", name)
+	installed := installedNamesFromRegistry(t, home)
+	require.True(t, installed["cycle-a"] && installed["cycle-b"], "both sides of the cycle install once")
+}
+
+// Contract 5: a node dependency that cannot be installed is reported but does
+// not fail the parent — the parent is already installed and usable.
+func TestInstallPackageWithResult_UninstallableDependencyDoesNotFailParent(t *testing.T) {
+	home := t.TempDir()
+	src := filepath.Join(t.TempDir(), "repo")
+	require.NoError(t, os.MkdirAll(src, 0o755))
+	require.NoError(t, os.WriteFile(filepath.Join(src, "agentfield-package.yaml"),
+		[]byte("name: parent-node\nversion: 1.0.0\nmain: main.py\ndependencies:\n  nodes:\n    - "+
+			filepath.Join(t.TempDir(), "does-not-exist")+"\n"), 0o644))
+	require.NoError(t, os.WriteFile(filepath.Join(src, "main.py"), []byte("print('ok')\n"), 0o644))
+
+	name, err := newLocalPackageService(t, home).InstallPackageWithResult(src, domain.InstallOptions{})
+	require.NoError(t, err, "the parent installed; a bad dependency is not its failure")
+	require.Equal(t, "parent-node", name)
+	require.True(t, installedNamesFromRegistry(t, home)["parent-node"])
+}

control-plane/internal/core/services/node_deps_test.go

@@ -75,9 +75,7 @@ func TestInstallNodeDependencies_SkipsAlreadyInstalled(t *testing.T) {
 		},
 	})
 
-	// `before` is empty, so both packages count as newly installed; the declared
-	// dep (echo-node) is already installed and must be skipped — no network call.
-	err := ps.installNodeDependencies(map[string]bool{}, domain.InstallOptions{})
+	err := ps.installNodeDependencies("caller", domain.InstallOptions{}, map[string]bool{"caller": true})
 	require.NoError(t, err)
 }