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")