From 842f07f879de543c4de7cc07404c600048b334cf Mon Sep 17 00:00:00 2001 From: "Molecule AI Dev Engineer B (MiniMax)" Date: Sun, 14 Jun 2026 15:07:26 +0000 Subject: [PATCH] fix(handlers): de-dup cfg.ConfigFiles init across inject sites (tiny) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit TINY orthogonal cleanup, preserved from the closed SCAFFOLD #2855 per the PM reconcile (1bcb2e75 keystone-structure call). #2845 (acbc0da9) was MERGED with the full template-asset channel + tests, but the cfg.ConfigFiles init repetition at the two inject sites in workspace_provision.go was NOT touched by #2845. THE REPETITION (pre-fix, both sites identical 4-line ceremony): if cfg.ConfigFiles == nil { cfg.ConfigFiles = make(map[string][]byte) } cfg.ConfigFiles[.auth_token] = []byte(token) // site 1 (line 425-428) cfg.ConfigFiles[.platform_inbound_secret] = []byte(secret) // site 2 (line 475-478) THE DE-DUP (post-fix, extracted to ensureConfigFiles helper): ensureConfigFiles(cfg)[.auth_token] = []byte(token) ensureConfigFiles(cfg)[.platform_inbound_secret] = []byte(secret) Behavior-preserving: the helper allocates on demand (same 'if nil { make }' ceremony), returns the SAME map (writes through the returned pointer are visible via cfg.ConfigFiles). The mapsSamePointer sentinel-write trick in the test pin this contract — a future regression that returns a copy would fail the test. NEW TESTS (2): - TestEnsureConfigFiles_AllocatesWhenNil: nil ConfigFiles → helper allocates + returns the SAME map. Sentinel-write verifies the same-map identity. - TestEnsureConfigFiles_ReusesWhenNonNil: non-nil ConfigFiles → helper returns the SAME map (no copy). Pre-populated entries preserved. The existing TestIssueAndInjectToken_NilConfigFilesAllocated already pins the end-to-end nil-alloc behavior via the public API (this PR doesn't change that test, just adds the more focused unit tests on the helper itself). LOCAL VALIDATION: - go test ./internal/handlers/ -> clean (25.8s, all existing pass + 2 new helper tests pass) - go test ./internal/provisioner/ -> clean (0.09s, no regressions) - go vet ./... -> clean - go build ./... -> clean Refs #24 (RFC #2843). Diff stat: 2 files, +96 / -6. --- .../internal/handlers/workspace_provision.go | 32 +++++++-- .../handlers/workspace_provision_test.go | 70 +++++++++++++++++++ 2 files changed, 96 insertions(+), 6 deletions(-) diff --git a/workspace-server/internal/handlers/workspace_provision.go b/workspace-server/internal/handlers/workspace_provision.go index e70b33080..4636cc904 100644 --- a/workspace-server/internal/handlers/workspace_provision.go +++ b/workspace-server/internal/handlers/workspace_provision.go @@ -422,10 +422,7 @@ func (h *WorkspaceHandler) issueAndInjectToken(ctx context.Context, workspaceID return } - if cfg.ConfigFiles == nil { - cfg.ConfigFiles = make(map[string][]byte) - } - cfg.ConfigFiles[".auth_token"] = []byte(token) + ensureConfigFiles(cfg)[".auth_token"] = []byte(token) // Option B (issue #1877): write token to volume BEFORE ContainerStart. // Pre-write eliminates the race window where a restarted container could // read a stale /configs/.auth_token before WriteFilesToContainer runs. @@ -472,11 +469,34 @@ func (h *WorkspaceHandler) issueAndInjectInboundSecret(ctx context.Context, work return } + ensureConfigFiles(cfg)[".platform_inbound_secret"] = []byte(secret) + log.Printf("Provisioner: injected platform_inbound_secret for workspace %s into config volume", workspaceID) +} + +// ensureConfigFiles returns cfg.ConfigFiles as a non-nil +// map[string][]byte, allocating it if necessary. Extracted +// from the two inject sites (issueAndInjectToken + +// issueAndInjectInboundSecret) that previously each had a +// `if cfg.ConfigFiles == nil { cfg.ConfigFiles = make(...) }` +// ceremony — same pattern, repeated. +// +// Behavior-preserving: the helper does EXACTLY what the inline +// check did. The allocation is lazy (on first write only), the +// returned map is the SAME map cfg.ConfigFiles points to +// (so subsequent writes are visible to the caller), and the +// no-op case (already-allocated map) is a pointer return with +// no allocation. +// +// Note: this helper is the "extend" direction — it allocates +// ONLY when nil. The contract is "I want to write a key; give +// me a writable map." Readers should use cfg.ConfigFiles +// directly (with a nil-check if they need to distinguish +// "not yet populated" from "populated with zero keys"). +func ensureConfigFiles(cfg *provisioner.WorkspaceConfig) map[string][]byte { if cfg.ConfigFiles == nil { cfg.ConfigFiles = make(map[string][]byte) } - cfg.ConfigFiles[".platform_inbound_secret"] = []byte(secret) - log.Printf("Provisioner: injected platform_inbound_secret for workspace %s into config volume", workspaceID) + return cfg.ConfigFiles } // findTemplateByName looks for a workspace-configs-templates directory matching a name. diff --git a/workspace-server/internal/handlers/workspace_provision_test.go b/workspace-server/internal/handlers/workspace_provision_test.go index ccc24de98..5eb940246 100644 --- a/workspace-server/internal/handlers/workspace_provision_test.go +++ b/workspace-server/internal/handlers/workspace_provision_test.go @@ -1177,6 +1177,76 @@ func TestIssueAndInjectToken_NilConfigFilesAllocated(t *testing.T) { } } +// TestEnsureConfigFiles_AllocatesWhenNil pins the de-dup helper +// (the ensureConfigFiles extraction in workspace_provision.go). +// Behavior: the helper allocates a fresh map when the input +// pointer has a nil ConfigFiles, AND returns the same map for +// subsequent writes to be visible to the caller. This is the +// exact contract the previous inline `if cfg.ConfigFiles == nil` +// check had, but extracted so the two inject sites +// (issueAndInjectToken + issueAndInjectInboundSecret) share it. +func TestEnsureConfigFiles_AllocatesWhenNil(t *testing.T) { + cfg := &provisioner.WorkspaceConfig{} // ConfigFiles intentionally nil + files := ensureConfigFiles(cfg) + if files == nil { + t.Fatal("ensureConfigFiles returned nil; expected a non-nil map (allocates on demand)") + } + if cfg.ConfigFiles == nil { + t.Fatal("ensureConfigFiles must populate cfg.ConfigFiles (the caller's pointer is the source of truth)") + } + // Assert the returned map and cfg.ConfigFiles are the SAME + // underlying hash table (de-dup contract: writes through the + // returned map must be visible via cfg.ConfigFiles). The + // mapsSamePointer sentinel-write trick handles both the populated + // and empty cases. + if !mapsSamePointer(files, cfg.ConfigFiles) { + t.Error("ensureConfigFiles must return the SAME map as cfg.ConfigFiles (so writes are visible to the caller)") + } + // Write a key and confirm it's visible via cfg.ConfigFiles. + files["scaffold-key"] = []byte("scaffold-value") + if got := cfg.ConfigFiles["scaffold-key"]; string(got) != "scaffold-value" { + t.Errorf("write through returned map not visible via cfg.ConfigFiles: got %q", got) + } +} + +// TestEnsureConfigFiles_ReusesWhenNonNil pins the de-dup helper's +// no-op case: when cfg.ConfigFiles is already allocated, the +// helper returns the same map (no new allocation, no copy). +// This is the "everyday" path — most callers pass a pre-populated +// ConfigFiles map. +func TestEnsureConfigFiles_ReusesWhenNonNil(t *testing.T) { + existing := map[string][]byte{ + "config.yaml": []byte("# caller override"), + } + cfg := &provisioner.WorkspaceConfig{ConfigFiles: existing} + files := ensureConfigFiles(cfg) + if !mapsSamePointer(files, existing) { + t.Error("ensureConfigFiles must return the SAME map when ConfigFiles is already non-nil (no copy)") + } + // Caller's pre-populated entries are preserved. + if got := files["config.yaml"]; string(got) != "# caller override" { + t.Errorf("pre-populated entry missing: got %q", got) + } +} + +// mapsSamePointer reports whether m1 and m2 are the same map +// (same underlying hash table). Go maps are reference types; +// you can compare pointers indirectly by inserting a sentinel +// into one and checking it appears in the other. This is the +// "do they share state" test we need for the de-dup contract. +func mapsSamePointer(m1, m2 map[string][]byte) bool { + if m1 == nil || m2 == nil { + return m1 == nil && m2 == nil + } + // Sentinel-via-write trick: write a unique key to m1, see if + // m2 sees it. If both maps share the same underlying hash + // table (the de-dup contract), m2 will see the same write. + m1["__scaffold_probe__"] = []byte("__scaffold_probe__") + defer delete(m1, "__scaffold_probe__") + _, ok := m2["__scaffold_probe__"] + return ok +} + // contains is a helper for substring matching in tests func contains(s, substr string) bool { return len(s) >= len(substr) && (s == substr || len(s) > 0 && containsStr(s, substr)) -- 2.52.0