From 8517c132dc9be9dc301a39afe501d4fe49d72beb Mon Sep 17 00:00:00 2001 From: Abir Abbas Date: Tue, 4 Aug 2026 16:31:42 -0400 Subject: [PATCH 1/6] fix(packages): an install job reports the package the installer actually installed MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `Manager.run` inferred the installed package by diffing the registry's names before and after: whichever name is new must be the one this job installed. That inference breaks on exactly the case `superseded_by` was added for. A successor may declare its predecessor's name — an in-place rename, which is what both Agent-Field/SWE-AF#122 and Agent-Field/pr-af#64 use, and what keeps a node id, its triggers, and its node-scoped secrets intact across the swap. The set of installed names is then identical before and after, so the diff finds nothing and the job reports an empty package name. AgentField Desktop streams that job's output, so the user watched a successful install end in "install completed: " with the name missing. When the successor's name *does* differ, the diff happened to work, but only by luck: it returns the first registry name that is new, so any unrelated entry appearing during the install is misattributed to this job. The installer already knows the answer — `GitInstaller` tracks it in `installedName` and propagates it through a redirect. Export it, thread it out through the package service as `InstallPackageWithResult`, and have the job prefer it, keeping the before/after diff as the fallback for installers that cannot report a name. Node-dependency discovery used the same diff idiom and is switched to the authoritative name too, which also stops it from walking the dependencies of a package some other caller installed concurrently. Updates take the authoritative name as well. `StartUpdate` pre-seeds the job with the name being updated, so previously the installer's answer was discarded — and an update whose recorded source redirects to a differently named successor would then try to restart the package the redirect had just uninstalled. It now reports and restarts the node that exists. Co-Authored-By: Claude Opus 5 (1M context) --- .../internal/core/services/node_deps_test.go | 4 +- .../internal/core/services/package_service.go | 69 ++++++----- control-plane/internal/packages/git.go | 6 + .../internal/packages/git_supersede_test.go | 36 +++++- .../internal/services/packagejobs/manager.go | 21 +++- .../services/packagejobs/manager_test.go | 110 +++++++++++++++++- 6 files changed, 211 insertions(+), 35 deletions(-) diff --git a/control-plane/internal/core/services/node_deps_test.go b/control-plane/internal/core/services/node_deps_test.go index a7b72558a..6232a6f2d 100644 --- a/control-plane/internal/core/services/node_deps_test.go +++ b/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{}) require.NoError(t, err) } diff --git a/control-plane/internal/core/services/package_service.go b/control-plane/internal/core/services/package_service.go index 64b9d7e79..99ae2f4bd 100644 --- a/control-plane/internal/core/services/package_service.go +++ b/control-plane/internal/core/services/package_service.go @@ -38,12 +38,18 @@ func NewPackageService( // InstallPackage installs a package from the given source func (ps *DefaultPackageService) InstallPackage(source string, options domain.InstallOptions) error { - // Snapshot installed packages so we can discover what this install adds and - // recursively pull in any node-to-node dependencies it declares. - before := ps.installedNames() + _, err := ps.InstallPackageWithResult(source, options) + return err +} - if err := ps.installOne(source, options); err != nil { - return err +// InstallPackageWithResult installs a package and reports the package name +// selected by the installer. For superseded packages this is the final +// successor, including when it replaces an existing package under the same +// name. +func (ps *DefaultPackageService) InstallPackageWithResult(source string, options domain.InstallOptions) (string, error) { + installedName, err := ps.installOne(source, options) + if err != nil { + return "", err } // The --path selector targets a subdirectory of THIS source only. Node @@ -51,11 +57,14 @@ func (ps *DefaultPackageService) InstallPackage(source string, options domain.In // into recursive dependency installs. depOptions := options depOptions.Path = "" - return ps.installNodeDependencies(before, depOptions) + if err := ps.installNodeDependencies(installedName, depOptions); err != nil { + return "", err + } + return installedName, nil } // installOne installs a single package from a git URL or local path. -func (ps *DefaultPackageService) installOne(source string, options domain.InstallOptions) error { +func (ps *DefaultPackageService) installOne(source string, options domain.InstallOptions) (string, error) { // Check if it's a Git URL (GitHub, GitLab, Bitbucket, etc.) if packages.IsGitURL(source) { installer := &packages.GitInstaller{ @@ -63,11 +72,14 @@ func (ps *DefaultPackageService) installOne(source string, options domain.Instal Verbose: options.Verbose, Subdir: options.Path, } - return installer.InstallFromGit(source, options.Force) + if err := installer.InstallFromGit(source, options.Force); err != nil { + return "", err + } + return installer.InstalledName(), nil } // Handle local package installation - return ps.installLocalPackage(source, options.Path, options.Force, options.Verbose) + return ps.installLocalPackageWithName(source, options.Path, options.Force, options.Verbose) } // installedNames returns the set of currently-installed package names. @@ -83,18 +95,18 @@ func (ps *DefaultPackageService) installedNames() map[string]bool { return names } -// installNodeDependencies installs the node-to-node dependencies declared by any -// packages added since `before`, recursively. Already-installed nodes are -// skipped, which also breaks dependency cycles. -func (ps *DefaultPackageService) installNodeDependencies(before map[string]bool, options domain.InstallOptions) error { +// installNodeDependencies installs the node-to-node dependencies declared by +// packageName, recursively. Already-installed nodes are skipped, which also +// breaks dependency cycles. +func (ps *DefaultPackageService) installNodeDependencies(packageName string, options domain.InstallOptions) error { registry, err := ps.loadRegistryDirect() if err != nil { return nil // base install already succeeded; don't fail on dep discovery } for name, pkg := range registry.Installed { - if before[name] { - continue // not newly installed in this pass + if name != packageName { + continue } metadata, err := packages.ParsePackageMetadata(pkg.Path) if err != nil { @@ -106,13 +118,13 @@ func (ps *DefaultPackageService) installNodeDependencies(before map[string]bool, continue // already present — also handles cycles } fmt.Printf("\n%s Installing node dependency: %s\n", ps.blue("→"), dep) - snapshot := ps.installedNames() - if err := ps.installOne(depSource, options); err != nil { + installedName, err := ps.installOne(depSource, options) + if err != nil { fmt.Printf("%s Failed to install node dependency %s: %v\n", ps.statusError(), dep, err) continue } // Recurse for the dependency's own node deps. - if err := ps.installNodeDependencies(snapshot, options); err != nil { + if err := ps.installNodeDependencies(installedName, options); err != nil { return err } } @@ -145,10 +157,15 @@ func resolveNodeRef(ref string) (source string, name string) { // 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 { + _, err := ps.installLocalPackageWithName(sourcePath, subdir, force, verbose) + return err +} + +func (ps *DefaultPackageService) installLocalPackageWithName(sourcePath string, subdir string, force bool, verbose bool) (string, error) { if strings.TrimSpace(subdir) != "" { resolved, err := packages.ResolvePackageSubdir(sourcePath, subdir) if err != nil { - return err + return "", err } sourcePath = resolved } @@ -156,7 +173,7 @@ func (ps *DefaultPackageService) installLocalPackage(sourcePath string, subdir s // Get package name first for better messaging metadata, err := ps.parsePackageMetadata(sourcePath) if err != nil { - return fmt.Errorf("failed to parse package metadata: %w", err) + return "", fmt.Errorf("failed to parse package metadata: %w", err) } fmt.Printf("Installing %s...\n", metadata.Name) @@ -166,13 +183,13 @@ func (ps *DefaultPackageService) installLocalPackage(sourcePath string, subdir s spinner.Start() if err := ps.validatePackage(sourcePath); err != nil { spinner.Error("Package validation failed") - return fmt.Errorf("package validation failed: %w", err) + return "", fmt.Errorf("package validation failed: %w", err) } spinner.Success("Package structure validated") // 2. Check if already installed if !force && ps.isPackageInstalled(metadata.Name) { - return fmt.Errorf("package %s already installed (use --force to reinstall)", metadata.Name) + return "", fmt.Errorf("package %s already installed (use --force to reinstall)", metadata.Name) } // 3. Copy package to global location @@ -181,7 +198,7 @@ func (ps *DefaultPackageService) installLocalPackage(sourcePath string, subdir s spinner.Start() if err := ps.copyPackage(sourcePath, destPath); err != nil { spinner.Error("Failed to copy package") - return fmt.Errorf("failed to copy package: %w", err) + return "", fmt.Errorf("failed to copy package: %w", err) } spinner.Success("Environment configured") @@ -190,13 +207,13 @@ func (ps *DefaultPackageService) installLocalPackage(sourcePath string, subdir s spinner.Start() if err := ps.installDependencies(destPath, metadata); err != nil { spinner.Error("Failed to install dependencies") - return fmt.Errorf("failed to install dependencies: %w", err) + return "", fmt.Errorf("failed to install dependencies: %w", err) } spinner.Success("Dependencies installed") // 5. Update installation registry if err := ps.updateRegistry(metadata, sourcePath, destPath); err != nil { - return fmt.Errorf("failed to update registry: %w", err) + return "", fmt.Errorf("failed to update registry: %w", err) } fmt.Printf("%s Installed %s v%s\n", ps.green(ps.statusSuccess()), ps.bold(metadata.Name), ps.gray(metadata.Version)) @@ -207,7 +224,7 @@ func (ps *DefaultPackageService) installLocalPackage(sourcePath string, subdir s fmt.Printf("\n%s %s\n", ps.blue("→"), ps.bold(fmt.Sprintf("Run: af run %s", metadata.Name))) - return nil + return metadata.Name, nil } // UninstallPackage removes an installed package diff --git a/control-plane/internal/packages/git.go b/control-plane/internal/packages/git.go index cffe4576d..eb5fbb1bb 100644 --- a/control-plane/internal/packages/git.go +++ b/control-plane/internal/packages/git.go @@ -48,6 +48,12 @@ type GitInstaller struct { installedName string } +// InstalledName returns the package name installed by the most recent +// successful InstallFromGit call. Redirects report the final successor name. +func (gi *GitInstaller) InstalledName() string { + return gi.installedName +} + // maxSupersedeRedirects bounds a superseded_by chain. Three is generous for the // real case (one hop) and still fails fast on a manifest cycle. const maxSupersedeRedirects = 3 diff --git a/control-plane/internal/packages/git_supersede_test.go b/control-plane/internal/packages/git_supersede_test.go index 07e56ccc8..cbb6f9498 100644 --- a/control-plane/internal/packages/git_supersede_test.go +++ b/control-plane/internal/packages/git_supersede_test.go @@ -41,10 +41,13 @@ func TestInstallFromGit_SupersededRedirectsToSuccessor(t *testing.T) { writeSubdirManifest(t, filepath.Join(repo, "go"), "dual-node-go") setupFakeGit(t, "copy", repo, false) - if err := (&GitInstaller{AgentFieldHome: home}). - InstallFromGit("https://gitlab.com/acme/dual", false); err != nil { + installer := &GitInstaller{AgentFieldHome: home} + if err := installer.InstallFromGit("https://gitlab.com/acme/dual", false); err != nil { t.Fatalf("InstallFromGit: %v", err) } + if installer.InstalledName() != "dual-node-go" { + t.Fatalf("installed name = %q, want successor", installer.InstalledName()) + } registry := readRegistryFile(t, filepath.Join(home, "installed.yaml")) if _, ok := registry.Installed["dual-node-go"]; !ok { @@ -228,12 +231,18 @@ func TestInstallFromGit_SupersededSameNameReplacesInPlace(t *testing.T) { t.Fatal(err) } - if err := (&GitInstaller{AgentFieldHome: home}). - InstallFromGit("https://gitlab.com/acme/dual", false); err != nil { + installer := &GitInstaller{AgentFieldHome: home} + if err := installer.InstallFromGit("https://gitlab.com/acme/dual", false); err != nil { t.Fatalf("same-name supersede must not need --force: %v", err) } + if installer.InstalledName() != "dual-node" { + t.Fatalf("installed name = %q, want shared name", installer.InstalledName()) + } registry := readRegistryFile(t, filepath.Join(home, "installed.yaml")) + if len(registry.Installed) != 1 { + t.Fatalf("in-place replacement must leave one registry entry, got %v", registry.Installed) + } pkg, ok := registry.Installed["dual-node"] if !ok { t.Fatalf("the shared name must still be installed, got %v", registry.Installed) @@ -241,6 +250,9 @@ func TestInstallFromGit_SupersededSameNameReplacesInPlace(t *testing.T) { if pkg.Version != "2.0.0" { t.Fatalf("registry still describes the predecessor: version %q", pkg.Version) } + if pkg.SourcePath != "https://gitlab.com/acme/dual//go" { + t.Fatalf("source path = %q, want successor source", pkg.SourcePath) + } if _, err := os.Stat(filepath.Join(oldDir, "successor.txt")); err != nil { t.Fatalf("successor's files are not installed: %v", err) } @@ -249,6 +261,22 @@ func TestInstallFromGit_SupersededSameNameReplacesInPlace(t *testing.T) { } } +// Contract: a plain install reports the manifest name without a redirect. +func TestInstallFromGit_PlainInstallReportsManifestName(t *testing.T) { + home := t.TempDir() + repo := filepath.Join(t.TempDir(), "repo") + writeTestPackage(t, repo, "name: plain-node\nversion: 1.0.0\n") + setupFakeGit(t, "copy", repo, false) + + installer := &GitInstaller{AgentFieldHome: home} + if err := installer.InstallFromGit("https://gitlab.com/acme/plain", false); err != nil { + t.Fatalf("InstallFromGit: %v", err) + } + if installer.InstalledName() != "plain-node" { + t.Fatalf("installed name = %q, want plain-node", installer.InstalledName()) + } +} + // Contract: node-scoped secrets survive a same-name replace. They never move — // the scope name is unchanged — so the risk is the retire path deleting them. func TestInstallFromGit_SupersededSameNameKeepsNodeScopedSecrets(t *testing.T) { diff --git a/control-plane/internal/services/packagejobs/manager.go b/control-plane/internal/services/packagejobs/manager.go index 68fabdc57..5c72e3770 100644 --- a/control-plane/internal/services/packagejobs/manager.go +++ b/control-plane/internal/services/packagejobs/manager.go @@ -70,6 +70,10 @@ type installer interface { GetPackageInfo(name string) (*domain.InstalledPackage, error) } +type resultInstaller interface { + InstallPackageWithResult(source string, options domain.InstallOptions) (string, error) +} + type Manager struct { mu sync.RWMutex installer installer @@ -218,7 +222,22 @@ func (m *Manager) run(jobID string, force bool) { before := m.installedNames() if err == nil { installSource, options := splitSubdir(source, force) - err = m.installer.InstallPackage(installSource, options) + if reporting, ok := m.installer.(resultInstaller); ok { + var installedName string + installedName, err = reporting.InstallPackageWithResult(installSource, options) + // The installer is authoritative about what it installed, and an + // update is where that matters most: a `superseded_by` redirect in + // the recorded source can retire the package being updated and put + // a differently-named successor in its place. Following the + // installer here means the job reports — and restarts — the node + // that now exists, rather than the name that went in and no longer + // resolves. + if err == nil && installedName != "" { + packageName = installedName + } + } else { + err = m.installer.InstallPackage(installSource, options) + } } if err == nil && packageName == "" { packageName = m.discoverPackageName(before) diff --git a/control-plane/internal/services/packagejobs/manager_test.go b/control-plane/internal/services/packagejobs/manager_test.go index 6f9d3ea7a..2d07a711f 100644 --- a/control-plane/internal/services/packagejobs/manager_test.go +++ b/control-plane/internal/services/packagejobs/manager_test.go @@ -17,6 +17,7 @@ import ( type stubInstaller struct { mu sync.Mutex installErr error + resultName string block <-chan struct{} installed []domain.InstalledPackage afterInstall []domain.InstalledPackage @@ -27,6 +28,14 @@ type stubInstaller struct { uninstallErr error } +func (s *stubInstaller) InstallPackageWithResult(source string, options domain.InstallOptions) (string, error) { + err := s.InstallPackage(source, options) + if err != nil { + return "", err + } + return s.resultName, nil +} + func (s *stubInstaller) InstallPackage(_ string, options domain.InstallOptions) error { if s.block != nil { <-s.block @@ -108,7 +117,7 @@ func waitForJob(t *testing.T, manager *Manager, id string) *Job { // Contract 1: a valid GitHub install succeeds and records its package name. func TestInstallSucceedsAndDiscoversPackageName(t *testing.T) { - inst := &stubInstaller{afterInstall: []domain.InstalledPackage{{Name: "demo"}}} + inst := &stubInstaller{resultName: "demo", afterInstall: []domain.InstalledPackage{{Name: "demo"}}} manager := newManager(inst, &stubAgentService{}, t.TempDir()) job, err := manager.StartInstall("https://github.com/owner/repo", false) if err != nil { @@ -120,6 +129,70 @@ func TestInstallSucceedsAndDiscoversPackageName(t *testing.T) { } } +func TestInstallUsesAuthoritativeNameWhenRegistrySetDoesNotChange(t *testing.T) { + inst := &stubInstaller{ + resultName: "shared-name", + installed: []domain.InstalledPackage{{Name: "shared-name"}}, + afterInstall: []domain.InstalledPackage{{Name: "shared-name"}}, + } + manager := newManager(inst, &stubAgentService{}, t.TempDir()) + job, err := manager.StartInstall("https://github.com/owner/predecessor", false) + if err != nil { + t.Fatal(err) + } + got := waitForJob(t, manager, job.ID) + if got.PackageName != "shared-name" { + t.Fatalf("package name = %q, want shared-name", got.PackageName) + } + if got.Lines[len(got.Lines)-1] != "install completed: shared-name" { + t.Fatalf("completion line = %q", got.Lines[len(got.Lines)-1]) + } +} + +func TestInstallUsesRedirectSuccessorName(t *testing.T) { + inst := &stubInstaller{ + resultName: "successor", + afterInstall: []domain.InstalledPackage{{Name: "successor"}}, + } + manager := newManager(inst, &stubAgentService{}, t.TempDir()) + job, _ := manager.StartInstall("https://github.com/owner/predecessor", false) + got := waitForJob(t, manager, job.ID) + if got.PackageName != "successor" { + t.Fatalf("package name = %q, want successor", got.PackageName) + } +} + +func TestInstallIgnoresUnrelatedConcurrentRegistryAddition(t *testing.T) { + inst := &stubInstaller{ + resultName: "job-package", + afterInstall: []domain.InstalledPackage{ + {Name: "unrelated"}, + {Name: "job-package"}, + }, + } + manager := newManager(inst, &stubAgentService{}, t.TempDir()) + job, _ := manager.StartInstall("https://github.com/owner/job", false) + got := waitForJob(t, manager, job.ID) + if got.PackageName != "job-package" { + t.Fatalf("package name = %q, want job-package", got.PackageName) + } +} + +func TestFailedInstallReportsNoNameOrCompletion(t *testing.T) { + inst := &stubInstaller{resultName: "must-not-leak", installErr: errors.New("boom")} + manager := newManager(inst, &stubAgentService{}, t.TempDir()) + job, _ := manager.StartInstall("https://github.com/owner/broken", false) + got := waitForJob(t, manager, job.ID) + if got.PackageName != "" { + t.Fatalf("failed install package name = %q", got.PackageName) + } + for _, line := range got.Lines { + if strings.HasPrefix(line, "install completed:") { + t.Fatalf("failed install claimed completion: %q", line) + } + } +} + // Contract 2: unsafe sources are rejected before a job is created. func TestInvalidSourcesCreateNoJobs(t *testing.T) { manager := newManager(&stubInstaller{}, &stubAgentService{}, t.TempDir()) @@ -220,6 +293,41 @@ func TestUpdateStopsForceInstallsAndRestarts(t *testing.T) { } } +// Contract: updating a package whose recorded source redirects (`superseded_by`) +// to a differently-named successor follows the rename. The old package is gone +// by the time the install returns, so reporting or restarting the name that went +// in would name a node that no longer exists. +func TestUpdateFollowsASupersededRename(t *testing.T) { + home := t.TempDir() + if err := os.WriteFile(filepath.Join(home, "installed.yaml"), []byte("installed:\n demo:\n source_path: https://github.com/o/repo\n"), 0600); err != nil { + t.Fatal(err) + } + var calls []string + inst := &stubInstaller{ + resultName: "demo-v2", + installed: []domain.InstalledPackage{{Name: "demo"}}, + afterInstall: []domain.InstalledPackage{{Name: "demo-v2"}}, + calls: &calls, + } + manager := newManager(inst, &stubAgentService{running: true, calls: &calls}, home) + job, err := manager.StartUpdate("demo") + if err != nil { + t.Fatal(err) + } + got := waitForJob(t, manager, job.ID) + if got.Status != StatusSucceeded { + t.Fatalf("job = %#v", got) + } + if got.PackageName != "demo-v2" { + t.Fatalf("package name = %q, want the successor", got.PackageName) + } + // Stopped under the old name (that is what was running), restarted under + // the new one (that is what is now installed). + if strings.Join(calls, ",") != "stop:demo,install,start:demo-v2" { + t.Fatalf("calls = %v", calls) + } +} + // Contract 9: progress retains only the most recent 500 lines. func TestJobLinesAreCapped(t *testing.T) { release := make(chan struct{}) From 13eed9a023e367e52ee2b78215ab0f50ea03d671 Mon Sep 17 00:00:00 2001 From: Abir Abbas Date: Tue, 4 Aug 2026 16:31:53 -0400 Subject: [PATCH 2/6] feat(desktop): name the node an install actually landed on MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Every install result in the app was phrased from the request: the catalog row's name, or the URL that was pasted. A `superseded_by:` redirect makes that a guess — the manifest at the source hands the install off to a successor, which may register under its own name. Now that the control plane reports what it installed, repeat that instead: - a pasted repo says "pr-af installed" rather than "Installed from https://github.com/Agent-Field/pr-af", which is the more useful half of the sentence and the only one that tells you what to run next; - a catalog install names the successor if it ever disagrees with the row — the two agree for every entry today (that is the invariant catalog.ts documents), so a disagreement is drift worth seeing rather than hiding behind the row's own label; - an update that followed a rename reads " replaced by " instead of claiming it updated a node that no longer exists. Each falls back to the previous wording when the control plane names nothing, so an older control plane behaves exactly as it does today. Co-Authored-By: Claude Opus 5 (1M context) --- desktop/src/main/installer.test.ts | 52 ++++++++++++++++++++++++++++ desktop/src/main/installer.ts | 54 +++++++++++++++++++++++++----- 2 files changed, 98 insertions(+), 8 deletions(-) diff --git a/desktop/src/main/installer.test.ts b/desktop/src/main/installer.test.ts index b5f658f3b..0e614cf18 100644 --- a/desktop/src/main/installer.test.ts +++ b/desktop/src/main/installer.test.ts @@ -200,6 +200,58 @@ describe('control-plane installs', () => { }) }) +// A manifest declaring `superseded_by:` redirects the install to a successor, +// which may register under its own name. The control plane reports what it +// actually installed as the job's package_name; the app must repeat that +// rather than the name it asked for. +describe('superseded installs report what actually landed', () => { + const landedAs = (name: string): CpClient => + installClient({ + watchInstallJob: vi.fn(async () => ({ + id: 'job', + source: '', + kind: 'install' as const, + status: 'succeeded' as const, + package_name: name, + lines: [] + })) + }) + + it('names the node a pasted repo actually installed, not the URL', async () => { + expect( + await installFromSource('https://github.com/Agent-Field/pr-af', () => {}, { + cpClient: landedAs('pr-af') + }) + ).toEqual({ ok: true, message: 'pr-af installed' }) + }) + + it('falls back to the source when the control plane names nothing', async () => { + expect( + await installFromSource('https://github.com/Agent-Field/pr-af', () => {}, { + cpClient: installClient() + }) + ).toEqual({ ok: true, message: 'Installed from https://github.com/Agent-Field/pr-af' }) + }) + + it('names the successor when a catalog install redirects elsewhere', async () => { + expect( + await installAgent(CATALOG[0].name, () => {}, false, { cpClient: landedAs('successor-node') }) + ).toEqual({ ok: true, message: 'successor-node installed' }) + }) + + it('reports an update that renamed the node as a replacement', async () => { + expect( + await updateAgent(CATALOG[0].name, () => {}, { cpClient: landedAs('successor-node') }) + ).toEqual({ ok: true, message: `${CATALOG[0].name} replaced by successor-node` }) + }) + + it('still reads as a plain update when the name is unchanged', async () => { + expect( + await updateAgent(CATALOG[0].name, () => {}, { cpClient: landedAs(CATALOG[0].name) }) + ).toEqual({ ok: true, message: `${CATALOG[0].name} updated` }) + }) +}) + describe('sanitizeInstallOutput', () => { it('unwraps zerolog JSON error lines to the underlying error text', () => { const line = diff --git a/desktop/src/main/installer.ts b/desktop/src/main/installer.ts index dfad4bfe6..4df0714fc 100644 --- a/desktop/src/main/installer.ts +++ b/desktop/src/main/installer.ts @@ -14,7 +14,7 @@ import { catalogEntry } from '../shared/catalog' import type { InstallResult } from '../shared/types' -import { CpApiError, createCpClient, type CpClient } from './cpClient' +import { CpApiError, createCpClient, type CpClient, type InstallJob } from './cpClient' // CSI sequences (colors, cursor movement, erase-line spinner frames) and OSC // sequences (terminal titles), per ECMA-48. Written with \u escapes so no @@ -85,10 +85,22 @@ function defaultInstallerDeps(): InstallerDeps { const UPDATE_REQUIRED = 'Control plane update required — update AgentField CLI' +/** + * The name the control plane says it actually installed, or null when it + * doesn't say. This is NOT always the name that went in: a manifest declaring + * `superseded_by:` redirects the install to a successor, which may carry its + * own name. Reporting the job's answer rather than the request's is what keeps + * the app honest about what the user now has. + */ +function installedName(job: InstallJob): string | null { + const name = job.package_name?.trim() + return name ? name : null +} + async function runInstall( source: string, onLine: (line: string) => void, - successMessage: () => string, + successMessage: (job: InstallJob) => string, deps: InstallerDeps, force?: boolean ): Promise { @@ -99,7 +111,7 @@ async function runInstall( const { job_id } = await deps.cpClient.installPackage(source, force) const job = await deps.cpClient.watchInstallJob(job_id, onLine) return job.status === 'succeeded' - ? { ok: true, message: successMessage() } + ? { ok: true, message: successMessage(job) } : { ok: false, message: job.error || job.lines.at(-1) || 'Install failed' } } catch (err) { if (err instanceof CpApiError && err.status === 404) { @@ -132,7 +144,17 @@ export function installAgent( if (!entry) { return Promise.resolve({ ok: false, message: `"${name}" is not in the install catalog` }) } - return runInstall(entry.source, onLine, () => `${name} installed`, deps, force) + // Name what landed, not what was asked for. They agree for every catalog + // entry (that is the invariant catalog.ts documents), so a disagreement here + // means the row has drifted from the manifest it redirects to — better said + // out loud than papered over with the row's own label. + return runInstall( + entry.source, + onLine, + (job) => `${installedName(job) ?? name} installed`, + deps, + force + ) } // The one host we install from. Every accepted source starts with this literal @@ -203,7 +225,15 @@ export function installFromSource( message: 'Enter a GitHub repository URL, e.g. https://github.com/org/repo (or …/repo//subdir)' }) } - return runInstall(normalized, onLine, () => `Installed from ${normalized}`, deps) + return runInstall( + normalized, + onLine, + (job) => { + const name = installedName(job) + return name ? `${name} installed` : `Installed from ${normalized}` + }, + deps + ) } /** @@ -230,9 +260,17 @@ export async function updateAgent( onLine(`Updating ${name}…`) const { job_id } = await deps.cpClient.updatePackage(name) const job = await deps.cpClient.watchInstallJob(job_id, onLine) - return job.status === 'succeeded' - ? { ok: true, message: `${name} updated` } - : { ok: false, message: job.error || job.lines.at(-1) || `Failed to update ${name}` } + if (job.status !== 'succeeded') { + return { ok: false, message: job.error || job.lines.at(-1) || `Failed to update ${name}` } + } + // An update reinstalls from the recorded source, so it can hit a + // `superseded_by:` redirect and come back as a different node. Saying + // " updated" would then name something that no longer exists. + const landed = installedName(job) + return { + ok: true, + message: landed && landed !== name ? `${name} replaced by ${landed}` : `${name} updated` + } } catch (err) { if (err instanceof CpApiError && err.status === 404) { return { ok: false, message: UPDATE_REQUIRED } From 36fed38cffacf33df0f695be1e0770e58d1b4484 Mon Sep 17 00:00:00 2001 From: Abir Abbas Date: Tue, 4 Aug 2026 16:32:06 -0400 Subject: [PATCH 3/6] refactor(catalog): one PR-AF row, and install both consolidated nodes by repo MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The catalog offered PR-AF twice — a Python row and a Go row shipping the same reasoners under a name the user had to know to type. The two were indistinguishable in the Install view except by the `-go` suffix, which is an implementation detail leaking into a product list. Agent-Field/pr-af#64 collapses them the way Agent-Field/SWE-AF#122 collapsed the SWE fleet: the root manifest redirects to `//go`, and the Go node takes the product's name. So this is one `pr-af` row, language go. Both consolidated rows now install from the bare repo URL rather than naming `//go` directly. Selecting the subdirectory would install the same node, but it skips the redirect — and the redirect is the part that carries an existing install across: it puts the successor in place first, migrates node-scoped secrets, and only then retires the predecessor. Someone who already has the Python node gets migrated by pressing Update; someone naming `//go` would only collide with it. Naming the repo and letting the manifest decide is also simply what a user can be told to type. That changes the rule both catalogs are written against, so both header comments now say the new one: an entry's `name` must equal the name the package ends up REGISTERED under once the install settles, which under a redirect is not the `name:` in the manifest at the source, and may live in a subdirectory the catalog never mentions. sec-af and cloudsecurity-af are untouched — neither ships a second implementation, so neither has anything to collapse. The SWE guard test generalizes to cover both repos: exactly one row per repo, named for the product, sourced at the bare URL, language go, and the retired implementation-suffixed name absent from the whole catalog — so a re-added row fails here instead of quietly reappearing. Co-Authored-By: Claude Opus 5 (1M context) --- control-plane/internal/cli/catalog.go | 32 ++++++++----- control-plane/internal/cli/catalog_test.go | 54 +++++++++++++++------- desktop/src/main/agentfield.test.ts | 25 ++++++---- desktop/src/shared/catalog.ts | 32 ++++++++----- 4 files changed, 93 insertions(+), 50 deletions(-) diff --git a/control-plane/internal/cli/catalog.go b/control-plane/internal/cli/catalog.go index a4a97dfca..4d7186b2c 100644 --- a/control-plane/internal/cli/catalog.go +++ b/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 ` 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", }, { diff --git a/control-plane/internal/cli/catalog_test.go b/control-plane/internal/cli/catalog_test.go index 9a4357ac4..40fc87822 100644 --- a/control-plane/internal/cli/catalog_test.go +++ b/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) } diff --git a/desktop/src/main/agentfield.test.ts b/desktop/src/main/agentfield.test.ts index dace3b2b2..7ba943702 100644 --- a/desktop/src/main/agentfield.test.ts +++ b/desktop/src/main/agentfield.test.ts @@ -563,15 +563,22 @@ describe('install catalog', () => { expect(catalogEntry('definitely-not-real')).toBeUndefined() }) - // The SWE fleet is offered as a single install, named for the product and - // sourced from the `//go` subdirectory. A second SWE row — or the old - // implementation-suffixed name creeping back in — must fail here rather than - // quietly reappear in the Install view. - it('offers the SWE fleet as one product-named entry sourced from //go', () => { - const sweEntries = CATALOG.filter((e) => e.source.includes('Agent-Field/SWE-AF')) - expect(sweEntries).toHaveLength(1) - expect(sweEntries[0].name).toBe('swe-planner') - expect(sweEntries[0].source.endsWith('//go')).toBe(true) + // A repo that ships both a Python node and its Go counterpart is offered as + // a single install, named for the product and sourced at the bare repo URL — + // the root manifest's `superseded_by:` redirect decides which node lands and + // carries an existing install across. A second row for the same repo, or the + // old implementation-suffixed name creeping back in, must fail here rather + // than quietly reappear in the Install view. + it.each([ + { repo: 'Agent-Field/SWE-AF', name: 'swe-planner', retired: 'swe-planner-go' }, + { repo: 'Agent-Field/pr-af', name: 'pr-af', retired: 'pr-af-go' } + ])('offers $name as one product-named entry sourced at the bare repo', (tc) => { + const entries = CATALOG.filter((e) => e.source.includes(tc.repo)) + expect(entries).toHaveLength(1) + expect(entries[0].name).toBe(tc.name) + expect(entries[0].source).toBe(`https://github.com/${tc.repo}`) + expect(entries[0].language).toBe('go') + expect(CATALOG.map((e) => e.name)).not.toContain(tc.retired) }) }) diff --git a/desktop/src/shared/catalog.ts b/desktop/src/shared/catalog.ts index 72cf7862d..42c39bc10 100644 --- a/desktop/src/shared/catalog.ts +++ b/desktop/src/shared/catalog.ts @@ -10,29 +10,37 @@ import type { CatalogEntry } from './types' // // What qualifies: an Agent-Field org repo is installable iff it has an // `agentfield-package.yaml` manifest — at the repo root, or in a -// subdirectory addressed with the `//` source selector (how the Go -// ports living beside their Python originals are installed). When adding an -// entry, `name` MUST equal the manifest's `name:` (the registry key after -// install — how the app detects installed state), which is often NOT the -// repo name (SWE-AF//go → swe-planner). +// subdirectory addressed with the `//` source selector. +// +// 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 ` 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 — that is how the app detects installed state. 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). export const CATALOG: CatalogEntry[] = [ { name: 'swe-planner', description: 'Software factory — turn any issue into a production-ready pull request, end to end', - source: 'https://github.com/Agent-Field/SWE-AF//go', + source: 'https://github.com/Agent-Field/SWE-AF', language: 'go' }, { name: 'pr-af', description: 'Code review — deep, evidence-backed review of any GitHub pull request', source: 'https://github.com/Agent-Field/pr-af', - language: 'python' - }, - { - name: 'pr-af-go', - description: 'Code review — deep, evidence-backed review of any GitHub pull request', - source: 'https://github.com/Agent-Field/pr-af//go', language: 'go' }, { From d4033dbd75724df542ed9f950fca3e3a4d765013 Mon Sep 17 00:00:00 2001 From: Abir Abbas Date: Tue, 4 Aug 2026 16:32:14 -0400 Subject: [PATCH 4/6] docs(skills): the PR review node is pr-af, not pr-af-go MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The agentfield-use skill is what a harness reads to learn how to call the nodes on this machine, and skillkit installs it into Claude Code, Codex, Cursor and the rest — so its examples are the ids an agent will actually try. Its `executions/active` sample still showed a run targeting `pr-af-go`, a name that stops existing once Agent-Field/pr-af#64 lands. Applied identically to the embedded copy under skillkit/skill_data so the two stay byte-identical. Co-Authored-By: Claude Opus 5 (1M context) --- .../internal/skillkit/skill_data/agentfield-use/SKILL.md | 2 +- skills/agentfield-use/SKILL.md | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/control-plane/internal/skillkit/skill_data/agentfield-use/SKILL.md b/control-plane/internal/skillkit/skill_data/agentfield-use/SKILL.md index fd4d2c057..1e54edae2 100644 --- a/control-plane/internal/skillkit/skill_data/agentfield-use/SKILL.md +++ b/control-plane/internal/skillkit/skill_data/agentfield-use/SKILL.md @@ -193,7 +193,7 @@ are running something"): ```bash curl -s http://localhost:8080/api/v1/executions/active -# {"count":2,"runs":[{"run_id":"...","target":"pr-af-go.review","root_status":"running", +# {"count":2,"runs":[{"run_id":"...","target":"pr-af.review","root_status":"running", # "active_executions":4,"total_executions":27,"started_at":"...","latest_activity":"..."}]} ``` diff --git a/skills/agentfield-use/SKILL.md b/skills/agentfield-use/SKILL.md index fd4d2c057..1e54edae2 100644 --- a/skills/agentfield-use/SKILL.md +++ b/skills/agentfield-use/SKILL.md @@ -193,7 +193,7 @@ are running something"): ```bash curl -s http://localhost:8080/api/v1/executions/active -# {"count":2,"runs":[{"run_id":"...","target":"pr-af-go.review","root_status":"running", +# {"count":2,"runs":[{"run_id":"...","target":"pr-af.review","root_status":"running", # "active_executions":4,"total_executions":27,"started_at":"...","latest_activity":"..."}]} ``` From 3020e08fd56316848b860c45fff88eb24a43a574 Mon Sep 17 00:00:00 2001 From: Abir Abbas Date: Tue, 4 Aug 2026 16:39:22 -0400 Subject: [PATCH 5/6] test(packages): pin that the production installer can report what it installed MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The job reaches the authoritative name through a type assertion, and a failed assertion is silent — it falls back to inferring the name from a registry diff, which is exactly the path that returns nothing for an in-place `superseded_by` replacement. Every other test in this file uses a stub that satisfies the interface by construction, so none of them would notice a production wiring change (a decorator, a swapped implementation) that quietly reverted the fix. This one asserts against the service the server actually constructs. Co-Authored-By: Claude Opus 5 (1M context) --- .../services/packagejobs/manager_test.go | 16 ++++++++++++++++ 1 file changed, 16 insertions(+) diff --git a/control-plane/internal/services/packagejobs/manager_test.go b/control-plane/internal/services/packagejobs/manager_test.go index 2d07a711f..1548046db 100644 --- a/control-plane/internal/services/packagejobs/manager_test.go +++ b/control-plane/internal/services/packagejobs/manager_test.go @@ -11,9 +11,25 @@ import ( "time" "github.com/Agent-Field/agentfield/control-plane/internal/core/domain" + coreservices "github.com/Agent-Field/agentfield/control-plane/internal/core/services" infrastorage "github.com/Agent-Field/agentfield/control-plane/internal/infrastructure/storage" ) +// Contract: the installer the server actually runs reports the name it +// installed. `run` reaches that behaviour through a type assertion, which fails +// *silently* — it falls back to inferring the name from a registry diff, the +// very thing that returns nothing for an in-place `superseded_by` replacement. +// Every other test here uses a stub that satisfies the interface by +// construction, so only this one would notice a production wiring (a decorator, +// a swapped implementation) that quietly drops back to the broken path. +func TestProductionInstallerReportsTheNameItInstalled(t *testing.T) { + var service installer = coreservices.NewPackageService(nil, infrastorage.NewFileSystemAdapter(), t.TempDir()) + if _, ok := service.(resultInstaller); !ok { + t.Fatalf("%T cannot report what it installed — install jobs would silently "+ + "fall back to the registry diff and report no name for an in-place supersede", service) + } +} + type stubInstaller struct { mu sync.Mutex installErr error From 4ffbe17b52ac3c4260beb4728ff2485bc798a0cd Mon Sep 17 00:00:00 2001 From: Abir Abbas Date: Tue, 4 Aug 2026 17:06:01 -0400 Subject: [PATCH 6/6] fix(packages): a node-dependency cycle must terminate MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Switching dependency discovery from a registry snapshot to the authoritative installed name dropped the only thing that stopped a cycle. The snapshot version terminated by accident but reliably: the recursive call received a snapshot that already contained the package just reinstalled, so the second lap skipped it. Recursing on a single name removed that, and the remaining guard — `depName != "" && isPackageInstalled(depName)` — cannot substitute. It only knows a dependency's name for `af://registry/…` refs, and a forced install reinstalls whatever is already there. Every update is forced (`StartUpdate` → `startJob(JobUpdate, …, true)`), so two packages declaring each other by bare git URL or local path recursed until the process died — with the package-job manager's `active` latch held, blocking every later install. Tracks the packages this install pass has walked instead, which does not depend on ref form, on Force, or on registry state. The accompanying suite pins the seam's behaviour end to end through the real git installer rather than a stub: a redirect reports the successor — including when the successor takes the predecessor's own name, the case a registry diff cannot see and the reason this seam exists — a failed install reports no name at each stage it can fail, an uninstallable dependency does not fail its parent, and a cycle terminates. That last one fails in 30s against this fix reverted. `manager_test.go` covers the other side: an installer that cannot report a name still installs and falls back to the registry diff, so the old path stays intact for anything that does not implement the newer seam. Co-Authored-By: Claude Opus 5 (1M context) --- .../core/services/install_result_test.go | 212 ++++++++++++++++++ .../internal/core/services/node_deps_test.go | 2 +- .../internal/core/services/package_service.go | 20 +- .../services/packagejobs/manager_test.go | 25 +++ 4 files changed, 253 insertions(+), 6 deletions(-) create mode 100644 control-plane/internal/core/services/install_result_test.go diff --git a/control-plane/internal/core/services/install_result_test.go b/control-plane/internal/core/services/install_result_test.go new file mode 100644 index 000000000..b71f6bf1e --- /dev/null +++ b/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"]) +} diff --git a/control-plane/internal/core/services/node_deps_test.go b/control-plane/internal/core/services/node_deps_test.go index 6232a6f2d..cfd5cfdcb 100644 --- a/control-plane/internal/core/services/node_deps_test.go +++ b/control-plane/internal/core/services/node_deps_test.go @@ -75,7 +75,7 @@ func TestInstallNodeDependencies_SkipsAlreadyInstalled(t *testing.T) { }, }) - err := ps.installNodeDependencies("caller", domain.InstallOptions{}) + err := ps.installNodeDependencies("caller", domain.InstallOptions{}, map[string]bool{"caller": true}) require.NoError(t, err) } diff --git a/control-plane/internal/core/services/package_service.go b/control-plane/internal/core/services/package_service.go index 99ae2f4bd..18b019deb 100644 --- a/control-plane/internal/core/services/package_service.go +++ b/control-plane/internal/core/services/package_service.go @@ -57,7 +57,7 @@ func (ps *DefaultPackageService) InstallPackageWithResult(source string, options // into recursive dependency installs. depOptions := options depOptions.Path = "" - if err := ps.installNodeDependencies(installedName, depOptions); err != nil { + if err := ps.installNodeDependencies(installedName, depOptions, map[string]bool{installedName: true}); err != nil { return "", err } return installedName, nil @@ -96,9 +96,15 @@ func (ps *DefaultPackageService) installedNames() map[string]bool { } // installNodeDependencies installs the node-to-node dependencies declared by -// packageName, recursively. Already-installed nodes are skipped, which also -// breaks dependency cycles. -func (ps *DefaultPackageService) installNodeDependencies(packageName string, options domain.InstallOptions) error { +// packageName, recursively. +// +// `visited` holds every package this install pass has already walked, and it is +// what terminates a dependency cycle. The already-installed check below cannot +// do that on its own: it only knows a dependency's name for `af://registry/…` +// refs, and a forced install — which every update is — reinstalls whatever is +// already there. So a cycle expressed with bare git URLs or local paths has +// nothing else stopping it. +func (ps *DefaultPackageService) installNodeDependencies(packageName string, options domain.InstallOptions, visited map[string]bool) error { registry, err := ps.loadRegistryDirect() if err != nil { return nil // base install already succeeded; don't fail on dep discovery @@ -123,8 +129,12 @@ func (ps *DefaultPackageService) installNodeDependencies(packageName string, opt fmt.Printf("%s Failed to install node dependency %s: %v\n", ps.statusError(), dep, err) continue } + if visited[installedName] { + continue // a cycle: this pass has already walked that package + } + visited[installedName] = true // Recurse for the dependency's own node deps. - if err := ps.installNodeDependencies(installedName, options); err != nil { + if err := ps.installNodeDependencies(installedName, options, visited); err != nil { return err } } diff --git a/control-plane/internal/services/packagejobs/manager_test.go b/control-plane/internal/services/packagejobs/manager_test.go index 1548046db..c80d0ec80 100644 --- a/control-plane/internal/services/packagejobs/manager_test.go +++ b/control-plane/internal/services/packagejobs/manager_test.go @@ -15,6 +15,31 @@ import ( infrastorage "github.com/Agent-Field/agentfield/control-plane/internal/infrastructure/storage" ) +// Contract: an installer that cannot report a name still installs, and the job +// falls back to inferring the name from the registry. That fallback is the +// pre-existing behaviour and has to keep working — the authoritative path is an +// upgrade, not a requirement. +func TestInstallFallsBackWhenInstallerCannotReportAName(t *testing.T) { + inst := &stubInstaller{afterInstall: []domain.InstalledPackage{{Name: "legacy-node"}}} + // Embedding in an anonymous struct exposes only `installer`, hiding the + // stub's InstallPackageWithResult — so the manager sees an implementation + // that cannot report results and must take the fallback. + var plain installer = struct{ installer }{inst} + manager := newManager(plain, &stubAgentService{}, t.TempDir()) + + job, err := manager.StartInstall("https://github.com/owner/repo", false) + if err != nil { + t.Fatal(err) + } + got := waitForJob(t, manager, job.ID) + if got.Status != StatusSucceeded { + t.Fatalf("job = %#v", got) + } + if got.PackageName != "legacy-node" { + t.Fatalf("package name = %q, want the name inferred from the registry", got.PackageName) + } +} + // Contract: the installer the server actually runs reports the name it // installed. `run` reaches that behaviour through a type assertion, which fails // *silently* — it falls back to inferring the name from a registry diff, the