Merge pull request 'fix(org): add per-workspace RequiredEnv preflight check (#232)' (#527) from pr-251 into main

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
+		// <files_dir>/.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{}{}

workspace-server/internal/handlers/org_external.go

@@ -346,7 +346,7 @@ func (g *gitFetcher) Fetch(ctx context.Context, rootDir, host, repoPath, ref str
 	// MkdirTemp creates the dir; git clone refuses to clone into a
 	// non-empty dir. Remove + recreate empty.
 	os.RemoveAll(tmpDir)
-	cloneAndConfig := append(gitArgs("clone", "--quiet", "--depth=1", "-b", ref, cloneURL, tmpDir))
+	cloneAndConfig := gitArgs("clone", "--quiet", "--depth=1", "-b", ref, cloneURL, tmpDir)
 	cmd := exec.CommandContext(ctx, "git", cloneAndConfig...)
 	cmd.Env = append(os.Environ(), "GIT_TERMINAL_PROMPT=0")
 	if out, err := cmd.CombinedOutput(); err != nil {

workspace-server/internal/handlers/org_import.go

@@ -941,6 +941,65 @@ 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 <files_dir>/.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 <files_dir>/.env (if filesDir is set)
+			//   3. Persona bootstrap env (MOLECULE_PERSONA_ROOT/<filesDir>/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)
+			// Convert map[string]string (from .env files) to map[string]struct{}
+			// to match IsSatisfied's signature.
+			envSet := make(map[string]struct{}, len(envFromFiles))
+			for k := range envFromFiles {
+				envSet[k] = struct{}{}
+			}
+			for _, req := range ws.RequiredEnv {
+				if req.IsSatisfied(globalSecrets) {
+					continue // covered by a global secret
+				}
+				if req.IsSatisfied(envSet) {
+					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`,

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)
+	}
+}

workspace-server/internal/pendinguploads/export_test.go

@@ -12,8 +12,8 @@ import (
 // time. The Go convention `export_test.go` keeps this seam OUT of the
 // production binary — files ending in _test.go are stripped at build
 // time, so this re-export only exists during `go test`.
-func StartSweeperWithIntervalForTest(ctx context.Context, storage Storage, ackRetention, interval time.Duration) {
-	startSweeperWithInterval(ctx, storage, ackRetention, interval, nil)
+func StartSweeperWithIntervalForTest(ctx context.Context, storage Storage, ackRetention, interval time.Duration, done chan struct{}) {
+	startSweeperWithInterval(ctx, storage, ackRetention, interval, done)
 }
 
 // StartSweeperForTest starts the sweeper and returns a done channel

workspace-server/internal/pendinguploads/sweeper_test.go

@@ -190,7 +190,14 @@ func TestStartSweeperWithInterval_TickerFiresAdditionalCycles(t *testing.T) {
 	ctx, cancel := context.WithCancel(context.Background())
 	defer cancel()
 
-	done := pendinguploads.StartSweeperForTest(ctx, store, time.Hour)
+	// Use a short ticker interval (100ms) so the test runs fast without
+	// burning real wall-clock time. StartSweeperWithIntervalForTest is the
+	// test-friendly variant that accepts a caller-specified interval; the
+	// production SweepInterval of 5m is too coarse for a 2s deadline on
+	// a loaded CI runner (the ticker may not fire at all under CPU
+	// contention — the root cause of the pre-existing CI flake).
+	done := make(chan struct{})
+	go pendinguploads.StartSweeperWithIntervalForTest(ctx, store, time.Hour, 100*time.Millisecond, done)
 	// Immediate cycle + at least one tick-driven cycle.
 	store.waitForCycle(t, 2, 2*time.Second)