diff --git a/pkg/behaviourtest/steps/base_dispatch.go b/pkg/behaviourtest/steps/base_dispatch.go index b867acd9ae..598fff95ee 100644 --- a/pkg/behaviourtest/steps/base_dispatch.go +++ b/pkg/behaviourtest/steps/base_dispatch.go @@ -222,6 +222,11 @@ func givenCustomHarnessWithURLBase(w *world.World, name, baseName, doc string) e // update boilerplate shared by givenCustomHarnessWithLocalBase and // givenCustomHarnessWithURLBase. func registerLocalAgentConfig(ctx context.Context, w *world.World, name, commitMsg string) error { + // Snapshot agents before modification so CleanupScenario can restore. + if err := snapshotAgents(w); err != nil { + return fmt.Errorf("snapshotting agents: %w", err) + } + cfgPath := path.Join(".fullsend", "config.yaml") cfgData, err := w.SCM.GetFileContent(ctx, w.Org, w.RepoName, cfgPath) if err != nil { diff --git a/pkg/behaviourtest/steps/cleanup.go b/pkg/behaviourtest/steps/cleanup.go index 0747ce6a40..198d542574 100644 --- a/pkg/behaviourtest/steps/cleanup.go +++ b/pkg/behaviourtest/steps/cleanup.go @@ -231,6 +231,19 @@ func CleanupScenario(w *world.World) { } } + // --- Agents cleanup --- + // Restore the pre-scenario agents list so a later scenario on this + // slot does not inherit custom agent entries (local or URL-sourced) + // from the previous lessee. Without this, harness registrations + // accumulate on the config overlay for the rest of the run. + if w.AgentsOverridden { + if err := cleanupRetry(w.Logf, "restore agents", func() error { + return RestoreAgents(w) + }); err != nil { + worldLogf(w, "behaviour cleanup: restore agents: %v", err) + } + } + // --- Dummy script cleanup --- if len(w.DummyOps) > 0 { if w.Org == "" || w.RepoName == "" { diff --git a/pkg/behaviourtest/steps/cleanup_test.go b/pkg/behaviourtest/steps/cleanup_test.go index 44f1905dad..bd18e2518c 100644 --- a/pkg/behaviourtest/steps/cleanup_test.go +++ b/pkg/behaviourtest/steps/cleanup_test.go @@ -775,6 +775,62 @@ func TestCleanupScenario_RestoreAllowedResources_Error(t *testing.T) { assert.Contains(t, logged[0], "restore allowed_remote_resources") } +// --- Agents cleanup tests --- + +func TestCleanupScenario_RestoresAgents(t *testing.T) { + t.Parallel() + + scmDriver := &fakeCleanupSCM{ + fileContent: []byte("version: \"1\"\nagents:\n - name: custom\n source: harness/custom.yaml\nroles:\n - triage\n"), + } + w := &world.World{ + Org: "org", + RepoOwner: "org", + RepoName: "repo", + AgentsOverridden: true, + AgentsOriginal: nil, // restore to empty (install-time default) + SCM: scmDriver, + } + CleanupScenario(w) + assert.True(t, scmDriver.commitFileCalled, "should commit config to restore agents") +} + +func TestCleanupScenario_SkipsAgentsWhenNotOverridden(t *testing.T) { + t.Parallel() + + scmDriver := &fakeCleanupSCM{} + w := &world.World{ + RepoOwner: "org", + RepoName: "repo", + AgentsOverridden: false, + SCM: scmDriver, + } + CleanupScenario(w) + assert.False(t, scmDriver.commitFileCalled, "should not commit when agents were not overridden") +} + +func TestCleanupScenario_RestoreAgents_Error(t *testing.T) { + t.Parallel() + + var logged []string + scmDriver := &fakeCleanupSCM{ + fileContent: []byte("version: \"1\"\nagents:\n - name: custom\n source: harness/custom.yaml\nroles:\n - triage\n"), + commitFileErr: fmt.Errorf("commit failed"), + } + w := &world.World{ + Org: "org", + RepoOwner: "org", + RepoName: "repo", + AgentsOverridden: true, + AgentsOriginal: nil, + SCM: scmDriver, + Logf: func(format string, args ...any) { logged = append(logged, fmt.Sprintf(format, args...)) }, + } + CleanupScenario(w) + require.Len(t, logged, 1) + assert.Contains(t, logged[0], "restore agents") +} + func TestCleanupScenario_BranchScenarioSweep(t *testing.T) { t.Parallel() diff --git a/pkg/behaviourtest/steps/dispatch.go b/pkg/behaviourtest/steps/dispatch.go index 06911f605e..cf09034305 100644 --- a/pkg/behaviourtest/steps/dispatch.go +++ b/pkg/behaviourtest/steps/dispatch.go @@ -117,6 +117,11 @@ func givenDisabledCustomHarness(w *world.World, name, doc string) error { return err } + // Snapshot agents before modification so CleanupScenario can restore. + if err := snapshotAgents(w); err != nil { + return fmt.Errorf("snapshotting agents: %w", err) + } + cfgPath := filepath.Join(".fullsend", "config.yaml") cfgData, err := w.SCM.GetFileContent(context.Background(), w.Org, w.RepoName, cfgPath) if err != nil { @@ -171,6 +176,11 @@ func givenCustomHarness(w *world.World, name, doc string) error { return err } + // Snapshot agents before modification so CleanupScenario can restore. + if err := snapshotAgents(w); err != nil { + return fmt.Errorf("snapshotting agents: %w", err) + } + cfgPath := filepath.Join(".fullsend", "config.yaml") cfgData, err := w.SCM.GetFileContent(context.Background(), w.Org, w.RepoName, cfgPath) if err != nil { diff --git a/pkg/behaviourtest/steps/url_dispatch.go b/pkg/behaviourtest/steps/url_dispatch.go index aed6ac2c48..1126823c5c 100644 --- a/pkg/behaviourtest/steps/url_dispatch.go +++ b/pkg/behaviourtest/steps/url_dispatch.go @@ -68,6 +68,35 @@ func snapshotAllowedResources(w *world.World) error { return nil } +// snapshotAgents captures the current agents list from config.yaml into +// w.AgentsOriginal — but only on the first call per scenario, so that +// multiple harness steps in the same scenario do not overwrite the +// original value with an already-modified copy. +// +// Called before any cfg.SetAgents() call (dispatch.go, url_dispatch.go, +// base_dispatch.go). CleanupScenario uses the snapshot to restore the +// original agents when the scenario is done. +func snapshotAgents(w *world.World) error { + if w.AgentsOverridden { + return nil // already snapshotted this scenario + } + cfgPath := path.Join(".fullsend", "config.yaml") + cfgData, err := w.SCM.GetFileContent(context.Background(), w.Org, w.RepoName, cfgPath) + if err != nil { + return fmt.Errorf("reading config for agents snapshot: %w", err) + } + cfg, err := config.ParsePerRepoConfigWriter(cfgData) + if err != nil { + return fmt.Errorf("parsing config for agents snapshot: %w", err) + } + orig := cfg.AgentEntries() + // Store a copy so later mutations do not alias the snapshot. + w.AgentsOriginal = make([]config.AgentEntry, len(orig)) + copy(w.AgentsOriginal, orig) + w.AgentsOverridden = true + return nil +} + // RestoreAllowedResources sets allowed_remote_resources back to the // pre-scenario value captured by snapshotAllowedResources. Exported so // CleanupScenario can call it during scenario teardown. @@ -95,6 +124,33 @@ func RestoreAllowedResources(w *world.World) error { return nil } +// RestoreAgents sets the agents list back to the pre-scenario value +// captured by snapshotAgents. Exported so CleanupScenario can call it +// during scenario teardown. +func RestoreAgents(w *world.World) error { + if w.Org == "" || w.RepoName == "" { + return fmt.Errorf("no repo configured; call 'Given the enrolled test repository' before agent operations") + } + cfgPath := path.Join(".fullsend", "config.yaml") + cfgData, err := w.SCM.GetFileContent(context.Background(), w.Org, w.RepoName, cfgPath) + if err != nil { + return fmt.Errorf("reading config: %w", err) + } + cfg, err := config.ParsePerRepoConfigWriter(cfgData) + if err != nil { + return fmt.Errorf("parsing config: %w", err) + } + cfg.SetAgents(w.AgentsOriginal) + merged, err := cfg.Marshal() + if err != nil { + return err + } + if err := w.SCM.CommitFile(context.Background(), w.Org, w.RepoName, cfgPath, "behaviour: restore agents", merged); err != nil { + return fmt.Errorf("updating config: %w", err) + } + return nil +} + // givenHarnessHostingRepo creates a public repository to host URL-sourced // harness YAML files. The repo is created in the same org as the test // repository. It is idempotent — if the repo already exists, it returns @@ -248,11 +304,15 @@ func givenURLSourcedCustomHarness(w *world.World, name, doc string, opts urlHarn // Build the URL prefix for the allowlist. urlPrefix := fmt.Sprintf("https://raw.githubusercontent.com/%s/%s/", hostOwner, hostRepo) - // Snapshot the current allowed_remote_resources before any modification - // so CleanupScenario can restore it when the slot is reused. + // Snapshot the current allowed_remote_resources and agents before any + // modification so CleanupScenario can restore them when the slot is + // reused. if err := snapshotAllowedResources(w); err != nil { return fmt.Errorf("snapshotting allowed_remote_resources: %w", err) } + if err := snapshotAgents(w); err != nil { + return fmt.Errorf("snapshotting agents: %w", err) + } // Update config.yaml on the enrolled test repo: register agent with URL // source and update allowlist. diff --git a/pkg/behaviourtest/steps/url_dispatch_test.go b/pkg/behaviourtest/steps/url_dispatch_test.go index e14f028858..009bfed12f 100644 --- a/pkg/behaviourtest/steps/url_dispatch_test.go +++ b/pkg/behaviourtest/steps/url_dispatch_test.go @@ -899,6 +899,122 @@ func TestGivenURLSourcedCustomHarness_SnapshotsAllowlist(t *testing.T) { "snapshot should not contain the newly added prefix") } +// --- snapshotAgents / RestoreAgents tests --- + +func TestSnapshotAgents_CapturesOriginal(t *testing.T) { + t.Parallel() + scm := &fakeURLSCM{files: map[string][]byte{ + "org/repo/.fullsend/config.yaml": []byte("version: \"1\"\nagents:\n - name: triage\n source: harness/triage.yaml\n"), + }} + w := &world.World{ + Org: "org", + RepoName: "repo", + SCM: scm, + } + err := snapshotAgents(w) + require.NoError(t, err) + assert.True(t, w.AgentsOverridden) + require.Len(t, w.AgentsOriginal, 1) + assert.Equal(t, "triage", w.AgentsOriginal[0].Name) + assert.Equal(t, "harness/triage.yaml", w.AgentsOriginal[0].Source) +} + +func TestSnapshotAgents_OnlySnapshotsOnce(t *testing.T) { + t.Parallel() + scm := &fakeURLSCM{files: map[string][]byte{ + "org/repo/.fullsend/config.yaml": []byte("version: \"1\"\nagents:\n - name: triage\n source: harness/triage.yaml\n"), + }} + w := &world.World{ + Org: "org", + RepoName: "repo", + SCM: scm, + } + err := snapshotAgents(w) + require.NoError(t, err) + require.Len(t, w.AgentsOriginal, 1) + + // Mutate the stored snapshot to verify it isn't overwritten on second call. + w.AgentsOriginal = nil + err = snapshotAgents(w) + require.NoError(t, err) + assert.Nil(t, w.AgentsOriginal, + "second call should not re-snapshot") +} + +func TestSnapshotAgents_EmptyAgentsList(t *testing.T) { + t.Parallel() + scm := &fakeURLSCM{files: map[string][]byte{ + "org/repo/.fullsend/config.yaml": []byte("version: \"1\"\nagents: []\n"), + }} + w := &world.World{ + Org: "org", + RepoName: "repo", + SCM: scm, + } + err := snapshotAgents(w) + require.NoError(t, err) + assert.True(t, w.AgentsOverridden) + assert.Empty(t, w.AgentsOriginal) +} + +func TestRestoreAgents_RestoresOriginal(t *testing.T) { + t.Parallel() + scm := &fakeURLSCM{files: map[string][]byte{ + "org/repo/.fullsend/config.yaml": []byte("version: \"1\"\nagents:\n - name: custom\n source: harness/custom.yaml\n"), + }} + original := []config.AgentEntry{{Name: "triage", Source: "harness/triage.yaml"}} + w := &world.World{ + Org: "org", + RepoName: "repo", + SCM: scm, + AgentsOriginal: original, + } + err := RestoreAgents(w) + require.NoError(t, err) + + // Parse the committed config and verify agents were restored. + cfgData := scm.files["org/repo/.fullsend/config.yaml"] + require.NotNil(t, cfgData) + assert.Contains(t, string(cfgData), "triage") + assert.NotContains(t, string(cfgData), "custom") +} + +func TestRestoreAgents_EmptyOrg(t *testing.T) { + t.Parallel() + w := &world.World{ + RepoName: "repo", + SCM: &fakeURLSCM{files: map[string][]byte{}}, + } + err := RestoreAgents(w) + require.Error(t, err) + assert.Contains(t, err.Error(), "no repo configured") +} + +func TestGivenURLSourcedCustomHarness_SnapshotsAgents(t *testing.T) { + stubRawHTTPClient(t) + scm := &fakeURLSCM{files: map[string][]byte{ + "my-org/my-repo/.fullsend/config.yaml": []byte( + "version: \"1\"\nagents:\n - name: existing\n source: harness/existing.yaml\nallowed_remote_resources:\n - \"https://example.com/\"\n"), + }} + w := &world.World{ + Org: "my-org", + RepoName: "my-repo", + SCM: scm, + URLHarnessRepoOwner: "my-org", + URLHarnessRepoName: "harness-host", + } + + err := givenURLSourcedCustomHarness(w, "url-test", + "agent: agents/triage.md\nrole: triage\nslug: url-test", urlHarnessOpts{}) + require.NoError(t, err) + + assert.True(t, w.AgentsOverridden, + "AgentsOverridden should be set after URL harness step") + require.Len(t, w.AgentsOriginal, 1) + assert.Equal(t, "existing", w.AgentsOriginal[0].Name, + "original agents should be preserved in snapshot") +} + // --- fakes --- // fakeURLSCM keys files by "owner/repo/path" so multi-repo tests diff --git a/pkg/behaviourtest/suite/init.go b/pkg/behaviourtest/suite/init.go index ee71ab201e..a5be4cb9a2 100644 --- a/pkg/behaviourtest/suite/init.go +++ b/pkg/behaviourtest/suite/init.go @@ -100,6 +100,8 @@ func resetScenarioWorld(w *world.World) { w.RuntimeOriginal = "" w.AllowedResourcesOverridden = false w.AllowedResourcesOriginal = nil + w.AgentsOverridden = false + w.AgentsOriginal = nil w.JiraMockServer = nil w.JiraMockState = nil w.JiraConfigDir = "" diff --git a/pkg/behaviourtest/suite/init_test.go b/pkg/behaviourtest/suite/init_test.go index 630eb09e18..903a49d5d8 100644 --- a/pkg/behaviourtest/suite/init_test.go +++ b/pkg/behaviourtest/suite/init_test.go @@ -12,6 +12,7 @@ import ( "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" + "github.com/fullsend-ai/fullsend/internal/config" "github.com/fullsend-ai/fullsend/internal/forge" "github.com/fullsend-ai/fullsend/pkg/behaviourtest/drivers/env" "github.com/fullsend-ai/fullsend/pkg/behaviourtest/drivers/install" @@ -149,6 +150,8 @@ func TestResetScenarioWorld_ClearsSharedState(t *testing.T) { URLHarnessRepoName: "harness-host", AllowedResourcesOverridden: true, AllowedResourcesOriginal: []string{"https://example.com/"}, + AgentsOverridden: true, + AgentsOriginal: []config.AgentEntry{{Name: "test", Source: "harness/test.yaml"}}, } resetScenarioWorld(w) assert.Equal(t, 0, w.PRNumber) @@ -164,6 +167,8 @@ func TestResetScenarioWorld_ClearsSharedState(t *testing.T) { assert.Equal(t, "", w.URLHarnessRepoName) assert.False(t, w.AllowedResourcesOverridden) assert.Nil(t, w.AllowedResourcesOriginal) + assert.False(t, w.AgentsOverridden) + assert.Nil(t, w.AgentsOriginal) } func TestSkipErrorForTagNames(t *testing.T) { diff --git a/pkg/behaviourtest/world/world.go b/pkg/behaviourtest/world/world.go index 8850080a23..3d351f8473 100644 --- a/pkg/behaviourtest/world/world.go +++ b/pkg/behaviourtest/world/world.go @@ -5,6 +5,7 @@ import ( "path/filepath" "time" + "github.com/fullsend-ai/fullsend/internal/config" "github.com/fullsend-ai/fullsend/internal/forge" "github.com/fullsend-ai/fullsend/internal/runtime" "github.com/fullsend-ai/fullsend/pkg/behaviourtest/drivers/ci" @@ -105,6 +106,13 @@ type World struct { AllowedResourcesOverridden bool AllowedResourcesOriginal []string + // AgentsOverridden records that this scenario modified the agents + // list in config.yaml; AgentsOriginal holds the pre-scenario value. + // CleanupScenario restores it so the next scenario on this slot + // does not inherit custom agent entries from the previous lessee. + AgentsOverridden bool + AgentsOriginal []config.AgentEntry + // Jira mock state — set by the "Given a mock Jira server" step. JiraMockServer *httptest.Server JiraMockState *jiramock.State