From ad73a56db18e2a89ac4b46623144bb2c3e0c2605 Mon Sep 17 00:00:00 2001 From: Hongming Wang Date: Fri, 24 Apr 2026 16:16:09 -0700 Subject: [PATCH] feat(env-preflight): support any_of OR groups (e.g. API_KEY OR OAUTH_TOKEN) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Extends the org-import env preflight so a template can declare an alternative: satisfy ANY one member to pass. Motivated by the Claude-family node case where either ANTHROPIC_API_KEY or CLAUDE_CODE_OAUTH_TOKEN unlocks the agent — forcing both was wrong. Server (workspace-server): - New EnvRequirement union type with custom YAML + JSON (un)marshaling. Accepts scalar (strict) or {any_of: [...]} in both on-disk org.yaml and inline POST /org/import bodies. - collectOrgEnv now returns []EnvRequirement. Dedups groups by sorted-member signature. "Strict wins" pruning drops any-of groups that mention a name already declared strictly (same tier and cross-tier). - Import preflight uses EnvRequirement.IsSatisfied — scalar = exact match, group = any member present. - Empty any_of: [] rejected at parse time (never-satisfiable). - 14 handler tests (6 updated for the union shape, 8 new covering any-of satisfaction, dedup, strict-dominates-group, cross-tier pruning, invalid-member filtering, YAML round-trip, and empty-any-of rejection). Canvas: - EnvRequirement = string | {any_of: string[]} with envReqMembers, envReqSatisfied, envReqKey helpers. - OrgImportPreflightModal renders strict rows and any-of groups via a new AnyOfEnvGroup sub-component: "Configure any one" banner, per-member input, ✓-satisfied indicator, and dimmed siblings once any member is configured so the user can still switch providers. - TemplatePalette.OrgTemplate.required_env / recommended_env retyped to EnvRequirement[]; passthrough to the modal unchanged. Co-Authored-By: Claude Opus 4.7 (1M context) --- .../components/OrgImportPreflightModal.tsx | 223 ++++++++++++-- canvas/src/components/TemplatePalette.tsx | 18 +- workspace-server/internal/handlers/org.go | 146 +++++++-- .../internal/handlers/org_import.go | 173 +++++++++-- .../internal/handlers/org_test.go | 281 ++++++++++++++++-- 5 files changed, 728 insertions(+), 113 deletions(-) diff --git a/canvas/src/components/OrgImportPreflightModal.tsx b/canvas/src/components/OrgImportPreflightModal.tsx index 97123663..a9d76e50 100644 --- a/canvas/src/components/OrgImportPreflightModal.tsx +++ b/canvas/src/components/OrgImportPreflightModal.tsx @@ -3,6 +3,40 @@ import { useCallback, useEffect, useMemo, useState } from "react"; import { createSecret } from "@/lib/api/secrets"; +/** + * One entry from the server's preflight `required_env` / `recommended_env`. + * + * - A plain string is a STRICT requirement: that exact env var must be + * configured. + * - A `{any_of: [...]}` object is an OR group: at least one member + * must be configured to satisfy it. Lets a template say "either + * ANTHROPIC_API_KEY or CLAUDE_CODE_OAUTH_TOKEN" without forcing + * both. + * + * Matches the Go `EnvRequirement` type's JSON shape (MarshalJSON in + * workspace-server/internal/handlers/org.go). The union is written so + * that a narrow check — `typeof e === "string"` — distinguishes cleanly. + */ +export type EnvRequirement = string | { any_of: string[] }; + +/** Flat member list for a requirement. */ +export function envReqMembers(r: EnvRequirement): string[] { + return typeof r === "string" ? [r] : r.any_of; +} + +/** True if any member is present in `configured`. */ +export function envReqSatisfied(r: EnvRequirement, configured: Set): boolean { + if (typeof r === "string") return configured.has(r); + return r.any_of.some((m) => configured.has(m)); +} + +/** Stable react-key / dedup key for a requirement. Sorted for groups so + * reordered-member variants still collapse to one entry. */ +export function envReqKey(r: EnvRequirement): string { + if (typeof r === "string") return r; + return [...r.any_of].sort().join("|"); +} + interface Props { open: boolean; /** Display name of the org template — headline only. */ @@ -10,11 +44,13 @@ interface Props { /** Total workspace count so the header can read "12 workspaces". */ workspaceCount: number; /** Env vars the server has declared MUST be set as global secrets. - * Import is disabled until every entry here is configured. */ - requiredEnv: string[]; + * Import is disabled until every entry here is configured. Entries + * are either a single key name or an any-of group. */ + requiredEnv: EnvRequirement[]; /** Env vars the server suggests — import can proceed without them, - * but the user sees them listed so they can decide. */ - recommendedEnv: string[]; + * but the user sees them listed so they can decide. Same union + * shape as `requiredEnv`. */ + recommendedEnv: EnvRequirement[]; /** Names of env vars already configured globally. Used to strike * through entries the user has already set up in another * session. Passed in rather than queried inside the modal so the @@ -69,11 +105,23 @@ export function OrgImportPreflightModal({ }: Props) { const [drafts, setDrafts] = useState>({}); + // Flatten the union-shaped requirement lists to the set of every key + // that could ever appear as an input row. Used purely to seed the + // drafts map — satisfaction semantics still read from the grouped + // EnvRequirement entries (a group can be satisfied by any one + // member). + const allMemberKeys = useMemo(() => { + const keys: string[] = []; + for (const r of requiredEnv) keys.push(...envReqMembers(r)); + for (const r of recommendedEnv) keys.push(...envReqMembers(r)); + return keys; + }, [requiredEnv, recommendedEnv]); + // Seed a draft entry per declared key the first time the modal // opens. Entries persist across `configuredKeys` changes so a mid- // save recheck doesn't wipe what the user typed. // - // Dep: dervie a STABLE string from the env-name lists rather than + // Dep: derive a STABLE string from the env-name lists rather than // the array refs themselves. The parent computes // `preflight.org.required_env ?? []`, which produces a fresh [] // identity on every re-render (e.g. when refreshConfiguredKeys @@ -81,14 +129,14 @@ export function OrgImportPreflightModal({ // effect on every parent render and mask any future edit that // drops the `if (!next[k])` guard as a silent input-reset bug. const envKeysSignature = useMemo( - () => [...requiredEnv, ...recommendedEnv].sort().join("|"), - [requiredEnv, recommendedEnv], + () => [...allMemberKeys].sort().join("|"), + [allMemberKeys], ); useEffect(() => { if (!open) return; setDrafts((prev) => { const next = { ...prev }; - for (const k of [...requiredEnv, ...recommendedEnv]) { + for (const k of allMemberKeys) { if (!next[k]) { next[k] = { key: k, value: "", saving: false, error: null }; } @@ -99,11 +147,11 @@ export function OrgImportPreflightModal({ }, [open, envKeysSignature]); const missingRequired = useMemo( - () => requiredEnv.filter((k) => !configuredKeys.has(k)), + () => requiredEnv.filter((r) => !envReqSatisfied(r, configuredKeys)), [requiredEnv, configuredKeys], ); const missingRecommended = useMemo( - () => recommendedEnv.filter((k) => !configuredKeys.has(k)), + () => recommendedEnv.filter((r) => !envReqSatisfied(r, configuredKeys)), [recommendedEnv, configuredKeys], ); const canProceed = missingRequired.length === 0; @@ -240,7 +288,7 @@ interface EnvListProps { tone: "required" | "recommended"; title: string; subtitle: string; - entries: string[]; + entries: EnvRequirement[]; configuredKeys: Set; drafts: Record; onChange: (key: string, value: string) => void; @@ -271,35 +319,162 @@ function EnvList({

{subtitle}

    - {entries.map((k) => { - const configured = configuredKeys.has(k); - const d = drafts[k]; + {entries.map((entry) => + typeof entry === "string" ? ( + + ) : ( + + ), + )} +
+ + ); +} + +interface StrictEnvRowProps { + envKey: string; + configured: boolean; + draft: DraftEntry | undefined; + onChange: (key: string, value: string) => void; + onSave: (key: string) => void; +} + +function StrictEnvRow({ + envKey, + configured, + draft: d, + onChange, + onSave, +}: StrictEnvRowProps) { + return ( +
  • + + {envKey} + + {configured ? ( + ✓ set + ) : ( + <> + onChange(envKey, e.target.value)} + onKeyDown={(e) => { + if (e.key === "Enter") { + e.preventDefault(); + onSave(envKey); + } + }} + disabled={d?.saving} + className="flex-1 px-2 py-1 rounded bg-zinc-800 border border-zinc-700 text-[11px] text-zinc-200 focus:outline-none focus:border-blue-500 disabled:opacity-50" + /> + + + )} + {d?.error && ( + + {d.error} + + )} +
    • + ); +} + +interface AnyOfEnvGroupProps { + members: string[]; + configuredKeys: Set; + drafts: Record; + onChange: (key: string, value: string) => void; + onSave: (key: string) => void; +} + +/** + * Renders an OR group: the user only needs to configure ONE of the + * members to satisfy the requirement. Once any member is configured + * the group shows a green banner identifying the satisfying key; the + * other inputs remain visible but muted so the user can still switch + * providers if they want (uncommon but cheap to support). + */ +function AnyOfEnvGroup({ + members, + configuredKeys, + drafts, + onChange, + onSave, +}: AnyOfEnvGroupProps) { + const satisfiedBy = members.find((m) => configuredKeys.has(m)); + return ( +
  • +
    + + Configure any one + + {satisfiedBy && ( + + ✓ using {satisfiedBy} + + )} +
    +
      + {members.map((m) => { + const isConfigured = configuredKeys.has(m); + const d = drafts[m]; + const dimmed = !!satisfiedBy && !isConfigured; return (
    • - {k} + {m} - {configured ? ( + {isConfigured ? ( ✓ set ) : ( <> onChange(k, e.target.value)} + onChange={(e) => onChange(m, e.target.value)} onKeyDown={(e) => { if (e.key === "Enter") { e.preventDefault(); - onSave(k); + onSave(m); } }} disabled={d?.saving} @@ -307,7 +482,7 @@ function EnvList({ />
    - +
    • ); } diff --git a/canvas/src/components/TemplatePalette.tsx b/canvas/src/components/TemplatePalette.tsx index b46ee155..8c40d022 100644 --- a/canvas/src/components/TemplatePalette.tsx +++ b/canvas/src/components/TemplatePalette.tsx @@ -6,7 +6,10 @@ import { useCanvasStore } from "@/store/canvas"; import type { WorkspaceData } from "@/store/socket"; import { type Template } from "@/lib/deploy-preflight"; import { useTemplateDeploy } from "@/hooks/useTemplateDeploy"; -import { OrgImportPreflightModal } from "./OrgImportPreflightModal"; +import { + OrgImportPreflightModal, + type EnvRequirement, +} from "./OrgImportPreflightModal"; import { ConfirmDialog } from "./ConfirmDialog"; import { Spinner } from "./Spinner"; import { showToast } from "./Toaster"; @@ -27,13 +30,18 @@ export interface OrgTemplate { /** Env vars that MUST be set as global secrets before the org can * import. Server refuses the import with 412 if any are missing; * the canvas preflights against /secrets/list to avoid the round - * trip. Aggregated from org-level + every workspace in the tree. */ - required_env?: string[]; + * trip. Aggregated from org-level + every workspace in the tree. + * + * Each entry is either a key name (strict) or an `{any_of: [...]}` + * group (any one of the listed members satisfies the requirement — + * e.g. `ANTHROPIC_API_KEY` OR `CLAUDE_CODE_OAUTH_TOKEN`). */ + required_env?: EnvRequirement[]; /** "Nice-to-have" tier. Import proceeds without them but features * may degrade — a channel's webhook posts get dropped, a fallback * LLM isn't available, etc. Surfaced to the user as a non-blocking - * warning with an "add now" affordance. */ - recommended_env?: string[]; + * warning with an "add now" affordance. Same union shape as + * `required_env`. */ + recommended_env?: EnvRequirement[]; } /** Fetch the list of org templates from the platform. Returns [] on error diff --git a/workspace-server/internal/handlers/org.go b/workspace-server/internal/handlers/org.go index a757b1e8..c31c81ee 100644 --- a/workspace-server/internal/handlers/org.go +++ b/workspace-server/internal/handlers/org.go @@ -5,6 +5,7 @@ package handlers import ( "context" + "encoding/json" "fmt" "log" "net/http" @@ -180,6 +181,108 @@ func NewOrgHandler(wh *WorkspaceHandler, b *events.Broadcaster, p *provisioner.P } } +// EnvRequirement is either a single env var name (strict: that exact +// var must be configured) or an any-of group (any one of the listed +// names satisfies the requirement). +// +// YAML shapes accepted: +// +// required_env: +// - GITHUB_TOKEN # single +// - any_of: [ANTHROPIC_API_KEY, CLAUDE_CODE_OAUTH_TOKEN] # OR group +// +// The any-of form exists because some runtimes accept either of two +// credential shapes — Claude Code takes ANTHROPIC_API_KEY or an OAuth +// token interchangeably, and forcing an org template to pick one +// would falsely block the other. For JSON (GET /org/templates), +// the same shapes round-trip: strings stay strings, groups stay +// {any_of: [...]}. +type EnvRequirement struct { + // Name is non-empty for a single required env var. + Name string + // AnyOf is non-empty for an OR group; any one member satisfies. + AnyOf []string +} + +// Members returns every env name this requirement considers — +// [Name] for single, AnyOf for groups. Used by preflight, collect, +// and the name-validation regex gate. +func (e EnvRequirement) Members() []string { + if e.Name != "" { + return []string{e.Name} + } + return e.AnyOf +} + +// IsSatisfied reports whether any member of the requirement is +// present in `configured`. Single: exact-match. AnyOf: at least +// one hit. +func (e EnvRequirement) IsSatisfied(configured map[string]struct{}) bool { + for _, m := range e.Members() { + if _, ok := configured[m]; ok { + return true + } + } + return false +} + +// UnmarshalYAML accepts either a scalar (string → single) or a map +// with an `any_of` list (→ group). +func (e *EnvRequirement) UnmarshalYAML(value *yaml.Node) error { + if value.Kind == yaml.ScalarNode { + var s string + if err := value.Decode(&s); err != nil { + return err + } + e.Name = s + return nil + } + var alt struct { + AnyOf []string `yaml:"any_of"` + } + if err := value.Decode(&alt); err != nil { + return fmt.Errorf("env requirement must be a string or {any_of: [...]}: %w", err) + } + if len(alt.AnyOf) == 0 { + return fmt.Errorf("env requirement any_of must contain at least one env var") + } + e.AnyOf = alt.AnyOf + return nil +} + +// MarshalJSON emits the dual shape so GET /org/templates callers get +// {"required_env": ["GITHUB_TOKEN", {"any_of": [...]}]}, matching +// the YAML syntax. +func (e EnvRequirement) MarshalJSON() ([]byte, error) { + if e.Name != "" { + return json.Marshal(e.Name) + } + return json.Marshal(struct { + AnyOf []string `json:"any_of"` + }{AnyOf: e.AnyOf}) +} + +// UnmarshalJSON is the inverse — accepts the same dual shape so +// POST /org/import with an inline `template` body works too. +func (e *EnvRequirement) UnmarshalJSON(data []byte) error { + var s string + if err := json.Unmarshal(data, &s); err == nil { + e.Name = s + return nil + } + var alt struct { + AnyOf []string `json:"any_of"` + } + if err := json.Unmarshal(data, &alt); err != nil { + return fmt.Errorf("env requirement must be a string or {any_of: [...]}: %w", err) + } + if len(alt.AnyOf) == 0 { + return fmt.Errorf("env requirement any_of must contain at least one env var") + } + e.AnyOf = alt.AnyOf + return nil +} + // OrgTemplate is the YAML structure for an org hierarchy. type OrgTemplate struct { Name string `yaml:"name" json:"name"` @@ -189,20 +292,18 @@ type OrgTemplate struct { // GlobalMemories is a list of org-wide memories seeded as GLOBAL scope // on the first root workspace (PM) during org import. Issue #1050. GlobalMemories []models.MemorySeed `yaml:"global_memories" json:"global_memories"` - // RequiredEnv is the union of env vars WHICH MUST be set globally (or + // RequiredEnv lists env vars that MUST be configured globally (or // on every workspace in the subtree that needs them) before import - // will succeed. Declared at the org level for shared creds, also - // extensible per-workspace via OrgWorkspace.RequiredEnv for team- - // scoped credentials (e.g. LEGAL_VAULT_TOKEN only matters if the Legal - // subtree is part of this template). The canvas preflights both. - RequiredEnv []string `yaml:"required_env" json:"required_env"` + // succeeds. Each entry is either a plain string (strict) or an + // {any_of: [...]} group (at least one member must be set). Declared + // at the org level for shared creds; also extensible per-workspace + // via OrgWorkspace.RequiredEnv for team-scoped credentials. + RequiredEnv []EnvRequirement `yaml:"required_env" json:"required_env"` // RecommendedEnv is the "nice-to-have" tier — import still succeeds - // without them, but features degrade. The canvas shows them as a - // yellow warning ("add now for best experience") rather than a - // blocking red. Example: SLACK_WEBHOOK_URL for a team whose agents - // occasionally post status updates, or ANTHROPIC_API_KEY when an - // agent might fall back to claude from its primary provider. - RecommendedEnv []string `yaml:"recommended_env" json:"recommended_env"` + // without them, but features degrade. Same single|any_of shape as + // RequiredEnv so a recommended OR group reads "set any one of these + // to unlock the feature; all missing = warning". + RecommendedEnv []EnvRequirement `yaml:"recommended_env" json:"recommended_env"` } type OrgDefaults struct { @@ -315,10 +416,11 @@ type OrgWorkspace struct { // OrgTemplate.RequiredEnv / RecommendedEnv. A workspace's subtree // inherits: a parent declaring ANTHROPIC_API_KEY as required // means every descendant considers it required too (no override - // needed at each leaf). - RequiredEnv []string `yaml:"required_env" json:"required_env"` - RecommendedEnv []string `yaml:"recommended_env" json:"recommended_env"` - Children []OrgWorkspace `yaml:"children" json:"children"` + // needed at each leaf). Same single|any_of shape as the org-level + // lists. + RequiredEnv []EnvRequirement `yaml:"required_env" json:"required_env"` + RecommendedEnv []EnvRequirement `yaml:"recommended_env" json:"recommended_env"` + Children []OrgWorkspace `yaml:"children" json:"children"` } // ListTemplates handles GET /org/templates — lists available org templates. @@ -483,10 +585,14 @@ func (h *OrgHandler) Import(c *gin.Context) { }) return } - var missing []string - for _, k := range required { - if _, ok := configured[k]; !ok { - missing = append(missing, k) + var missing []EnvRequirement + for _, req := range required { + // For a single requirement this is exact-match; for an + // any-of group, any one member satisfies. Groups whose + // alternative is already configured drop out here — the + // user doesn't need to re-configure them. + if !req.IsSatisfied(configured) { + missing = append(missing, req) } } if len(missing) > 0 { diff --git a/workspace-server/internal/handlers/org_import.go b/workspace-server/internal/handlers/org_import.go index 166d9a19..1c957943 100644 --- a/workspace-server/internal/handlers/org_import.go +++ b/workspace-server/internal/handlers/org_import.go @@ -555,56 +555,165 @@ func (h *OrgHandler) createWorkspaceTree(ws OrgWorkspace, parentID *string, absX // problem at a single choke point. var envVarNamePattern = regexp.MustCompile(`^[A-Z][A-Z0-9_]{0,127}$`) +// sanitizeEnvMembers filters a requirement's member list through the +// name-validation regex, logging rejections. Returns the filtered +// list and a boolean indicating whether any valid members remain. +// Used so a group containing one valid + one bogus name is kept +// (valid member carries the group) rather than silently dropped. +func sanitizeEnvMembers(members []string, where string) ([]string, bool) { + out := make([]string, 0, len(members)) + for _, k := range members { + if !envVarNamePattern.MatchString(k) { + if k != "" { + log.Printf("collectOrgEnv: rejecting invalid env var name %q from %s (must match %s)", k, where, envVarNamePattern) + } + continue + } + out = append(out, k) + } + return out, len(out) > 0 +} + +// envRequirementKey canonicalises a requirement for dedup — sorted +// member list joined with NUL so `any_of: [A, B]` and `any_of: [B, A]` +// collapse to the same key. Single requirements are length-1 groups. +func envRequirementKey(members []string) string { + cp := append([]string(nil), members...) + sort.Strings(cp) + return strings.Join(cp, "\x00") +} + // collectOrgEnv walks the whole template tree and returns the union of // required_env and recommended_env declared anywhere — at the org -// level, on root workspaces, or on any nested child. Deduplicates so -// the canvas sees a clean set. An env var declared as required at ANY -// level wins over recommended (required is strictly stricter). -// Entries that fail envVarNamePattern are dropped with a log line so -// a bad template surfaces in operator logs without breaking import. -func collectOrgEnv(tmpl *OrgTemplate) (required, recommended []string) { - req := map[string]struct{}{} - rec := map[string]struct{}{} - accept := func(into map[string]struct{}, src []string, where string) { - for _, k := range src { - if !envVarNamePattern.MatchString(k) { - if k != "" { - log.Printf("collectOrgEnv: rejecting invalid env var name %q from %s (must match %s)", k, where, envVarNamePattern) - } +// level, on root workspaces, or on any nested child. Deduplicates by +// group membership (same set of members = same requirement) and +// sorts deterministically so the canvas sees a stable order. +// +// "Required wins" rules: +// +// - A requirement that appears in BOTH required and recommended +// (same members) surfaces only as required. +// - A single-name requirement (e.g. "API_KEY") and a group that +// contains that same name (e.g. {any_of: [API_KEY, OTHER]}) are +// NOT deduplicated — they're semantically different (strict vs +// satisfiable-by-alternative) and the stricter "single" one wins, +// so the any-of group is dropped when its members overlap with a +// strict requirement declared elsewhere. +// +// Invalid names fail envVarNamePattern; the filter is applied per +// group so a group with one bogus entry keeps the rest. A group +// whose ALL members are invalid is dropped entirely with a log. +func collectOrgEnv(tmpl *OrgTemplate) (required, recommended []EnvRequirement) { + reqByKey := map[string]EnvRequirement{} + recByKey := map[string]EnvRequirement{} + // Names covered by strict (single) required entries. A group in + // EITHER tier whose any-of contains ONE of these names is + // dominated by the strict requirement and gets dropped on the + // second pass. + strictRequiredNames := map[string]struct{}{} + + accept := func(into map[string]EnvRequirement, src []EnvRequirement, where string, markStrict bool) { + for _, req := range src { + members, ok := sanitizeEnvMembers(req.Members(), where) + if !ok { continue } - into[k] = struct{}{} + key := envRequirementKey(members) + if _, exists := into[key]; exists { + continue + } + if req.Name != "" && len(members) == 1 { + into[key] = EnvRequirement{Name: members[0]} + if markStrict { + strictRequiredNames[members[0]] = struct{}{} + } + } else { + into[key] = EnvRequirement{AnyOf: members} + } } } - accept(req, tmpl.RequiredEnv, "template root") - accept(rec, tmpl.RecommendedEnv, "template root") + accept(reqByKey, tmpl.RequiredEnv, "template root", true) + accept(recByKey, tmpl.RecommendedEnv, "template root", false) var walk func([]OrgWorkspace) walk = func(ws []OrgWorkspace) { for _, w := range ws { - accept(req, w.RequiredEnv, "workspace "+w.Name) - accept(rec, w.RecommendedEnv, "workspace "+w.Name) + accept(reqByKey, w.RequiredEnv, "workspace "+w.Name, true) + accept(recByKey, w.RecommendedEnv, "workspace "+w.Name, false) walk(w.Children) } } walk(tmpl.Workspaces) - // Required wins — a key recommended at one layer and required at - // another surfaces only on the required side. - for k := range req { - delete(rec, k) + + // Required wins across tiers: any requirement whose members + // overlap with a strict required name gets dropped from + // recommended. Keeps the canvas modal from showing the same + // key in both sections. + prune := func(from map[string]EnvRequirement) { + for k, r := range from { + for _, m := range r.Members() { + if _, strict := strictRequiredNames[m]; strict { + delete(from, k) + break + } + } + } } - required = make([]string, 0, len(req)) - for k := range req { - required = append(required, k) + prune(recByKey) + + // Same-tier: a strict required X dominates any-of groups in + // required that CONTAIN X (a group saying "any of X, Y" is + // automatically satisfied when X is required anyway, so it's + // redundant). Same logic applied to recommended. + pruneSameTier := func(tier map[string]EnvRequirement) { + strictInTier := map[string]struct{}{} + for _, r := range tier { + if r.Name != "" { + strictInTier[r.Name] = struct{}{} + } + } + for k, r := range tier { + if len(r.AnyOf) == 0 { + continue + } + for _, m := range r.AnyOf { + if _, strict := strictInTier[m]; strict { + delete(tier, k) + break + } + } + } } - recommended = make([]string, 0, len(rec)) - for k := range rec { - recommended = append(recommended, k) - } - sort.Strings(required) - sort.Strings(recommended) + pruneSameTier(reqByKey) + pruneSameTier(recByKey) + + required = flattenAndSortRequirements(reqByKey) + recommended = flattenAndSortRequirements(recByKey) return required, recommended } +func flattenAndSortRequirements(by map[string]EnvRequirement) []EnvRequirement { + out := make([]EnvRequirement, 0, len(by)) + for _, r := range by { + out = append(out, r) + } + sort.Slice(out, func(i, j int) bool { + // Sort singles first by name; groups after, ordered by + // joined-member string. Gives the canvas a deterministic + // render order so the same template always produces the + // same modal layout. + iSingle := out[i].Name != "" + jSingle := out[j].Name != "" + if iSingle != jSingle { + return iSingle + } + if iSingle { + return out[i].Name < out[j].Name + } + return envRequirementKey(out[i].AnyOf) < envRequirementKey(out[j].AnyOf) + }) + return out +} + // loadConfiguredGlobalSecretKeys returns the set of key names present // in global_secrets WHERE the encrypted_value is non-empty. Filtering // on the payload size catches the failure mode where a row was diff --git a/workspace-server/internal/handlers/org_test.go b/workspace-server/internal/handlers/org_test.go index 306440ae..41d3b1bf 100644 --- a/workspace-server/internal/handlers/org_test.go +++ b/workspace-server/internal/handlers/org_test.go @@ -1,6 +1,7 @@ package handlers import ( + "sort" "strings" "testing" "time" @@ -655,19 +656,54 @@ func TestOrgImport_ScheduleComputeError(t *testing.T) { // Org env-preflight aggregation (collectOrgEnv) // ============================================================================ +// strictReq builds a slice of single-name EnvRequirements for test +// fixtures. Equivalent to the old []string literal but wrapped in +// the new union shape. +func strictReq(names ...string) []EnvRequirement { + out := make([]EnvRequirement, 0, len(names)) + for _, n := range names { + out = append(out, EnvRequirement{Name: n}) + } + return out +} + +// anyOfReq builds a single any-of EnvRequirement for test fixtures. +func anyOfReq(names ...string) EnvRequirement { + return EnvRequirement{AnyOf: append([]string(nil), names...)} +} + +// reqNames flattens a slice of EnvRequirements into a single comparable +// slice: single-name reqs contribute their Name, any-of reqs contribute +// "anyOf(A|B|C)" with members sorted for deterministic output. Lets +// tests assert against a string form regardless of which kind each +// entry takes. +func reqNames(reqs []EnvRequirement) []string { + out := make([]string, 0, len(reqs)) + for _, r := range reqs { + if r.Name != "" { + out = append(out, r.Name) + continue + } + members := append([]string(nil), r.AnyOf...) + sort.Strings(members) + out = append(out, "anyOf("+strings.Join(members, "|")+")") + } + return out +} + func TestCollectOrgEnv_UnionAcrossLevels(t *testing.T) { tmpl := &OrgTemplate{ - RequiredEnv: []string{"ANTHROPIC_API_KEY"}, - RecommendedEnv: []string{"SLACK_WEBHOOK_URL"}, + RequiredEnv: strictReq("ANTHROPIC_API_KEY"), + RecommendedEnv: strictReq("SLACK_WEBHOOK_URL"), Workspaces: []OrgWorkspace{ { - Name: "Root", - RequiredEnv: []string{"GITHUB_TOKEN"}, + Name: "Root", + RequiredEnv: strictReq("GITHUB_TOKEN"), Children: []OrgWorkspace{ { Name: "Leaf", - RequiredEnv: []string{"OPENROUTER_API_KEY"}, - RecommendedEnv: []string{"DISCORD_WEBHOOK_URL"}, + RequiredEnv: strictReq("OPENROUTER_API_KEY"), + RecommendedEnv: strictReq("DISCORD_WEBHOOK_URL"), }, }, }, @@ -676,12 +712,12 @@ func TestCollectOrgEnv_UnionAcrossLevels(t *testing.T) { req, rec := collectOrgEnv(tmpl) // Required is the union of top-level + root + leaf. wantReq := []string{"ANTHROPIC_API_KEY", "GITHUB_TOKEN", "OPENROUTER_API_KEY"} - if !stringSlicesEqual(req, wantReq) { - t.Errorf("required mismatch: got %v, want %v", req, wantReq) + if !stringSlicesEqual(reqNames(req), wantReq) { + t.Errorf("required mismatch: got %v, want %v", reqNames(req), wantReq) } wantRec := []string{"DISCORD_WEBHOOK_URL", "SLACK_WEBHOOK_URL"} - if !stringSlicesEqual(rec, wantRec) { - t.Errorf("recommended mismatch: got %v, want %v", rec, wantRec) + if !stringSlicesEqual(reqNames(rec), wantRec) { + t.Errorf("recommended mismatch: got %v, want %v", reqNames(rec), wantRec) } } @@ -691,17 +727,17 @@ func TestCollectOrgEnv_RequiredWinsOverRecommended(t *testing.T) { // declaration is strictly stricter than a recommended one, and // listing it in both tiers would confuse the preflight modal. tmpl := &OrgTemplate{ - RecommendedEnv: []string{"API_KEY"}, + RecommendedEnv: strictReq("API_KEY"), Workspaces: []OrgWorkspace{ - {Name: "X", RequiredEnv: []string{"API_KEY"}}, + {Name: "X", RequiredEnv: strictReq("API_KEY")}, }, } req, rec := collectOrgEnv(tmpl) - if len(req) != 1 || req[0] != "API_KEY" { - t.Errorf("required should contain API_KEY, got %v", req) + if len(req) != 1 || req[0].Name != "API_KEY" { + t.Errorf("required should contain API_KEY, got %v", reqNames(req)) } - for _, k := range rec { - if k == "API_KEY" { + for _, r := range rec { + if r.Name == "API_KEY" { t.Errorf("API_KEY must not appear in recommended once required elsewhere") } } @@ -710,17 +746,17 @@ func TestCollectOrgEnv_RequiredWinsOverRecommended(t *testing.T) { func TestCollectOrgEnv_Dedup(t *testing.T) { // Same key declared twice at different levels should appear once. tmpl := &OrgTemplate{ - RequiredEnv: []string{"K", "K"}, + RequiredEnv: strictReq("K", "K"), Workspaces: []OrgWorkspace{ - {Name: "A", RequiredEnv: []string{"K"}}, - {Name: "B", RequiredEnv: []string{"K"}, Children: []OrgWorkspace{ - {Name: "C", RequiredEnv: []string{"K"}}, + {Name: "A", RequiredEnv: strictReq("K")}, + {Name: "B", RequiredEnv: strictReq("K"), Children: []OrgWorkspace{ + {Name: "C", RequiredEnv: strictReq("K")}, }}, }, } req, _ := collectOrgEnv(tmpl) - if len(req) != 1 || req[0] != "K" { - t.Errorf("dedup failed: got %v, want [K]", req) + if len(req) != 1 || req[0].Name != "K" { + t.Errorf("dedup failed: got %v, want [K]", reqNames(req)) } } @@ -728,7 +764,7 @@ func TestCollectOrgEnv_Empty(t *testing.T) { tmpl := &OrgTemplate{} req, rec := collectOrgEnv(tmpl) if len(req) != 0 || len(rec) != 0 { - t.Errorf("empty template should produce empty slices, got req=%v rec=%v", req, rec) + t.Errorf("empty template should produce empty slices, got req=%v rec=%v", reqNames(req), reqNames(rec)) } } @@ -754,17 +790,17 @@ func TestCollectOrgEnv_RequiredWinsOnSameStruct(t *testing.T) { Workspaces: []OrgWorkspace{ { Name: "X", - RequiredEnv: []string{"API_KEY"}, - RecommendedEnv: []string{"API_KEY"}, + RequiredEnv: strictReq("API_KEY"), + RecommendedEnv: strictReq("API_KEY"), }, }, } req, rec := collectOrgEnv(tmpl) - if len(req) != 1 || req[0] != "API_KEY" { - t.Errorf("required should contain API_KEY once, got %v", req) + if len(req) != 1 || req[0].Name != "API_KEY" { + t.Errorf("required should contain API_KEY once, got %v", reqNames(req)) } - for _, k := range rec { - if k == "API_KEY" { + for _, r := range rec { + if r.Name == "API_KEY" { t.Errorf("API_KEY must not appear in recommended when also required on same struct") } } @@ -776,7 +812,7 @@ func TestCollectOrgEnv_RejectsInvalidNames(t *testing.T) { // asserted here; the output slice assertion is enough to prove the // filter fires. tmpl := &OrgTemplate{ - RequiredEnv: []string{ + RequiredEnv: strictReq( "VALID_ONE", "lowercase_bad", "../../etc/passwd", @@ -785,10 +821,191 @@ func TestCollectOrgEnv_RejectsInvalidNames(t *testing.T) { "'; DROP TABLE users;--", "", "A", // single char — still valid per regex + ), + } + req, _ := collectOrgEnv(tmpl) + if !stringSlicesEqual(reqNames(req), []string{"A", "VALID_ONE"}) { + t.Errorf("expected only valid names, got %v", reqNames(req)) + } +} + +// TestEnvRequirement_UnmarshalYAML proves the on-disk YAML shape +// (scalar OR `{any_of: [...]}` block) round-trips into EnvRequirement +// correctly. The preflight pipeline reads user-authored org.yaml +// files; a regression here would silently drop requirements. +func TestEnvRequirement_UnmarshalYAML(t *testing.T) { + src := ` +required_env: + - GITHUB_TOKEN + - any_of: + - ANTHROPIC_API_KEY + - CLAUDE_CODE_OAUTH_TOKEN +` + var parsed struct { + RequiredEnv []EnvRequirement `yaml:"required_env"` + } + if err := yaml.Unmarshal([]byte(src), &parsed); err != nil { + t.Fatalf("unmarshal failed: %v", err) + } + if len(parsed.RequiredEnv) != 2 { + t.Fatalf("want 2 requirements, got %d", len(parsed.RequiredEnv)) + } + if parsed.RequiredEnv[0].Name != "GITHUB_TOKEN" { + t.Errorf("first should be strict GITHUB_TOKEN, got %+v", parsed.RequiredEnv[0]) + } + if parsed.RequiredEnv[1].Name != "" || len(parsed.RequiredEnv[1].AnyOf) != 2 { + t.Errorf("second should be any-of group, got %+v", parsed.RequiredEnv[1]) + } +} + +// TestEnvRequirement_UnmarshalYAML_RejectsEmptyAnyOf guards against a +// template that ships `any_of: []` — ambiguous semantics (impossible +// to satisfy), so the parser must fail loudly rather than silently +// pass a never-satisfiable requirement through the preflight. +func TestEnvRequirement_UnmarshalYAML_RejectsEmptyAnyOf(t *testing.T) { + src := ` +required_env: + - any_of: [] +` + var parsed struct { + RequiredEnv []EnvRequirement `yaml:"required_env"` + } + err := yaml.Unmarshal([]byte(src), &parsed) + if err == nil { + t.Errorf("expected error for empty any_of, got nil: %+v", parsed) + } +} + +// --------------------------------------------------------------------- +// any_of group tests — the new EnvRequirement union shape allows a +// single requirement to be satisfied by any of a list of members (e.g. +// ANTHROPIC_API_KEY OR CLAUDE_CODE_OAUTH_TOKEN). collectOrgEnv + +// IsSatisfied together must handle this correctly. +// --------------------------------------------------------------------- + +func TestEnvRequirement_IsSatisfied(t *testing.T) { + configured := map[string]struct{}{ + "ANTHROPIC_API_KEY": {}, + "GITHUB_TOKEN": {}, + } + tests := []struct { + name string + req EnvRequirement + want bool + }{ + {"strict present", EnvRequirement{Name: "ANTHROPIC_API_KEY"}, true}, + {"strict absent", EnvRequirement{Name: "MISSING_KEY"}, false}, + {"any-of first member present", anyOfReq("ANTHROPIC_API_KEY", "CLAUDE_CODE_OAUTH_TOKEN"), true}, + {"any-of second member present", anyOfReq("CLAUDE_CODE_OAUTH_TOKEN", "ANTHROPIC_API_KEY"), true}, + {"any-of none present", anyOfReq("OPENAI_API_KEY", "CLAUDE_CODE_OAUTH_TOKEN"), false}, + {"any-of single member present", anyOfReq("GITHUB_TOKEN"), true}, + } + for _, tt := range tests { + if got := tt.req.IsSatisfied(configured); got != tt.want { + t.Errorf("%s: got %v, want %v", tt.name, got, tt.want) + } + } +} + +func TestCollectOrgEnv_AnyOfGroupPreserved(t *testing.T) { + // A group with two alternatives should come through as a single + // EnvRequirement carrying both members. + tmpl := &OrgTemplate{ + RequiredEnv: []EnvRequirement{ + anyOfReq("ANTHROPIC_API_KEY", "CLAUDE_CODE_OAUTH_TOKEN"), }, } req, _ := collectOrgEnv(tmpl) - if !stringSlicesEqual(req, []string{"A", "VALID_ONE"}) { - t.Errorf("expected only valid names, got %v", req) + if len(req) != 1 { + t.Fatalf("expected 1 requirement, got %d: %v", len(req), reqNames(req)) + } + if req[0].Name != "" { + t.Errorf("expected any-of group, got strict name %q", req[0].Name) + } + wantMembers := []string{"ANTHROPIC_API_KEY", "CLAUDE_CODE_OAUTH_TOKEN"} + got := append([]string(nil), req[0].AnyOf...) + sort.Strings(got) + if !stringSlicesEqual(got, wantMembers) { + t.Errorf("any-of members mismatch: got %v, want %v", got, wantMembers) + } +} + +func TestCollectOrgEnv_AnyOfGroupDedup(t *testing.T) { + // Two identical groups (members in different order) declared at + // different levels must collapse to one. + tmpl := &OrgTemplate{ + RequiredEnv: []EnvRequirement{ + anyOfReq("ANTHROPIC_API_KEY", "CLAUDE_CODE_OAUTH_TOKEN"), + }, + Workspaces: []OrgWorkspace{ + { + Name: "Root", + RequiredEnv: []EnvRequirement{ + anyOfReq("CLAUDE_CODE_OAUTH_TOKEN", "ANTHROPIC_API_KEY"), + }, + }, + }, + } + req, _ := collectOrgEnv(tmpl) + if len(req) != 1 { + t.Errorf("expected 1 requirement after dedup, got %d: %v", len(req), reqNames(req)) + } +} + +func TestCollectOrgEnv_StrictDominatesGroup(t *testing.T) { + // If a strict requirement X is declared anywhere, any-of groups + // that CONTAIN X are redundant — the strict requirement will force + // X to be configured, which satisfies any group mentioning it too. + // Same-tier pruning drops the group. + tmpl := &OrgTemplate{ + RequiredEnv: []EnvRequirement{ + {Name: "ANTHROPIC_API_KEY"}, + anyOfReq("ANTHROPIC_API_KEY", "CLAUDE_CODE_OAUTH_TOKEN"), + }, + } + req, _ := collectOrgEnv(tmpl) + if len(req) != 1 || req[0].Name != "ANTHROPIC_API_KEY" { + t.Errorf("strict should dominate group, got %v", reqNames(req)) + } +} + +func TestCollectOrgEnv_StrictRequiredDominatesRecommendedGroup(t *testing.T) { + // Cross-tier: a strict required X drops any-of groups in the + // recommended tier that mention X. + tmpl := &OrgTemplate{ + RequiredEnv: strictReq("ANTHROPIC_API_KEY"), + RecommendedEnv: []EnvRequirement{ + anyOfReq("ANTHROPIC_API_KEY", "CLAUDE_CODE_OAUTH_TOKEN"), + {Name: "SLACK_WEBHOOK_URL"}, + }, + } + req, rec := collectOrgEnv(tmpl) + if len(req) != 1 || req[0].Name != "ANTHROPIC_API_KEY" { + t.Errorf("required mismatch: got %v", reqNames(req)) + } + // The any-of group should have been pruned; only SLACK remains. + if len(rec) != 1 || rec[0].Name != "SLACK_WEBHOOK_URL" { + t.Errorf("recommended mismatch: got %v, want [SLACK_WEBHOOK_URL]", reqNames(rec)) + } +} + +func TestCollectOrgEnv_AnyOfWithInvalidMemberKeepsValidOnes(t *testing.T) { + // A group with one valid + one invalid member should keep the + // valid one (group carried by any remaining legitimate name). A + // group where ALL members are invalid is dropped entirely. + tmpl := &OrgTemplate{ + RequiredEnv: []EnvRequirement{ + anyOfReq("VALID_ONE", "lowercase_bad"), + anyOfReq("'; DROP TABLE;--", ""), + }, + } + req, _ := collectOrgEnv(tmpl) + if len(req) != 1 { + t.Fatalf("expected 1 requirement, got %d: %v", len(req), reqNames(req)) + } + // The remaining group has only one valid member, so it gets + // promoted to a single-name requirement (len(members)==1 path). + if req[0].Name != "VALID_ONE" && !stringSlicesEqual(req[0].AnyOf, []string{"VALID_ONE"}) { + t.Errorf("expected VALID_ONE to survive, got %v", reqNames(req)) } }