fix(#2528): sync VALID_ROLES with Go's ValidRoles() - #6369
Conversation
The web admin's VALID_ROLES had only 4 roles while the backend accepts 8. Add the missing fix, retro, prioritize, and e2e roles and update both error messages. A parameterized test now covers all 8 roles to prevent future drift. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Signed-off-by: Shai Revivo <srevivo@redhat.com>
E2E tests did not runE2E tests run automatically for org/repo members and collaborators on pull requests. For other contributors, a maintainer must add the See E2E testing guide for details. |
PR Summary by QodoSync web admin role validation with Go backend
AI Description
Diagram
High-Level Assessment
Files changed (2)
|
Code Review by Qodo
1.
|
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
…urce/name/enabled VALID_ROLES was missing fix, retro, prioritize, and e2e — add them for defaults.roles validation. Separately, the agent type still used the legacy role/name/slug format that Go rejects since ADR 0045 Phase 4. Update the TypeScript agent type, parser, and downstream consumers to use Go's current source/name/enabled schema with DerivedName() logic for name extraction. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Signed-off-by: Shai Revivo <srevivo@redhat.com>
|
/fs-review |
Site previewPreview: https://0a4f44c0-site.fullsend-ai.workers.dev Commit: |
|
🤖 Finished Review · ✅ Success · Started 10:08 AM UTC · Completed 10:26 AM UTC Commit: |
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
ReviewFindingsMedium
Low
|
![fullsend-ai-review[bot]](https://avatars.githubusercontent.com/in/3478501?s=60&v=4){kind=small}
There was a problem hiding this comment.
See the review comment for full details.
Note: The following inline comments could not be posted on the diff (GitHub returned 422) and are included here instead:
web/admin/src/lib/layers/orgConfigParse.ts:14: [medium] intent-mismatch
The PR title "fix(#2528): sync VALID_ROLES with Go's ValidRoles()" understates the actual scope. Beyond expanding VALID_ROLES from 4 to 8 entries, this PR also refactors the agent config schema from { role, name, slug } to (string | { source, name, enabled }), adds derivedAgentName/sourceBaseName/isAgentEnabled helpers, and propagates the role-to-name rename across multiple files.
Suggested fix: Update the title to reflect the full scope, e.g., fix(#2528): sync VALID_ROLES and agent config schema with Go.
web/admin/src/lib/layers/orgConfigParse.ts:12: [low] scope-creep
The agent type refactoring goes beyond what issue #2528 explicitly authorizes. However, Go's AgentEntry struct already uses Source/Name/Enabled fields and DerivedName() logic, so aligning the TypeScript type is a natural companion to the VALID_ROLES sync.
web/admin/src/lib/layers/orgConfigParse.ts:153: [low] missing-authorization
The per-agent VALID_ROLES check in validateOrgConfig was removed. This aligns with Go behavior: Go's Validate() uses ValidateAgentEntries() for structural checks (name format, source URL allowlists, duplicate detection) — not role membership.
web/admin/src/lib/layers/orgConfigParse.ts:150: [low] input-validation
Agent names now flow from user-supplied source/name values into secretNameForRole/variableNameForRole without an allowlist check. Risk is minimal: Go pre-validates agent names with validConfigAgentName regex, the TS admin is read-only, and Octokit URL-encodes path parameters.
web/admin/src/lib/layers/secrets.ts(file-level): Line 5 · [low] naming-convention
secretNameForRole and variableNameForRole retain 'Role' in their names and 'role' as their parameter despite now being called with agent.name. Cosmetic inconsistency with 2 call sites.
Suggested fix: Rename to secretNameForAgent/variableNameForAgent and parameter from role to name.
web/admin/src/lib/layers/orgConfigParse.ts:166: [low] input-validation
sourceBaseName accepts arbitrary strings without character validation. Go handles this via validConfigAgentName regex and case-insensitive duplicate detection. The TS implementation mirrors Go's DerivedName() by design.
web/admin/src/lib/layers/orgConfigParse.ts:170: [low] edge-case
derivedAgentName returns empty string when an agent entry has no source and no name, producing malformed secret names. Go's ValidateAgentEntries rejects this case, so this path is unreachable for valid configs.
web/admin/src/lib/layers/orgConfigParse.ts:16: [low] naming-convention
VALID_ROLES constant name still references 'role' despite the agent schema moving to name-based naming. However, the constant validates defaults.roles entries, which are still called 'roles' in both Go and TS schemas — the name is accurate for its current purpose.
fullsend-ai-review
Bot
added
the
requires-manual-review
waynesun09
left a comment
waynesun09
left a comment
There was a problem hiding this comment.
Review findings
Four findings on the agent-schema refactor that ships alongside the VALID_ROLES sync — 2 HIGH, 2 MEDIUM, posted inline. Summary:
- HIGH
web/admin/src/lib/layers/secrets.ts:33— secret/variable names are now derived from agent names; Go derives them from roles, so the layer looks up credentials that cannot exist. - HIGH
web/admin/src/lib/layers/orgConfigParse.test.ts:101— the new test asserts a fixture is valid that Go rejects for two independent reasons, andvalidateOrgConfigstill claims Go parity while porting none ofValidateAgentEntries. - MEDIUM
web/admin/src/lib/layers/orgConfigParse.ts:114— the removedtypeofguard lets a non-stringagents[].namesurface as a rawTypeErroron the org row instead of degrading. - MEDIUM
web/admin/src/lib/layers/orgConfigParse.ts:167—sourceBaseNamediverges from Go'sDerivedName()on trailing slashes and dot-leading basenames.
Review-only; no changes requested and no approval implied.
|
|
||
| for (const agent of agents) { | ||
| const sName = secretNameForRole(agent.role); | ||
| const sName = secretNameForRole(agent.name); |
There was a problem hiding this comment.
HIGH — Secrets layer now keys secret/variable names off derived agent names; Go keys them off roles
secrets.ts is documented as a read-only port of SecretsLayer.Analyze (internal/layers/secrets.go), but this PR changes the value fed into secretNameForRole/variableNameForRole from agent.role to agent.name, where name is now the derived agent name (explicit name:, or the basename of source:).
Verified on this head, Go always names these credentials from the role:
internal/layers/secrets.go:123/134callsecretName(agent.Role)/variableName(agent.Role);secretNameisFULLSEND_%s_APP_PRIVATE_KEYofstrings.ToUpper(role)(secrets.go:183-188).- Every construction of
layers.AgentCredentialsiterates a role list —internal/cli/admin.go:1434(for _, role := range roles),internal/cli/admin.go:2025(for _, role := range config.DefaultAgentRoles()inrunAnalyze, the exact path this TS mirrors),internal/cli/github.go:658, andtoAgentCredentials(role, ...)atadmin.go:2284. AgentEntry.DerivedName()is used only for the harness registry, dispatch matching, poll, andfullsend agentlisting (harness/registry.go:50,cli/run.go:3799,cli/poll.go:238,config/interfaces.go:240) — never for a secret or variable name (grep APP_PRIVATE_KEYunderinternal/returns only role-based constructions).
Concrete regression: for the config in this PR's own new test (source: https://example.com/coder.yaml#sha256=… + name: my-coder), the UI now queries FULLSEND_MY-CODER_APP_PRIVATE_KEY (confirmed by running the shipped helpers). GitHub Actions secret names cannot contain hyphens, so that secret can never exist and the layer permanently reports not_installed/degraded for any org registering a custom agent — while the real FULLSEND_CODER_* credentials go unchecked. analyzeOrg.ts:13 was updated to assert the wrong contract in prose too ("Agent names from org config (drives secret/variable names)").
Note: the separate false-green when a config has no agents: key at all (config.NewOrgConfig never emits one, so agentsFromConfig returns [] and the layer reports "installed" having checked nothing) is pre-existing, but this PR re-commits to that source of truth in a rewritten function and doc comment.
Suggestion: Feed the secrets layer from roles, matching Go's analyze path — pass roles: string[] (from cfg.defaults.roles, falling back to Go's config.DefaultAgentRoles() set when absent, which is what runAnalyze uses) instead of agents: {name}[], and restore the analyzeOrg.ts:13 comment to say roles drive secret/variable names, citing internal/layers/secrets.go secretName/variableName. Keep agentsFromConfig/derived names only for callers that genuinely need agent identity. Add a test asserting a config with defaults.roles and no agents: key still checks FULLSEND_<ROLE>_APP_PRIVATE_KEY for every role.
| agents: | ||
| - role: not-a-valid-role | ||
| - source: harness/triage.yaml | ||
| - source: https://example.com/coder.yaml#sha256=abc123 |
There was a problem hiding this comment.
HIGH — New test asserts a config Go rejects twice is valid; all agent validation dropped while the doc comment still claims Go parity
The new test "accepts agents with source-based entries" asserts validateOrgConfig(cfg) === null for a fixture containing - source: https://example.com/coder.yaml#sha256=abc123. Verified against ValidateAgentEntries on this head (internal/config/config.go:436-443):
urlutil.ParseIntegrityHashrequires a 64-hex sha256, soabc123yieldshasHash=false→agents[1] (my-coder): URL source must include a valid #sha256=<64-hex-char> integrity fragment.- Even with a valid digest, the fixture has no
allowed_remote_resources, soexample.comfailsMatchingAllowedPrefixInList→URL %q is not covered by allowed_remote_resources.
So the PR whose stated purpose is UI/Go parity ships a test hardcoding a UI/Go divergence, and the fixture will mislead the next person who treats it as a valid-config example.
More broadly, validateOrgConfig (line 132) still carries the doc comment "matches Go Validate errors" while performing zero agent validation. Go's Validate calls ValidateAgentEntries, which enforces: source required on enabled entries (config.go:410), explicit name on disabled entries (config.go:407), derived name against ^[a-zA-Z0-9][a-zA-Z0-9_-]*$ (config.go:415), case-insensitive duplicate names (config.go:420-426), https-only sources, no http://, no other :// schemes, no absolute paths, no .. traversal (config.go:444-460).
Sub-point: a legacy - role: triage entry now passes the TS shape checks and derives name "" (confirmed by running the helpers), where Go's AgentEntry.UnmarshalYAML fails at parse time with "agents entry uses legacy role/name/slug format (removed by ADR 0045 Phase 4)" — i.e. the stale-format config this migration targets is exactly the one the UI stops reporting.
Suggestion: Fix the fixture to a real 64-hex #sha256= digest under an allowed_remote_resources-covered prefix (or flip the assertion to the expected Go error string), and port the checked subset of ValidateAgentEntries into validateOrgConfig: require source on enabled entries, an explicit name on disabled entries, ^[a-zA-Z0-9][a-zA-Z0-9_-]*$ on derived names, https-only with a valid integrity fragment plus allowlist match, reject http://, other schemes, absolute paths, backslashes and .., and detect case-insensitive duplicates. Add negative tests mirroring each Go error string, including the legacy - role: entry. If a narrower scope is intended, at minimum narrow the validateOrgConfig doc comment to state which Go validations are and are not ported.
| } | ||
| for (let i = 0; i < doc.agents.length; i++) { | ||
| const el = doc.agents[i]; | ||
| if (typeof el === "string") continue; |
There was a problem hiding this comment.
MEDIUM — Removed typeof guard lets a non-string agents[].name escape as a raw TypeError on the org row
This PR deletes the only type assertion on agent fields (agents[${i}].role must be a string) and replaces it with if (typeof el === "string") continue;, validating nothing inside mappings (orgConfigParse.ts:112-118).
Reproduced by running the shipped helpers in Node: a YAML name: 42 parses to a number under the core schema, derivedAgentName returns it unchanged (if (entry.name) return entry.name; — line 163 does not check the type), agentsFromConfig returns [{name: 42}] without throwing, and the throw only happens later in secretNameForRole (secrets.ts:6): TypeError: r.toUpperCase is not a function. That throw originates inside analyzeSecretsLayer, which runs at orgListRow.ts:104 — outside the inner try/catch whose comment deliberately degrades gracefully ("invalid YAML — still analyze other layers with empty agents/repos", orgListRow.ts:100) — so it lands in the generic outer catch at orgListRow.ts:133-137 and the whole org row renders a raw JS internal message instead of degrading.
For accuracy: the neighbouring case of a non-string source does not have this problem — source: 123 throws inside sourceBaseName during agentsFromConfig at orgListRow.ts:89, which is inside the inner try, so it degrades correctly. Only the name path escapes.
Related: a quoted enabled: "false" is !== false, so isAgentEnabled keeps the agent enabled with no type check.
Suggestion: In assertOrgConfigShape, assert name and source are strings when present and enabled is a boolean when present, using the same agents[${i}].<field> must be a … message style as the check that was removed.
| return sourceBaseName(entry.source ?? ""); | ||
| } | ||
|
|
||
| function sourceBaseName(src: string): string { |
There was a problem hiding this comment.
MEDIUM — sourceBaseName does not mirror Go's DerivedName() as its doc comment claims
agentsFromConfig is documented as mirroring config.OrgConfig.Agents, and sourceBaseName hand-reimplements Go's AgentEntry.DerivedName() (internal/config/config.go:74-85, path.Base + strings.TrimSuffix(base, path.Ext(base))). Running both implementations side by side on the same inputs, they disagree:
| input | Go DerivedName() |
this TS |
|---|---|---|
harness/ |
harness |
"" |
harness/triage.yaml/ |
triage |
"" |
x/.yaml |
"" (then rejected by Go's name regex) |
.yaml → FULLSEND_.YAML_APP_PRIVATE_KEY |
.yaml |
"" |
.yaml |
The dotIdx > 0 guard is what breaks dot-leading basenames, and split("/").pop() is what breaks trailing slashes where path.Base strips them. (For an empty source Go returns "", not "." — path.Ext(".") == ".".)
This matters because the derived name is now the secret-lookup key (see the HIGH finding on secrets.ts), and this PR drops the derived-name regex check Go applies at config.go:415, so a divergent name reaches the GitHub API instead of failing validation. The existing [input-validation] note on this function states the TS implementation "mirrors Go's DerivedName() by design" — the runs above disprove that specific claim.
Suggestion: Strip trailing / before taking the last segment and mirror path.Ext for dot-leading basenames (no dotIdx > 0 special case), then add Go's ^[a-zA-Z0-9][a-zA-Z0-9_-]*$ check on the derived name so remaining malformed cases fail validation instead of producing a lookup key. Add unit tests for harness/, harness/triage.yaml/, .yaml and "" pinning the Go-matching values.
…s, not agent names Go's secrets layer keys credentials by role (from defaults.roles), not by derived agent name. Replace agentsFromConfig (which derived names from agent source) with rolesFromConfig (which reads defaults.roles directly). This matches Go's AgentCredentials.Role flow. Remove the now-unnecessary derivedAgentName/sourceBaseName helpers and their edge-case-prone reimplementation of DerivedName(). Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Signed-off-by: Shai Revivo <srevivo@redhat.com>
waynesun09
left a comment
waynesun09
left a comment
There was a problem hiding this comment.
Validated at head 1edfda57:
VALID_ROLESnow exactly matches Go'sValidRoles()(internal/config/config.go:184) — all 8 roles, and the error string mirrors Go'sinvalid role %q: must be one of …format.- The agents schema rewrite matches Go's
AgentEntry(string | {source, name, enabled}), and dropping the per-agent role check is correct post-ADR-0045: Go validates roles only ondefaults.roles/per-reporoles, never on agents. - My previous findings are all fixed at head: secrets analysis is keyed off
defaults.rolesviarolesFromConfig(matching Go'sAgentCredentials{Role: role}+secretName(role)ininternal/layers/secrets.go), the name-derivation helpers (derivedAgentName/sourceBaseName) and their divergences are removed entirely, and the test fixture now uses local-path sources Go actually accepts. - Tests cover all 8 roles parameterized, the invalid-role rejection, and both
rolesFromConfigpaths. CI green.
Two residual gaps, both non-blocking for a read-only dashboard, fine as follow-ups:
- TS accepts legacy
role:-only agent mappings that Go rejects with the ADR-0045 error, and doesn't portValidateAgentEntries(sha256 integrity fragment, remote-resource allowlist, duplicate names) — so the admin UI can report a config as valid that the CLI rejects. Worth a code comment stating that agent-entry validation is intentionally out of scope here. - Go's
Validate()also rejects duplicate roles indefaults.roles(config.go:341); the TS validator doesn't, soroles: [triage, triage]shows valid in the UI but fails in Go.
Dismissing my approval — see the comment below. This is not a code-quality issue; web/admin is being removed.
|
Closing this, and I want to be clear that the reason has nothing to do with the quality of the work. Work on the web admin SPA is stopped, and we have decided to remove Follow-ups on our side:
|
|
🤖 Finished Retro · ❌ Failure · Started 2:51 PM UTC · Completed 2:51 PM UTC Commit: |
Work on the web admin SPA is stopped and the feature is being removed rather than left paused. Paused-but-present code kept advertising itself as live work: the pause was recorded only in the title of docs/web-admin-deployment.md, while CI still ran the admin test suite and issue #2528 carried both good-first-issue and ready-to-code, so the code agent and human contributors were both routed at it (PR #6369). Removed: - web/admin/ (Svelte SPA) and web/docs/ (orphan test whose source was already gone) - The OAuth BFF in the site Worker: /api/oauth/authorize, /api/oauth/token and /api/github/user, plus oauthCors.ts and the admin API tests. The Worker is now a passthrough to the ASSETS binding. - The two [[ratelimits]] blocks, run_worker_first, and the wrangler.toml patch script that existed only to keep their namespace ids unique. The deploy already passes --name explicitly, so the name patch was redundant. - GITHUB_APP_* and TURNSTILE_* wiring from site-deploy.yml (both the production deploy and the PR preview upload), sample.env.local, and the matching miniflare test bindings. - The root Vite build, svelte-check, eslint config, the Svelte half of the Prettier config, and every runtime npm dependency, all of which were admin-only. vite.config.ts survives as a vitest-only config so the VitePress theme tests keep running. Note that removing run_worker_first makes the passthrough the Worker's only code path. The ASSETS binding was never declared in wrangler.toml, which was harmless while the Worker only ran for /api/* (those paths returned JSON before reaching the ASSETS branch) but would now 503 every request that reaches the Worker. wrangler.toml declares binding = "ASSETS", and a test asserts it is present. Unaffected, and verified so: - The VitePress documentation site builds and ships unchanged; docs:build passes and /docs/ is still assembled into the deploy bundle. - The public mint at mint.fullsend.sh is a separate Worker provisioned from internal/dispatch/cf/ with its own wrangler.toml. No file under internal/ is touched. - e2e/admin/ is the CLI install e2e suite, not the SPA, and is untouched. Cloudflare-side secrets (GITHUB_APP_CLIENT_SECRET, TURNSTILE_SECRET_KEY) and the FULLSEND_GITHUB_APP_* / FULLSEND_TURNSTILE_* repo secrets and variables are now unused and should be deleted out of band. The admin SPA was never rolled out, so this is not a user-facing breaking change and carries no `!` marker: there is no deployed UI for anyone to migrate off. Installation has always been driven by the CLI (`fullsend github setup`, `fullsend repos`). Stale /admin/* paths fall back to the landing page rather than 404, since not_found_handling stays "single-page-application". Refs #2528 Assisted-by: Claude Signed-off-by: Wayne Sun <gsun@redhat.com>
Work on the web admin SPA is stopped and the feature is being removed rather than left paused. Paused-but-present code kept advertising itself as live work: the pause was recorded only in the title of docs/web-admin-deployment.md, while CI still ran the admin test suite and issue #2528 carried both good-first-issue and ready-to-code, so the code agent and human contributors were both routed at it (PR #6369). Removed: - web/admin/ (Svelte SPA) and web/docs/ (orphan test whose source was already gone) - The OAuth BFF in the site Worker: /api/oauth/authorize, /api/oauth/token and /api/github/user, plus oauthCors.ts and the admin API tests. The Worker is now a passthrough to the ASSETS binding. - The two [[ratelimits]] blocks, run_worker_first, and the wrangler.toml patch script that existed only to keep their namespace ids unique. The deploy already passes --name explicitly, so the name patch was redundant. - GITHUB_APP_* and TURNSTILE_* wiring from site-deploy.yml (both the production deploy and the PR preview upload), sample.env.local, and the matching miniflare test bindings. - The root Vite build, svelte-check, eslint config, the Svelte half of the Prettier config, and every runtime npm dependency, all of which were admin-only. vite.config.ts survives as a vitest-only config so the VitePress theme tests keep running. Note that removing run_worker_first makes the passthrough the Worker's only code path. The ASSETS binding was never declared in wrangler.toml, which was harmless while the Worker only ran for /api/* (those paths returned JSON before reaching the ASSETS branch) but would now 503 every request that reaches the Worker. wrangler.toml declares binding = "ASSETS", and a test asserts it is present. Unaffected, and verified so: - The VitePress documentation site builds and ships unchanged; docs:build passes and /docs/ is still assembled into the deploy bundle. - The public mint at mint.fullsend.sh is a separate Worker provisioned from internal/dispatch/cf/ with its own wrangler.toml. No file under internal/ is touched. - e2e/admin/ is the CLI install e2e suite, not the SPA, and is untouched. Cloudflare-side secrets (GITHUB_APP_CLIENT_SECRET, TURNSTILE_SECRET_KEY) and the FULLSEND_GITHUB_APP_* / FULLSEND_TURNSTILE_* repo secrets and variables are now unused and should be deleted out of band. The admin SPA was never rolled out, so this is not a user-facing breaking change and carries no `!` marker: there is no deployed UI for anyone to migrate off. Installation has always been driven by the CLI (`fullsend github setup`, `fullsend repos`). Stale /admin/* paths fall back to the landing page rather than 404, since not_found_handling stays "single-page-application". Refs #2528 Assisted-by: Claude Signed-off-by: Wayne Sun <gsun@redhat.com>
Work on the web admin SPA is stopped and the feature is being removed rather than left paused. Paused-but-present code kept advertising itself as live work: the pause was recorded only in the title of docs/web-admin-deployment.md, while CI still ran the admin test suite and issue #2528 carried both good-first-issue and ready-to-code, so the code agent and human contributors were both routed at it (PR #6369). Removed: - web/admin/ (Svelte SPA) and web/docs/ (orphan test whose source was already gone) - The OAuth BFF in the site Worker: /api/oauth/authorize, /api/oauth/token and /api/github/user, plus oauthCors.ts and the admin API tests. The Worker is now a passthrough to the ASSETS binding. - The two [[ratelimits]] blocks, run_worker_first, and the wrangler.toml patch script that existed only to keep their namespace ids unique. The deploy already passes --name explicitly, so the name patch was redundant. - GITHUB_APP_* and TURNSTILE_* wiring from site-deploy.yml (both the production deploy and the PR preview upload), sample.env.local, and the matching miniflare test bindings. - The root Vite build, svelte-check, eslint config, the Svelte half of the Prettier config, and every runtime npm dependency, all of which were admin-only. vite.config.ts survives as a vitest-only config so the VitePress theme tests keep running. Note that removing run_worker_first makes the passthrough the Worker's only code path. The ASSETS binding was never declared in wrangler.toml, which was harmless while the Worker only ran for /api/* (those paths returned JSON before reaching the ASSETS branch) but would now 503 every request that reaches the Worker. wrangler.toml declares binding = "ASSETS", and a test asserts it is present. Unaffected, and verified so: - The VitePress documentation site builds and ships unchanged; docs:build passes and /docs/ is still assembled into the deploy bundle. - The public mint at mint.fullsend.sh is a separate Worker provisioned from internal/dispatch/cf/ with its own wrangler.toml. No file under internal/ is touched. - e2e/admin/ is the CLI install e2e suite, not the SPA, and is untouched. Cloudflare-side secrets (GITHUB_APP_CLIENT_SECRET, TURNSTILE_SECRET_KEY) and the FULLSEND_GITHUB_APP_* / FULLSEND_TURNSTILE_* repo secrets and variables are now unused and should be deleted out of band. The admin SPA was never rolled out, so this is not a user-facing breaking change and carries no `!` marker: there is no deployed UI for anyone to migrate off. Installation has always been driven by the CLI (`fullsend github setup`, `fullsend repos`). Stale /admin/* paths fall back to the landing page rather than 404, since not_found_handling stays "single-page-application". Refs #2528 Assisted-by: Claude Signed-off-by: Wayne Sun <gsun@redhat.com>
3 participants
Summary
fix,retro,prioritize, ande2eroles toVALID_ROLESinweb/admin/src/lib/layers/orgConfigParse.ts, matching Go'sValidRoles()Closes #2528
Test plan
vitest run orgConfigParse.test.ts— 18/18 pass (8 parameterized role tests + 10 existing)🤖 Generated with Claude Code