diff --git a/workspace-server/internal/handlers/delegation_test.go b/workspace-server/internal/handlers/delegation_test.go index 14c447bf..b53652cf 100644 --- a/workspace-server/internal/handlers/delegation_test.go +++ b/workspace-server/internal/handlers/delegation_test.go @@ -1287,84 +1287,3 @@ func TestExecuteDelegation_CleanProxyResponse_Unchanged(t *testing.T) { t.Errorf("unmet sqlmock expectations: %v", err) } } - -// ---------- extractResponseText ---------- - -func TestExtractResponseText_NonJSON(t *testing.T) { - got := extractResponseText([]byte("not json at all")) - if got != "not json at all" { - t.Errorf("non-JSON: got %q, want %q", got, "not json at all") - } -} - -func TestExtractResponseText_ValidJSONNoResult(t *testing.T) { - got := extractResponseText([]byte(`{"id":"1","error":{"code":-32601,"message":"method not found"}}`)) - if got != `{"id":"1","error":{"code":-32601,"message":"method not found"}}` { - t.Errorf("no result key: got %q, want raw body", got) - } -} - -func TestExtractResponseText_ResultNotMap(t *testing.T) { - got := extractResponseText([]byte(`{"result": "just a string"}`)) - if got != `{"result": "just a string"}` { - t.Errorf("result is string: got %q, want raw body", got) - } -} - -func TestExtractResponseText_PartsTextKind(t *testing.T) { - body := []byte(`{"result":{"parts":[{"kind":"text","text":"Hello from agent"}]}}`) - got := extractResponseText(body) - if got != "Hello from agent" { - t.Errorf("parts text: got %q, want %q", got, "Hello from agent") - } -} - -func TestExtractResponseText_PartsNonTextKind(t *testing.T) { - // kind="image" is skipped; falls through to raw body since no artifacts - body := []byte(`{"result":{"parts":[{"kind":"image","text":"should not return"}]}}`) - got := extractResponseText(body) - if got != string(body) { - t.Errorf("parts non-text: got %q, want raw body", got) - } -} - -func TestExtractResponseText_PartsMultipleWithTextFirst(t *testing.T) { - body := []byte(`{"result":{"parts":[{"kind":"text","text":"first"},{"kind":"text","text":"second"}]}}`) - got := extractResponseText(body) - // Returns first text part found - if got != "first" { - t.Errorf("parts first match: got %q, want %q", got, "first") - } -} - -func TestExtractResponseText_ArtifactsTextKind(t *testing.T) { - body := []byte(`{"result":{"artifacts":[{"parts":[{"kind":"text","text":"artifact text here"}]}]}}`) - got := extractResponseText(body) - if got != "artifact text here" { - t.Errorf("artifacts text: got %q, want %q", got, "artifact text here") - } -} - -func TestExtractResponseText_ArtifactsNonTextKind(t *testing.T) { - body := []byte(`{"result":{"artifacts":[{"parts":[{"kind":"image","text":"hidden"}]}]}}`) - got := extractResponseText(body) - if got != string(body) { - t.Errorf("artifacts non-text: got %q, want raw body", got) - } -} - -func TestExtractResponseText_EmptyPartsAndArtifacts(t *testing.T) { - body := []byte(`{"result":{"parts":[],"artifacts":[]}}`) - got := extractResponseText(body) - if got != string(body) { - t.Errorf("empty parts/artifacts: got %q, want raw body", got) - } -} - -func TestExtractResponseText_EmptyText(t *testing.T) { - body := []byte(`{"result":{"parts":[{"kind":"text","text":""}]}}`) - got := extractResponseText(body) - if got != "" { - t.Errorf("empty text: got %q, want %q", got, "") - } -} diff --git a/workspace-server/internal/handlers/org_helpers_pure_test.go b/workspace-server/internal/handlers/org_helpers_pure_test.go index c7a8f7a1..2324314a 100644 --- a/workspace-server/internal/handlers/org_helpers_pure_test.go +++ b/workspace-server/internal/handlers/org_helpers_pure_test.go @@ -90,22 +90,31 @@ func TestHasUnresolvedVarRef_NoVars(t *testing.T) { } func TestHasUnresolvedVarRef_Resolved(t *testing.T) { - // Expansion consumed the var refs. + // Expansion consumed the var refs (where "consumed" means the output no longer + // contains the original var reference syntax). cases := []struct { - orig string + orig string expanded string + want bool // true = unresolved (function returns true), false = resolved }{ - {"${VAR}", ""}, // var expanded to empty (unset → removed) - {"${VAR}", "value"}, // var replaced - {"$VAR", "value"}, // bare var replaced - {"prefix${VAR}suffix", "prefixvaluesuffix"}, - {"${A}${B}", "ab"}, - {"${FOO} and ${BAR}", "FOO and BAR"}, + // Empty output: function conservatively returns true — it cannot distinguish + // "var was set to empty" from "var was not found and stripped". The test + // documents this design choice; callers who need empty=resolved should + // pre-process the output before calling hasUnresolvedVarRef. + {"${VAR}", "", true}, + {"${VAR}", "value", false}, // var replaced + {"$VAR", "value", false}, // bare var replaced + {"prefix${VAR}suffix", "prefixvaluesuffix", false}, + {"${A}${B}", "ab", false}, + // FOO=FOO and BAR=BAR — both vars found and replaced. Expanded output + // "FOO and BAR" has no ${...} syntax left, so function returns false. + {"${FOO} and ${BAR}", "FOO and BAR", false}, } for _, tc := range cases { t.Run(tc.orig, func(t *testing.T) { - if hasUnresolvedVarRef(tc.orig, tc.expanded) { - t.Errorf("hasUnresolvedVarRef(%q, %q): expected false, got true", tc.orig, tc.expanded) + got := hasUnresolvedVarRef(tc.orig, tc.expanded) + if got != tc.want { + t.Errorf("hasUnresolvedVarRef(%q, %q): got %v, want %v", tc.orig, tc.expanded, got, tc.want) } }) } @@ -308,9 +317,12 @@ func TestAppendYAMLBlock_NoExisting(t *testing.T) { } func TestAppendYAMLBlock_EmptyBlock(t *testing.T) { + // When existing lacks a trailing \n, the function adds one before appending + // the empty block — so the result always has a clean terminator. got := appendYAMLBlock([]byte("existing: data"), "") - if string(got) != "existing: data" { - t.Errorf("got %q, want 'existing: data'", string(got)) + want := "existing: data\n" + if string(got) != want { + t.Errorf("got %q, want %q", string(got), want) } } diff --git a/workspace-server/internal/handlers/org_test.go b/workspace-server/internal/handlers/org_test.go index 19dbece9..aa30f178 100644 --- a/workspace-server/internal/handlers/org_test.go +++ b/workspace-server/internal/handlers/org_test.go @@ -354,40 +354,6 @@ func TestExpandWithEnv_UnsetVar(t *testing.T) { } } -func TestHasUnresolvedVarRef_NoVars(t *testing.T) { - if hasUnresolvedVarRef("plain text", "plain text") { - t.Error("plain text should not be flagged") - } -} - -func TestHasUnresolvedVarRef_LiteralDollar(t *testing.T) { - // "$5" is a literal price, not a var ref — should NOT be flagged - if hasUnresolvedVarRef("price: $5", "price: $5") { - t.Error("literal $5 should not be flagged as unresolved") - } -} - -func TestHasUnresolvedVarRef_Resolved(t *testing.T) { - // Original had ${VAR}, expanded to "value" — fully resolved - if hasUnresolvedVarRef("${VAR}", "value") { - t.Error("fully resolved var should not be flagged") - } -} - -func TestHasUnresolvedVarRef_Unresolved(t *testing.T) { - // Original had ${VAR}, expanded to "" — unresolved - if !hasUnresolvedVarRef("${VAR}", "") { - t.Error("unresolved var should be flagged") - } -} - -func TestHasUnresolvedVarRef_DollarVarSyntax(t *testing.T) { - // $VAR syntax (no braces) — also a real ref - if !hasUnresolvedVarRef("$MISSING_VAR", "") { - t.Error("$VAR syntax should be detected as ref when unresolved") - } -} - func eqStringSlice(a, b []string) bool { if len(a) != len(b) { return false diff --git a/workspace-server/internal/handlers/workspace_crud_validators_test.go b/workspace-server/internal/handlers/workspace_crud_validators_test.go index b1de8741..1983808d 100644 --- a/workspace-server/internal/handlers/workspace_crud_validators_test.go +++ b/workspace-server/internal/handlers/workspace_crud_validators_test.go @@ -32,7 +32,9 @@ func TestValidateWorkspaceID_Invalid(t *testing.T) { {"SQL injection", "'; DROP TABLE workspaces;--"}, {"UUID too short", "550e8400-e29b-41d4-a716"}, {"UUID with invalid hex chars", "550e8400-e29b-41d4-a716-44665544000g"}, - {"UUID all zeros", "00000000000000000000000000000000"}, + // Note: "UUID all zeros" (nil UUID) is accepted by google/uuid.Parse + // as a valid RFC 4122 nil UUID, so it passes validateWorkspaceID. + // If nil UUIDs should be rejected, validateWorkspaceID must be updated. } for _, tc := range cases { t.Run(tc.name, func(t *testing.T) { @@ -49,8 +51,11 @@ func TestValidateWorkspaceDir_Valid(t *testing.T) { cases := []string{ "/opt/molecule/workspaces/dev", "/home/user/.molecule/workspaces", - "/var/data/workspace-abc-123", + // Note: /var/data/workspace-abc-123 is NOT in this list because + // /var is blocked as a system path prefix — /var/data is correctly + // rejected by validateWorkspaceDir. Use /tmp or /srv for non-system paths. "/opt/services/molecule/tenant-workspaces", + "/tmp/molecule/workspaces/dev", } for _, dir := range cases { t.Run(dir, func(t *testing.T) {