From 0937bebf325829fb1ad676306ccb26ac32ab03ec Mon Sep 17 00:00:00 2001 From: Wayne Sun Date: Thu, 3 Sep 2026 11:44:20 -0400 Subject: [PATCH 01/17] feat(#6966): add role table, trigger presets and spec parsing for agent new MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Groundwork for `fullsend agent new`: the three pure, network-free pieces the generator is built on. The role table is hardcoded rather than derived from mintcore.BuiltInRoles(). Derivation would re-admit `scribe`, which config.ValidRoles() deliberately excludes as a mint-only dogfood role that "must not silently pass config validation", and it would fail open for any future canonical role with no provider pairing. Drift is caught by a test asserting each role is in both BuiltInRoles() and ValidRoles() and that its permissions equal mintcore.RolePermissionsFor(role) — BuiltInRoles rather than HasRole, because HasRole also returns true for standalone-mint custom roles registered at runtime. Trigger presets are pinned by string equality, not just compiled. An expression can compile and still touch an absent optional field, which fails only at first dispatch — MatchHarnesses turns that into a red ::error:: annotation on every matching event. The `command` preset guards fork pull requests with has() rather than the reference doc's `!= null`: state.change_proposal is absent, not null, on a non-PR comment, so `!= null` raises a missing-key error on every issue comment. Assisted-by: Claude Signed-off-by: Wayne Sun --- internal/agentnew/roles.go | 147 +++++++++++++++++++++++++++++ internal/agentnew/roles_test.go | 128 ++++++++++++++++++++++++++ internal/agentnew/spec.go | 77 ++++++++++++++++ internal/agentnew/spec_test.go | 82 +++++++++++++++++ internal/agentnew/trigger.go | 116 +++++++++++++++++++++++ internal/agentnew/trigger_test.go | 148 ++++++++++++++++++++++++++++++ internal/config/config.go | 7 ++ 7 files changed, 705 insertions(+) create mode 100644 internal/agentnew/roles.go create mode 100644 internal/agentnew/roles_test.go create mode 100644 internal/agentnew/spec.go create mode 100644 internal/agentnew/spec_test.go create mode 100644 internal/agentnew/trigger.go create mode 100644 internal/agentnew/trigger_test.go diff --git a/internal/agentnew/roles.go b/internal/agentnew/roles.go new file mode 100644 index 0000000000..236feb5e51 --- /dev/null +++ b/internal/agentnew/roles.go @@ -0,0 +1,147 @@ +package agentnew + +import ( + "fmt" + "sort" + "strings" + + "github.com/fullsend-ai/fullsend/internal/config" +) + +// DefaultRole is the role assigned when --role is not given. It matches the +// role the Bring Your Own Agent guide's examples use. +const DefaultRole = "triage" + +// Role describes one mint role that `fullsend agent new` will generate for, +// together with the sandbox resources a harness needs to run under it. +// +// The table is deliberately hardcoded rather than derived from +// mintcore.BuiltInRoles(). Derivation would re-admit roles the rest of the +// CLI excludes on purpose — config.ValidRoles() documents that mint-only +// dogfood roles such as scribe "must not silently pass config validation" — +// and it would fail open for any future canonical role that has no provider +// pairing here. RoleTableMatchesMint (roles_test.go) fails CI if the mint +// and this table drift apart. +type Role struct { + // Name is the harness `role:` value and the mint role. + Name string + // Permissions mirrors mintcore's canonicalRolePermissions for this role. + // It is reproduced here so the CLI can explain what a role grants + // without importing the mint's internals into its help text. + Permissions map[string]string + // Providers are harness `providers:` entries, by path. Paths rather than + // bare names: a bare name that has no definition on disk degrades to a + // warning and then a sandbox that cannot reach Vertex, because the + // embedded provider fallback covers only the OpenAI provider. + Providers []string + // Profiles are harness `openshell.profiles:` entries, by path. + Profiles []string + // Image is the sandbox image this role's agents run under. + Image string +} + +// roleTable is the set of roles `agent new` will generate for. Excluded on +// purpose: `fix` (dispatch mints `coder` for both the code and fix stages, so +// no fix App is enrolled), `fullsend` and `e2e` (infrastructure identities), +// and `scribe` (recognised by the mint but absent from config.ValidRoles(), +// DefaultAgentRoles() and PerRepoDefaultRoles(), and its fleet harness has no +// forge provider pair to copy). +var roleTable = map[string]Role{ + "triage": { + Name: "triage", + Permissions: map[string]string{"contents": "read", "issues": "write", "metadata": "read"}, + Providers: []string{"providers/vertex-ai.yaml", "providers/github-ro.yaml"}, + Profiles: []string{"profiles/fullsend-vertex-ai.yaml", "profiles/fullsend-github-ro.yaml"}, + Image: config.DefaultSandboxImage, + }, + "review": { + Name: "review", + Permissions: map[string]string{ + "contents": "read", "pull_requests": "write", "issues": "write", + "checks": "read", "metadata": "read", + }, + Providers: []string{"providers/vertex-ai.yaml", "providers/github-ro.yaml"}, + Profiles: []string{"profiles/fullsend-vertex-ai.yaml", "profiles/fullsend-github-ro.yaml"}, + Image: config.DefaultCodeImage, + }, + "coder": { + Name: "coder", + Permissions: map[string]string{ + "contents": "write", "packages": "read", "pull_requests": "write", + "issues": "write", "checks": "read", "metadata": "read", + }, + Providers: []string{"providers/vertex-ai.yaml", "providers/github.yaml"}, + Profiles: []string{"profiles/fullsend-vertex-ai.yaml", "profiles/fullsend-github.yaml"}, + Image: config.DefaultCodeImage, + }, + "retro": { + Name: "retro", + Permissions: map[string]string{ + "actions": "read", "contents": "read", "pull_requests": "write", + "issues": "write", "metadata": "read", + }, + // retro is the only role taking two forge providers: github-ro for + // the repository and github-artifacts for workflow run artifacts. + Providers: []string{"providers/vertex-ai.yaml", "providers/github-ro.yaml", "providers/github-artifacts.yaml"}, + Profiles: []string{ + "profiles/fullsend-vertex-ai.yaml", + "profiles/fullsend-github-ro.yaml", + "profiles/fullsend-github-artifacts.yaml", + }, + Image: config.DefaultSandboxImage, + }, + "prioritize": { + Name: "prioritize", + Permissions: map[string]string{ + "contents": "read", "issues": "write", + "organization_projects": "write", "metadata": "read", + }, + Providers: []string{"providers/vertex-ai.yaml", "providers/github-ro.yaml"}, + Profiles: []string{"profiles/fullsend-vertex-ai.yaml", "profiles/fullsend-github-ro.yaml"}, + Image: config.DefaultSandboxImage, + }, +} + +// RoleNames returns the offered role names in table order. +func RoleNames() []string { + names := make([]string, 0, len(roleTable)) + for name := range roleTable { + names = append(names, name) + } + sort.Strings(names) + return names +} + +// LookupRole returns the Role for name. An unknown role is an error carrying +// the whole table, so the user sees the valid choices and their permissions +// rather than discovering the problem as an opaque 403 from the mint at first +// dispatch. +func LookupRole(name string) (Role, error) { + if r, ok := roleTable[name]; ok { + return r, nil + } + return Role{}, fmt.Errorf("unknown role %q\n\n%s", name, RoleHelp()) +} + +// RoleHelp renders the role table for error messages and --help text. +func RoleHelp() string { + var b strings.Builder + b.WriteString("The hosted mint serves these roles:\n\n") + for _, name := range RoleNames() { + r := roleTable[name] + perms := make([]string, 0, len(r.Permissions)) + for k, v := range r.Permissions { + perms = append(perms, k+":"+v) + } + sort.Strings(perms) + suffix := "" + if name == DefaultRole { + suffix = " (default)" + } + fmt.Fprintf(&b, " %-11s%s %s\n", name, suffix, strings.Join(perms, ", ")) + } + b.WriteString("\n\"scribe\" is recognised by the mint but is not wired for dispatch, so it is\n") + b.WriteString("not offered here. To use a role the hosted mint does not serve you need\n") + b.WriteString("your own mint — see docs/guides/user/custom-agent-identity.md.\n") + return b.String() +} diff --git a/internal/agentnew/roles_test.go b/internal/agentnew/roles_test.go new file mode 100644 index 0000000000..0208526ac0 --- /dev/null +++ b/internal/agentnew/roles_test.go @@ -0,0 +1,128 @@ +package agentnew + +import ( + "reflect" + "slices" + "strings" + "testing" + + "github.com/fullsend-ai/fullsend/internal/config" + "github.com/fullsend-ai/fullsend/internal/mintcore" +) + +// TestRoleTableMatchesMint is the anti-drift gate that lets roleTable stay +// hardcoded. Deriving the table from mintcore.BuiltInRoles() would re-admit +// roles the rest of the CLI excludes on purpose; this test instead fails CI +// if the mint's permissions and this table stop agreeing. +func TestRoleTableMatchesMint(t *testing.T) { + builtIn := mintcore.BuiltInRoles() + valid := config.ValidRoles() + + for _, name := range RoleNames() { + t.Run(name, func(t *testing.T) { + role, err := LookupRole(name) + if err != nil { + t.Fatalf("LookupRole(%q): %v", name, err) + } + // BuiltInRoles, not HasRole: HasRole returns a unified view that + // includes standalone-mint custom roles registered at runtime, + // so it would pass for a role the hosted mint does not serve. + if !slices.Contains(builtIn, name) { + t.Errorf("role %q is not in mintcore.BuiltInRoles() %v", name, builtIn) + } + if !slices.Contains(valid, name) { + t.Errorf("role %q is not in config.ValidRoles() %v", name, valid) + } + if want := mintcore.RolePermissionsFor(name); !reflect.DeepEqual(role.Permissions, want) { + t.Errorf("permissions drifted from the mint\n got: %v\nwant: %v", role.Permissions, want) + } + if role.Name != name { + t.Errorf("roleTable[%q].Name = %q", name, role.Name) + } + if role.Image == "" { + t.Error("role has no image") + } + if len(role.Providers) != len(role.Profiles) { + t.Errorf("each provider needs a matching profile: %d providers, %d profiles", + len(role.Providers), len(role.Profiles)) + } + }) + } +} + +// TestExcludedRolesAreRejected pins the roles agent new must never offer. +// Each is excluded for a different reason and a regression on any of them +// produces an opaque 403 from the mint at first dispatch rather than a +// config error, so they are asserted individually. +func TestExcludedRolesAreRejected(t *testing.T) { + for _, name := range []string{"fix", "fullsend", "e2e", "scribe", "", "Triage", "nonsense"} { + t.Run("role="+name, func(t *testing.T) { + if _, err := LookupRole(name); err == nil { + t.Fatalf("LookupRole(%q) succeeded; it must be rejected", name) + } else if !strings.Contains(err.Error(), "triage") { + t.Errorf("error should print the role table, got: %v", err) + } + }) + } +} + +// TestRoleHelpNamesScribe: a user who copies the fleet's scribe harness needs +// a direct answer, since scribe IS a real mint role but is not wired for +// dispatch. fix and fullsend are deliberately not mentioned. +func TestRoleHelpNamesScribe(t *testing.T) { + help := RoleHelp() + if !strings.Contains(help, "scribe") { + t.Error("role help should explain why scribe is not offered") + } + for _, hidden := range []string{"fix", "fullsend", "e2e"} { + if strings.Contains(help, hidden) { + t.Errorf("role help should not mention %q", hidden) + } + } + for _, name := range RoleNames() { + if !strings.Contains(help, name) { + t.Errorf("role help omits %q", name) + } + } +} + +func TestRetroIsTheOnlyTwoForgeProviderRole(t *testing.T) { + for _, name := range RoleNames() { + role, err := LookupRole(name) + if err != nil { + t.Fatal(err) + } + forge := 0 + for _, p := range role.Providers { + if strings.Contains(p, "github") { + forge++ + } + } + want := 1 + if name == "retro" { + want = 2 + } + if forge != want { + t.Errorf("role %q has %d forge providers, want %d (%v)", name, forge, want, role.Providers) + } + } +} + +// TestCoderUsesEmbeddedGithubProvider guards the one provider-name trap: the +// fleet's code harness uses providers/github-code.yaml, which the embedded +// scaffold does not ship. A generated coder harness must name the bare +// github provider that the scaffold does have. +func TestCoderUsesEmbeddedGithubProvider(t *testing.T) { + role, err := LookupRole("coder") + if err != nil { + t.Fatal(err) + } + for _, p := range role.Providers { + if strings.Contains(p, "github-code") { + t.Errorf("coder must not reference %q; the embedded scaffold has no github-code provider", p) + } + } + if !slices.Contains(role.Providers, "providers/github.yaml") { + t.Errorf("coder should use providers/github.yaml, got %v", role.Providers) + } +} diff --git a/internal/agentnew/spec.go b/internal/agentnew/spec.go new file mode 100644 index 0000000000..a880b63bd9 --- /dev/null +++ b/internal/agentnew/spec.go @@ -0,0 +1,77 @@ +package agentnew + +import ( + "bytes" + "fmt" + "io" + "os" + + "gopkg.in/yaml.v3" +) + +// SpecVersion is the only accepted `version:` value in a spec file. +const SpecVersion = "1" + +// AgentSpec is the document accepted by `fullsend agent new -f `. Keys +// mirror the command-line flags one for one so a local coding agent can emit +// either form. Command-line flags override spec keys. +type AgentSpec struct { + Version string `yaml:"version"` + Name string `yaml:"name"` + Role string `yaml:"role,omitempty"` + Description string `yaml:"description,omitempty"` + On string `yaml:"on,omitempty"` + Trigger string `yaml:"trigger,omitempty"` + Model string `yaml:"model,omitempty"` + Effort string `yaml:"effort,omitempty"` + Runtime string `yaml:"runtime,omitempty"` + Slug string `yaml:"slug,omitempty"` + Image string `yaml:"image,omitempty"` + TimeoutMinutes int `yaml:"timeout_minutes,omitempty"` + ValidationLoop bool `yaml:"validation_loop,omitempty"` +} + +// LoadSpecFile reads and validates a spec file. +func LoadSpecFile(path string) (*AgentSpec, error) { + data, err := os.ReadFile(path) + if err != nil { + return nil, fmt.Errorf("reading spec file: %w", err) + } + return ParseSpec(data) +} + +// ParseSpec decodes a spec document. Unknown keys are an error rather than a +// silent no-op: a typo in a spec file would otherwise generate a different +// agent than the author asked for, which is the failure mode this command +// exists to remove. +func ParseSpec(data []byte) (*AgentSpec, error) { + dec := yaml.NewDecoder(bytes.NewReader(data)) + dec.KnownFields(true) + + var spec AgentSpec + if err := dec.Decode(&spec); err != nil { + if err == io.EOF { + return nil, fmt.Errorf("spec file is empty") + } + return nil, fmt.Errorf("parsing spec file: %w", err) + } + // A second document would be silently ignored otherwise. + var extra AgentSpec + if err := dec.Decode(&extra); err == nil { + return nil, fmt.Errorf("spec file must contain exactly one YAML document") + } + + if spec.Version != SpecVersion { + return nil, fmt.Errorf("spec version must be %q, got %q", SpecVersion, spec.Version) + } + if spec.Name == "" { + return nil, fmt.Errorf("spec field \"name\" is required") + } + if spec.On != "" && spec.Trigger != "" { + return nil, fmt.Errorf("spec fields \"on\" and \"trigger\" are mutually exclusive") + } + if spec.TimeoutMinutes < 0 { + return nil, fmt.Errorf("spec field \"timeout_minutes\" must not be negative, got %d", spec.TimeoutMinutes) + } + return &spec, nil +} diff --git a/internal/agentnew/spec_test.go b/internal/agentnew/spec_test.go new file mode 100644 index 0000000000..b91c56feaf --- /dev/null +++ b/internal/agentnew/spec_test.go @@ -0,0 +1,82 @@ +package agentnew + +import ( + "os" + "path/filepath" + "strings" + "testing" +) + +func TestParseSpec(t *testing.T) { + spec, err := ParseSpec([]byte(` +version: "1" +name: lint-docs +role: triage +description: Check docs changes for broken links +on: command:/fs-lint-docs +model: opus +effort: high +runtime: claude +slug: my-org-lint-docs +timeout_minutes: 20 +validation_loop: true +`)) + if err != nil { + t.Fatalf("ParseSpec: %v", err) + } + if spec.Name != "lint-docs" || spec.Role != "triage" { + t.Errorf("unexpected spec: %+v", spec) + } + if spec.TimeoutMinutes != 20 || !spec.ValidationLoop { + t.Errorf("unexpected spec: %+v", spec) + } + if spec.Description != "Check docs changes for broken links" { + t.Errorf("description: %q", spec.Description) + } +} + +func TestParseSpecRejects(t *testing.T) { + tests := []struct { + name, doc, wantErr string + }{ + {"unknown key", "version: \"1\"\nname: a\nrol: triage\n", "field rol not found"}, + {"missing version", "name: a\n", "version"}, + {"wrong version", "version: \"2\"\nname: a\n", "version"}, + {"missing name", "version: \"1\"\n", "name"}, + {"on and trigger", "version: \"1\"\nname: a\non: label\ntrigger: 'true'\n", "mutually exclusive"}, + {"negative timeout", "version: \"1\"\nname: a\ntimeout_minutes: -1\n", "negative"}, + {"empty", "", "empty"}, + {"two documents", "version: \"1\"\nname: a\n---\nversion: \"1\"\nname: b\n", "exactly one"}, + {"not a mapping", "- a\n- b\n", "parsing spec file"}, + } + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + _, err := ParseSpec([]byte(tc.doc)) + if err == nil { + t.Fatalf("ParseSpec(%q) succeeded; want error", tc.doc) + } + if !strings.Contains(err.Error(), tc.wantErr) { + t.Errorf("error %q should mention %q", err, tc.wantErr) + } + }) + } +} + +func TestLoadSpecFile(t *testing.T) { + dir := t.TempDir() + path := filepath.Join(dir, "spec.yaml") + if err := os.WriteFile(path, []byte("version: \"1\"\nname: lint-docs\n"), 0o644); err != nil { + t.Fatal(err) + } + spec, err := LoadSpecFile(path) + if err != nil { + t.Fatalf("LoadSpecFile: %v", err) + } + if spec.Name != "lint-docs" { + t.Errorf("name: %q", spec.Name) + } + + if _, err := LoadSpecFile(filepath.Join(dir, "missing.yaml")); err == nil { + t.Error("LoadSpecFile on a missing path should fail") + } +} diff --git a/internal/agentnew/trigger.go b/internal/agentnew/trigger.go new file mode 100644 index 0000000000..8fa73455e0 --- /dev/null +++ b/internal/agentnew/trigger.go @@ -0,0 +1,116 @@ +package agentnew + +import ( + "fmt" + "strings" +) + +// Trigger presets. The expansions below are reproduced from +// docs/guides/user/cel-triggers-reference.md rather than composed here: an +// expression that compiles but touches an absent optional field raises a +// missing-key error at dispatch time, and MatchHarnesses turns that into a +// red ::error:: annotation on every matching event +// (internal/harnessdispatch/enumerate.go:95-99). Emitting known-good text is +// the whole point of generating the trigger instead of asking the user to +// write one. TestTriggerPresetsArePinned asserts the exact strings. +const ( + // PresetCommand fires on a slash command. The change_proposal clause + // uses has() rather than the reference doc's `!= null` because + // change_proposal is ABSENT from state on a non-PR comment (schema + // $defs.state requires only "labels"; see the jira-fs-triage-comment + // and discussion-fs-vouch-comment fixtures), so `!= null` raises a + // missing-key error on every issue comment. The guard refuses fork + // pull requests without excluding plain issues. + PresetCommand = "command" + // PresetLabel fires when a label is added. + PresetLabel = "label" + // PresetIssueOpened fires on new work items. + PresetIssueOpened = "issue-opened" + // PresetPROpened fires when a non-fork pull request is opened or updated. + PresetPROpened = "pr-opened" +) + +// PresetNames lists the recognised --on presets in help order. +func PresetNames() []string { + return []string{ + PresetCommand + ":/", + PresetLabel + ":