From e0897874fa7a26f89f3079b116db4f0bcda0036f Mon Sep 17 00:00:00 2001 From: Nelson Melo Date: Tue, 19 May 2026 14:54:04 -0400 Subject: [PATCH] feat(add-agent): opt-in --custom flag for non-catalog role names The roles package documents itself as an open set ("unknown role names are valid and receive sensible defaults") and LookupRole correctly returns defaults for arbitrary names. But IsValidRoleName at the CLI gated add-agent to the catalog + numbered families, and its own doc comment scoped the fix out: "Operators wanting truly custom roles must add them to the Catalog or design a separate opt-in (out of scope here)." This adds that opt-in. add-agent now accepts a --custom flag. When set, names outside the catalog go through IsValidCustomRoleName, a pure pattern check requiring ^[a-z][a-z0-9-]{0,31}$. The default closed-set gate stays intact for typo protection. Custom roles get the bare default RoleDef (Autonomous, no src, no playbooks) via LookupRole's existing fallback, so no scaffold or runtime changes are needed. Motivated by adding marketing-shaped roles (content, demandgen, events, community) to a non-software project. The same opt-in lets future users register designer, dba, or any other vocabulary their team needs. --- cmd/add_agent.go | 14 ++++- cmd/add_agent_test.go | 111 +++++++++++++++++++++++++++++++++ internal/roles/catalog.go | 30 ++++++++- internal/roles/catalog_test.go | 73 ++++++++++++++++++++++ 4 files changed, 224 insertions(+), 4 deletions(-) diff --git a/cmd/add_agent.go b/cmd/add_agent.go index 3c93ad0..57f5091 100644 --- a/cmd/add_agent.go +++ b/cmd/add_agent.go @@ -20,6 +20,7 @@ import ( var newAddAgentRunner = func() iexec.Runner { return &iexec.DefaultRunner{} } var addAgentList bool +var addAgentCustom bool var addAgentCmd = &cobra.Command{ Use: "add-agent ", @@ -38,8 +39,14 @@ The agent name must be a known role or a member of a numbered family Known roles: %s. Numbered families: eng1..N, qa1..N (e.g. eng7, qa10). +For roles outside the catalog (e.g. "marketing", "designer", "dba"), pass +--custom to opt out of the strict catalog gate. Custom names must match +^[a-z][a-z0-9-]{0,31}$. Custom roles receive sensible defaults: Autonomous +permission, no src submodule, no playbooks. + Restart initech (or run 'initech' in a new session) to activate the new agent.`, knownRoleNames()) addAgentCmd.Flags().BoolVarP(&addAgentList, "list", "l", false, "List all agents and their install status") + addAgentCmd.Flags().BoolVar(&addAgentCustom, "custom", false, "Allow a custom role name outside the catalog (must match ^[a-z][a-z0-9-]{0,31}$)") addAgentCmd.ValidArgsFunction = completeAddAgent rootCmd.AddCommand(addAgentCmd) } @@ -85,7 +92,12 @@ func runAddAgent(cmd *cobra.Command, args []string) error { runner := newAddAgentRunner() if !roles.IsValidRoleName(roleName) { - return fmt.Errorf("unknown agent %q. Known agents: %s. Numbered families also accepted: eng1..N, qa1..N", roleName, knownRoleNames()) + if !addAgentCustom { + return fmt.Errorf("unknown agent %q. Known agents: %s. Numbered families also accepted: eng1..N, qa1..N. Pass --custom to add a role outside the catalog", roleName, knownRoleNames()) + } + if !roles.IsValidCustomRoleName(roleName) { + return fmt.Errorf("invalid custom role name %q: must match ^[a-z][a-z0-9-]{0,31}$ (lowercase letter start, then up to 31 lowercase letters, digits, or hyphens)", roleName) + } } wd, err := os.Getwd() diff --git a/cmd/add_agent_test.go b/cmd/add_agent_test.go index 7da2b5d..de36e8f 100644 --- a/cmd/add_agent_test.go +++ b/cmd/add_agent_test.go @@ -23,6 +23,117 @@ func TestRunAddAgent_UnknownRole(t *testing.T) { if !strings.Contains(err.Error(), "unknown agent") { t.Errorf("error = %q, want to contain 'unknown agent'", err.Error()) } + if !strings.Contains(err.Error(), "--custom") { + t.Errorf("error = %q, want hint mentioning --custom", err.Error()) + } +} + +// TestRunAddAgent_CustomFlag_AllowsArbitraryName proves the opt-in path: +// names outside the catalog and numbered families (e.g. "marketing") are +// accepted when --custom is passed. Regression for the gap where the package +// claimed an open set ("unknown role names are valid and receive sensible +// defaults") but the CLI gate rejected every non-catalog name. +func TestRunAddAgent_CustomFlag_AllowsArbitraryName(t *testing.T) { + root := t.TempDir() + writeTestConfig(t, root, []string{"pm"}) + if err := os.MkdirAll(filepath.Join(root, "pm"), 0755); err != nil { + t.Fatal(err) + } + + chdirTemp(t, root) + + origRunner := newAddAgentRunner + newAddAgentRunner = func() iexec.Runner { return &iexec.FakeRunner{} } + t.Cleanup(func() { newAddAgentRunner = origRunner }) + + addAgentCustom = true + t.Cleanup(func() { addAgentCustom = false }) + + var buf bytes.Buffer + addAgentCmd.SetOut(&buf) + t.Cleanup(func() { addAgentCmd.SetOut(nil) }) + + if err := runAddAgent(addAgentCmd, []string{"marketing"}); err != nil { + t.Fatalf("unexpected error: %v", err) + } + + if _, err := os.Stat(filepath.Join(root, "marketing")); err != nil { + t.Error("marketing/ directory not created") + } + + updated, err := config.Load(filepath.Join(root, "initech.yaml")) + if err != nil { + t.Fatal(err) + } + found := false + for _, r := range updated.Roles { + if r == "marketing" { + found = true + break + } + } + if !found { + t.Errorf("marketing not added to config roles: %v", updated.Roles) + } +} + +// TestRunAddAgent_CustomFlag_RejectsBadIdentifier verifies the opt-in still +// enforces the safe identifier pattern. Uppercase, leading digits, and other +// shell- or filesystem-hostile inputs must reject with a clear pattern error. +func TestRunAddAgent_CustomFlag_RejectsBadIdentifier(t *testing.T) { + root := t.TempDir() + writeTestConfig(t, root, []string{"pm"}) + chdirTemp(t, root) + + addAgentCustom = true + t.Cleanup(func() { addAgentCustom = false }) + + cases := []string{ + "Marketing", // uppercase + "1marketing", // leads with digit + "market.ing", // dot + "market ing", // space + "data_eng", // underscore + "", // empty + } + + for _, name := range cases { + t.Run(name, func(t *testing.T) { + // Empty triggers cobra's "agent name required" branch instead + // of the validator. Skip the empty case here so this test + // stays focused on the pattern gate. + if name == "" { + return + } + err := runAddAgent(addAgentCmd, []string{name}) + if err == nil { + t.Fatalf("expected error for invalid custom name %q", name) + } + if !strings.Contains(err.Error(), "invalid custom role name") { + t.Errorf("error = %q, want to mention 'invalid custom role name'", err.Error()) + } + }) + } +} + +// TestRunAddAgent_WithoutCustomFlag_StillRejectsUnknown locks in the +// existing behavior: --custom is strictly opt-in. The default closed-set +// gate remains for typo protection. +func TestRunAddAgent_WithoutCustomFlag_StillRejectsUnknown(t *testing.T) { + root := t.TempDir() + writeTestConfig(t, root, []string{"pm"}) + chdirTemp(t, root) + + // Ensure custom is off (other tests set it; defend against ordering). + addAgentCustom = false + + err := runAddAgent(addAgentCmd, []string{"marketing"}) + if err == nil { + t.Fatal("expected error for unknown role without --custom") + } + if !strings.Contains(err.Error(), "unknown agent") { + t.Errorf("error = %q, want 'unknown agent'", err.Error()) + } } func TestRunAddAgent_AlreadyExists(t *testing.T) { diff --git a/internal/roles/catalog.go b/internal/roles/catalog.go index e7a0832..9a09a5b 100644 --- a/internal/roles/catalog.go +++ b/internal/roles/catalog.go @@ -20,6 +20,13 @@ import ( // (CLI gate) and LookupRole (default selection for unlisted family members). var numberedRoleRe = regexp.MustCompile(`^(qa|eng)\d+$`) +// customRoleNameRe matches the safe identifier pattern for opt-in custom role +// names. Lowercase letter start, then up to 31 more lowercase letters, digits, +// or hyphens. Hyphens (not underscores) to keep names URL- and directory-shape +// friendly and consistent with the catalog's own naming. Used by +// IsValidCustomRoleName; not used by IsValidRoleName. +var customRoleNameRe = regexp.MustCompile(`^[a-z][a-z0-9-]{0,31}$`) + // PermissionTier controls whether an agent runs with --dangerously-skip-permissions. type PermissionTier int @@ -98,14 +105,15 @@ func LookupRole(name string) RoleDef { } // IsValidRoleName reports whether name is acceptable as a role name for CLI -// commands like 'initech hire' / 'initech add-agent'. Two paths to acceptance: +// commands like 'initech hire' / 'initech add-agent' without any opt-in flag. +// Two paths to acceptance: // - exact match against the Catalog (covers the curated role set), or // - match against the numbered family pattern qa\d+ / eng\d+ (covers // arbitrary scaling like qa10, eng7, qa007). // // Custom non-numbered names (e.g. "designer", "dba") are deliberately rejected -// at the CLI to preserve typo protection. Operators wanting truly custom roles -// must add them to the Catalog or design a separate opt-in (out of scope here). +// here to preserve typo protection. The opt-in path is IsValidCustomRoleName, +// gated at the CLI by an explicit --custom flag. func IsValidRoleName(name string) bool { if _, ok := Catalog[name]; ok { return true @@ -113,6 +121,22 @@ func IsValidRoleName(name string) bool { return numberedRoleRe.MatchString(name) } +// IsValidCustomRoleName reports whether name is acceptable as an opt-in custom +// role name. The check is strictly a pattern check: lowercase identifier, +// 1-32 characters, starting with a letter, with alphanumerics and hyphens +// allowed thereafter (^[a-z][a-z0-9-]{0,31}$). +// +// Callers should reach for this only after IsValidRoleName has returned false +// and the operator has explicitly opted in (e.g. via --custom). Catalog and +// numbered-family names also satisfy this pattern; callers can safely check +// IsValidRoleName first and treat IsValidCustomRoleName as the fallback gate. +// +// The pattern keeps custom names URL- and directory-friendly and consistent +// with the catalog's own naming. Underscores are intentionally excluded. +func IsValidCustomRoleName(name string) bool { + return customRoleNameRe.MatchString(name) +} + // RoleFamily groups roles that share notification and lifecycle semantics. // Used by initech deliver to pick the right announce template per caller, and // by status-transition logic to decide which lifecycle move (if any) to make. diff --git a/internal/roles/catalog_test.go b/internal/roles/catalog_test.go index f6b1dc1..388c11f 100644 --- a/internal/roles/catalog_test.go +++ b/internal/roles/catalog_test.go @@ -161,6 +161,79 @@ func TestIsValidRoleName(t *testing.T) { } } +// TestIsValidCustomRoleName_AcceptsAndRejects covers the opt-in custom-role +// gate. The function is a pure pattern check — lowercase letter start, then up +// to 31 lowercase letters, digits, or hyphens. Catalog and numbered-family +// names also satisfy the pattern; the catalog clash is handled by callers +// that consult IsValidRoleName first. +// +// Regression for the missing opt-in scoped out by IsValidRoleName's prior doc +// comment: operators legitimately want custom names like "marketing" and +// "designer" but the CLI rejected every non-catalog name. +func TestIsValidCustomRoleName_AcceptsAndRejects(t *testing.T) { + tests := []struct { + name string + want bool + }{ + // Legitimate custom names operators want to use. + {"marketing", true}, + {"designer", true}, + {"dba", true}, + {"content", true}, + {"demandgen", true}, + {"leadops", true}, + {"events", true}, + {"community", true}, + {"data-eng", true}, + {"lead-gen", true}, + + // Catalog and numbered-family names also satisfy the pattern. + // Callers check IsValidRoleName first; this is fine. + {"super", true}, + {"pm", true}, + {"eng1", true}, + {"qa10", true}, + + // Empty rejects. + {"", false}, + + // Must start with a lowercase letter. + {"1marketing", false}, + {"-marketing", false}, + {"9designer", false}, + + // Uppercase rejects (consistency with IsValidRoleName). + {"Marketing", false}, + {"DESIGNER", false}, + {"Data-Eng", false}, + + // Underscores rejected by design (hyphens only). + {"data_eng", false}, + {"lead_gen", false}, + + // Other punctuation and whitespace rejected. + {"marketing!", false}, + {"market.ing", false}, + {"market ing", false}, + {"marketing/", false}, + {"marketing\n", false}, + {" marketing", false}, + {"marketing ", false}, + + // Max length 32 characters: the boundary cases. + {"abcdefghijklmnopqrstuvwxyz012345", true}, // 32 chars + {"abcdefghijklmnopqrstuvwxyz0123456", false}, // 33 chars + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + got := IsValidCustomRoleName(tt.name) + if got != tt.want { + t.Errorf("IsValidCustomRoleName(%q) = %v, want %v", tt.name, got, tt.want) + } + }) + } +} + func TestLookupRole_NumberedFamily(t *testing.T) { t.Run("qa10 inherits qa1/qa2 defaults", func(t *testing.T) { def := LookupRole("qa10")