From ab41503d25b00056fa57a624507d3c52ec21f996 Mon Sep 17 00:00:00 2001 From: Molecule AI Integration Tester Date: Sun, 10 May 2026 07:11:16 +0000 Subject: [PATCH] fix(org): add per-workspace RequiredEnv preflight check (#232) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Before returning 201 on /org/import, verify that every RequiredEnv declared at the workspace level is covered by either: (a) a global secret key (already validated by the existing preflight) (b) a key present in the workspace's .env files (org root .env + per-workspace /.env), matching the resolution order used by createWorkspaceTree at runtime Previously, collectOrgEnv correctly walked all tmpl.Workspaces[].RequiredEnv and added them to the global preflight check, but loadConfiguredGlobalSecretKeys only checked global_secrets. Workspace-specific .env files are injected into workspace_secrets AFTER the 201 response, so an unsatisfied per-workspace RequiredEnv returned 201 and the workspace came up NOT CONFIGURED — breaking on every LLM call with no signal to the operator. Changes: - org_import.go: add PerWorkspaceUnsatisfied struct + collectPerWorkspaceUnsatisfied (mirrors createWorkspaceTree's three-source .env resolution stack) - org.go: after the global preflight block, call collectPerWorkspaceUnsatisfied if orgBaseDir != ""; return 412 with per-workspace details before creating any workspaces - org_workspace_required_env_test.go: 8 unit tests covering global coverage, .env coverage, missing keys, any-of groups, nested children, empty orgBaseDir, and multiple workspaces Co-Authored-By: Claude Opus 4.7 --- workspace-server/internal/handlers/org.go | 25 ++ .../internal/handlers/org_import.go | 53 ++++ .../org_workspace_required_env_test.go | 226 ++++++++++++++++++ 3 files changed, 304 insertions(+) create mode 100644 workspace-server/internal/handlers/org_workspace_required_env_test.go diff --git a/workspace-server/internal/handlers/org.go b/workspace-server/internal/handlers/org.go index 8b5c4585..4a67d633 100644 --- a/workspace-server/internal/handlers/org.go +++ b/workspace-server/internal/handlers/org.go @@ -697,6 +697,31 @@ func (h *OrgHandler) Import(c *gin.Context) { }) return } + + // Per-workspace RequiredEnv preflight: checks that every RequiredEnv + // declared at the workspace level is covered by either (a) a global + // secret key (already validated above) or (b) a key present in the + // workspace's on-disk .env files (org root .env + per-workspace + // /.env). If neither covers the key the workspace is + // imported NOT CONFIGURED, which silently breaks the workspace at + // start time — the container boots without the required credential + // and every LLM call 401s or fails silently. Issue #232. + // orgBaseDir is empty when importing via body.Template (inline YAML); + // in that case we cannot check .env files, so we skip this check + // and fall back to the global-only gate above (which correctly + // rejects any strict requirement not covered by global_secrets). + if orgBaseDir != "" { + wsMissing := collectPerWorkspaceUnsatisfied(tmpl.Workspaces, orgBaseDir, configured) + if len(wsMissing) > 0 { + c.JSON(http.StatusPreconditionFailed, gin.H{ + "error": "missing per-workspace required environment variables", + "missing_workspace_env": wsMissing, + "template": tmpl.Name, + "suggestion": "add these keys to the workspace's .env file or set them as global secrets before importing", + }) + return + } + } } results := []map[string]interface{}{} diff --git a/workspace-server/internal/handlers/org_import.go b/workspace-server/internal/handlers/org_import.go index 2e06479f..6576043e 100644 --- a/workspace-server/internal/handlers/org_import.go +++ b/workspace-server/internal/handlers/org_import.go @@ -936,6 +936,59 @@ func flattenAndSortRequirements(by map[string]EnvRequirement) []EnvRequirement { // can investigate. const globalSecretsPreflightLimit = 10000 +// PerWorkspaceUnsatisfied describes one per-workspace RequiredEnv that is +// not covered by either a global secret or a key present in the +// corresponding .env file. +type PerWorkspaceUnsatisfied struct { + Workspace string `json:"workspace"` + FilesDir string `json:"files_dir,omitempty"` + Unsatisfied EnvRequirement `json:"unsatisfied_env"` +} + +// collectPerWorkspaceUnsatisfied recursively walks workspaces and returns +// per-workspace RequiredEnv entries that are not covered by (a) a global +// secret key or (b) a key present in the workspace's .env file(s) (org root +// .env + per-workspace /.env). This complements +// collectOrgEnv + loadConfiguredGlobalSecretKeys, which together only +// validate global-level RequiredEnv against global_secrets. The .env +// lookup mirrors the runtime resolution in createWorkspaceTree so that +// the preflight result matches what the container actually receives at +// start time. +func collectPerWorkspaceUnsatisfied(workspaces []OrgWorkspace, orgBaseDir string, globalSecrets map[string]struct{}) []PerWorkspaceUnsatisfied { + var out []PerWorkspaceUnsatisfied + var walk func([]OrgWorkspace) + walk = func(wsList []OrgWorkspace) { + for _, ws := range wsList { + // Build the set of keys available to this workspace from .env. + // This is the same three-source stack that createWorkspaceTree + // injects into the container: + // 1. Org root .env (parseEnvFile, no filesDir) + // 2. Workspace /.env (if filesDir is set) + // 3. Persona bootstrap env (MOLECULE_PERSONA_ROOT//env) + // Items 1+2 are on-disk and testable; item 3 is host-only and + // skipped here (persona env does NOT satisfy required_env — + // it carries identity tokens, not workspace LLM keys). + envFromFiles := loadWorkspaceEnv(orgBaseDir, ws.FilesDir) + for _, req := range ws.RequiredEnv { + if req.IsSatisfied(globalSecrets) { + continue // covered by a global secret + } + if req.IsSatisfied(envFromFiles) { + continue // covered by a per-workspace .env file + } + out = append(out, PerWorkspaceUnsatisfied{ + Workspace: ws.Name, + FilesDir: ws.FilesDir, + Unsatisfied: req, + }) + } + walk(ws.Children) + } + } + walk(workspaces) + return out +} + func loadConfiguredGlobalSecretKeys(ctx context.Context) (map[string]struct{}, error) { rows, err := db.DB.QueryContext(ctx, `SELECT key FROM global_secrets WHERE octet_length(encrypted_value) > 0 LIMIT $1`, diff --git a/workspace-server/internal/handlers/org_workspace_required_env_test.go b/workspace-server/internal/handlers/org_workspace_required_env_test.go new file mode 100644 index 00000000..a54845d2 --- /dev/null +++ b/workspace-server/internal/handlers/org_workspace_required_env_test.go @@ -0,0 +1,226 @@ +package handlers + +import ( + "os" + "path/filepath" + "testing" +) + +// TestCollectPerWorkspaceUnsatisfied_BothFiles covers the case where a key +// is present in both the org root .env and the workspace-specific .env. Both +// should satisfy the requirement (no entry in output). +func TestCollectPerWorkspaceUnsatisfied_BothFiles(t *testing.T) { + tmp := t.TempDir() + writeEnvFile(t, tmp, ".env", "PER_WS_KEY=globalvalue") + writeEnvFile(t, tmp, "ws-a/.env", "PER_WS_KEY=wsvalue") + + workspaces := []OrgWorkspace{ + {Name: "ws-a", FilesDir: "ws-a", RequiredEnv: []EnvRequirement{{Name: "PER_WS_KEY"}}}, + } + + // Global secret covers it. + globals := map[string]struct{}{"PER_WS_KEY": {}} + missing := collectPerWorkspaceUnsatisfied(workspaces, tmp, globals) + + if len(missing) != 0 { + t.Errorf("PER_WS_KEY present in global + .env: should be satisfied, got %d missing", len(missing)) + } +} + +// TestCollectPerWorkspaceUnsatisfied_WorkspaceEnvOnly covers a key present +// only in the workspace-specific .env file (not global). Should be satisfied. +func TestCollectPerWorkspaceUnsatisfied_WorkspaceEnvOnly(t *testing.T) { + tmp := t.TempDir() + writeEnvFile(t, tmp, "dev-lead/.env", "WORKSPACE_KEY=val") + + workspaces := []OrgWorkspace{ + {Name: "Dev Lead", FilesDir: "dev-lead", RequiredEnv: []EnvRequirement{{Name: "WORKSPACE_KEY"}}}, + } + + globals := map[string]struct{}{} // nothing in global + missing := collectPerWorkspaceUnsatisfied(workspaces, tmp, globals) + + if len(missing) != 0 { + t.Errorf("WORKSPACE_KEY in ws .env only: should be satisfied, got %d missing", len(missing)) + } +} + +// TestCollectPerWorkspaceUnsatisfied_OrgRootEnvOnly covers a key present +// only in the org root .env file (not per-workspace). Should be satisfied. +func TestCollectPerWorkspaceUnsatisfied_OrgRootEnvOnly(t *testing.T) { + tmp := t.TempDir() + writeEnvFile(t, tmp, ".env", "ORG_ROOT_KEY=val") + + workspaces := []OrgWorkspace{ + {Name: "ws-b", FilesDir: "ws-b", RequiredEnv: []EnvRequirement{{Name: "ORG_ROOT_KEY"}}}, + } + + globals := map[string]struct{}{} + missing := collectPerWorkspaceUnsatisfied(workspaces, tmp, globals) + + if len(missing) != 0 { + t.Errorf("ORG_ROOT_KEY in org root .env only: should be satisfied, got %d missing", len(missing)) + } +} + +// TestCollectPerWorkspaceUnsatisfied_GlobalCovers checks that a global +// secret alone satisfies a per-workspace RequiredEnv even when the .env +// files don't have the key. +func TestCollectPerWorkspaceUnsatisfied_GlobalCovers(t *testing.T) { + tmp := t.TempDir() + // No .env files at all. + + workspaces := []OrgWorkspace{ + {Name: "ws-c", RequiredEnv: []EnvRequirement{{Name: "GLOBAL_COVERED"}}}, + } + + globals := map[string]struct{}{"GLOBAL_COVERED": {}} + missing := collectPerWorkspaceUnsatisfied(workspaces, tmp, globals) + + if len(missing) != 0 { + t.Errorf("GLOBAL_COVERED satisfied by global: should be satisfied, got %d missing", len(missing)) + } +} + +// TestCollectPerWorkspaceUnsatisfied_Missing covers the core bug: a +// RequiredEnv declared at the workspace level where the key is absent from +// both global_secrets and the .env file. The import MUST return 412. +func TestCollectPerWorkspaceUnsatisfied_Missing(t *testing.T) { + tmp := t.TempDir() + // No .env files at all. + + workspaces := []OrgWorkspace{ + {Name: "Dev Lead", FilesDir: "dev-lead", RequiredEnv: []EnvRequirement{{Name: "MISSING_REQUIRED_KEY"}}}, + } + + globals := map[string]struct{}{} // no global secret + missing := collectPerWorkspaceUnsatisfied(workspaces, tmp, globals) + + if len(missing) != 1 { + t.Fatalf("expected 1 missing entry, got %d", len(missing)) + } + if missing[0].Workspace != "Dev Lead" { + t.Errorf("expected workspace 'Dev Lead', got %q", missing[0].Workspace) + } + if missing[0].Unsatisfied.Name != "MISSING_REQUIRED_KEY" { + t.Errorf("expected unsatisfied key 'MISSING_REQUIRED_KEY', got %q", missing[0].Unsatisfied.Name) + } + if missing[0].FilesDir != "dev-lead" { + t.Errorf("expected files_dir 'dev-lead', got %q", missing[0].FilesDir) + } +} + +// TestCollectPerWorkspaceUnsatisfied_AnyOfGroup covers an any-of group where +// none of the alternatives are present in global or .env. Should report +// the group as unsatisfied. +func TestCollectPerWorkspaceUnsatisfied_AnyOfGroup(t *testing.T) { + tmp := t.TempDir() + + workspaces := []OrgWorkspace{ + { + Name: "Claude Bot", + FilesDir: "claude-bot", + RequiredEnv: []EnvRequirement{ + {AnyOf: []string{"ANTHROPIC_API_KEY", "CLAUDE_CODE_OAUTH_TOKEN"}}, + }, + }, + } + + globals := map[string]struct{}{} + missing := collectPerWorkspaceUnsatisfied(workspaces, tmp, globals) + + if len(missing) != 1 { + t.Fatalf("expected 1 missing any-of entry, got %d", len(missing)) + } + if missing[0].Workspace != "Claude Bot" { + t.Errorf("expected workspace 'Claude Bot', got %q", missing[0].Workspace) + } + if len(missing[0].Unsatisfied.AnyOf) != 2 { + t.Errorf("expected any-of group with 2 members, got %v", missing[0].Unsatisfied.AnyOf) + } +} + +// TestCollectPerWorkspaceUnsatisfied_NestedChildren covers grandchildren +// workspaces that also declare RequiredEnv. The recursive walk must visit +// children and grandchildren. +func TestCollectPerWorkspaceUnsatisfied_NestedChildren(t *testing.T) { + tmp := t.TempDir() + + workspaces := []OrgWorkspace{ + { + Name: "Root", + Children: []OrgWorkspace{ + { + Name: "Child", + Children: []OrgWorkspace{ + {Name: "Grandchild", FilesDir: "grandchild", RequiredEnv: []EnvRequirement{{Name: "DEEP_KEY"}}}, + }, + }, + }, + }, + } + + globals := map[string]struct{}{} + missing := collectPerWorkspaceUnsatisfied(workspaces, tmp, globals) + + if len(missing) != 1 { + t.Fatalf("expected 1 missing entry from grandchild, got %d", len(missing)) + } + if missing[0].Workspace != "Grandchild" { + t.Errorf("expected 'Grandchild', got %q", missing[0].Workspace) + } +} + +// TestCollectPerWorkspaceUnsatisfied_EmptyOrgBaseDir covers the case where +// orgBaseDir is empty (inline template import). No .env files can be +// checked, so missing keys cannot be attributed to .env absence. The +// function should NOT crash and should only report entries satisfiable +// by global (all missing since globals is empty). +func TestCollectPerWorkspaceUnsatisfied_EmptyOrgBaseDir(t *testing.T) { + workspaces := []OrgWorkspace{ + {Name: "ws-x", RequiredEnv: []EnvRequirement{{Name: "KEY_X"}}}, + } + + globals := map[string]struct{}{} + missing := collectPerWorkspaceUnsatisfied(workspaces, "", globals) + + // With no orgBaseDir and no global, KEY_X must be reported missing. + if len(missing) != 1 { + t.Errorf("expected 1 missing with empty orgBaseDir, got %d", len(missing)) + } +} + +// TestCollectPerWorkspaceUnsatisfied_MultipleWorkspaces reports only the +// workspace whose RequiredEnv is unsatisfied, not the whole batch. +func TestCollectPerWorkspaceUnsatisfied_MultipleWorkspaces(t *testing.T) { + tmp := t.TempDir() + writeEnvFile(t, tmp, "ws-ok/.env", "OK_KEY=val") + + workspaces := []OrgWorkspace{ + {Name: "ws-ok", FilesDir: "ws-ok", RequiredEnv: []EnvRequirement{{Name: "OK_KEY"}}}, + {Name: "ws-missing", FilesDir: "ws-missing", RequiredEnv: []EnvRequirement{{Name: "BAD_KEY"}}}, + } + + globals := map[string]struct{}{} + missing := collectPerWorkspaceUnsatisfied(workspaces, tmp, globals) + + if len(missing) != 1 { + t.Errorf("expected exactly 1 missing (BAD_KEY), got %d", len(missing)) + } + if missing[0].Workspace != "ws-missing" { + t.Errorf("expected missing workspace 'ws-missing', got %q", missing[0].Workspace) + } +} + +// writeEnvFile is a test helper that creates a .env file at the given path +// with the given content. +func writeEnvFile(t *testing.T, baseDir, relPath, content string) { + t.Helper() + fullPath := filepath.Join(baseDir, relPath) + if err := os.MkdirAll(filepath.Dir(fullPath), 0755); err != nil { + t.Fatalf("mkdirAll: %v", err) + } + if err := os.WriteFile(fullPath, []byte(content), 0644); err != nil { + t.Fatalf("writeFile %s: %v", fullPath, err) + } +} -- 2.45.2