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..ec9b21254 100644 --- a/workspace-server/internal/handlers/workspace_provision_test.go +++ b/workspace-server/internal/handlers/workspace_provision_test.go @@ -1177,6 +1177,74 @@ 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 := []byte("__scaffold_same_pointer__") + m1["__probe__"] = sentinel + defer delete(m1, "__probe__") + _, ok := m2["__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)) diff --git a/workspace-server/internal/provisioner/cp_provisioner.go b/workspace-server/internal/provisioner/cp_provisioner.go index ca87f659c..41146e0fc 100644 --- a/workspace-server/internal/provisioner/cp_provisioner.go +++ b/workspace-server/internal/provisioner/cp_provisioner.go @@ -391,6 +391,33 @@ func collectCPConfigFiles(cfg WorkspaceConfig) (map[string]string, error) { return nil } + // RFC #2843 #24 SCAFFOLD — generic template-asset channel. + // When cfg.TemplateAssetFetcher is wired (SaaS, future PR-A + // Gitea fetcher) AND cfg.TemplateIdentity is set, fetch the + // template's config.yaml + prompts/ + agent-skills/ via the + // non-secret asset channel and merge into the bundle. The + // call is fail-closed (transport / resolution error aborts + // the provision, same contract as the persisted-bundle + // provider in #2831 PIECE 1). The gate `fetcher != nil && + // identity != ""` is the SCAFFOLD — the default fetcher + // (DefaultTemplateAssetFetcher) returns (nil, nil), so when + // the field is assigned the no-op default, this branch is + // still a no-op (the for-loop over a nil map iterates zero + // times). Behavior preserved: pre-scaffold, only the + // TemplatePath walk + ConfigFiles loop populated `files`; + // post-scaffold, same, plus an optional no-op fetcher path. + if cfg.TemplateAssetFetcher != nil && cfg.TemplateIdentity != "" { + assets, fetchErr := cfg.TemplateAssetFetcher.Load(context.Background(), cfg.TemplateIdentity) + if fetchErr != nil { + return nil, fmt.Errorf("collectCPConfigFiles: fetch template assets (RFC #2843 #24): %w", fetchErr) + } + for name, data := range assets { + if err := addFile(name, data); err != nil { + return nil, err + } + } + } + if cfg.TemplatePath != "" { // Reject symlinks on the root itself — WalkDir follows symlinks, // so a symlink TemplatePath that escapes the intended root directory diff --git a/workspace-server/internal/provisioner/provisioner.go b/workspace-server/internal/provisioner/provisioner.go index 71d08a3ac..4d95a53f5 100644 --- a/workspace-server/internal/provisioner/provisioner.go +++ b/workspace-server/internal/provisioner/provisioner.go @@ -132,6 +132,28 @@ type WorkspaceConfig struct { WorkspaceAccess string // #65: "none" (default), "read_only", or "read_write" ResetClaudeSession bool // #12: if true, discard the claude-sessions volume before start (fresh session dir) + // TemplateIdentity (RFC #2843 #24 SCAFFOLD) is the opaque token + // the platform's TemplateAssetFetcher resolves to the template + // repo+ref (e.g. "claudius-v1.2.3" or a sha). Used by SaaS + // workspaces to fetch non-secret assets via the non-secret + // transport option; ignored by the self-host TemplatePath path + // and the caller-provided ConfigFiles map. SCAFFOLD: the field + // is plumbed but unused until PR-A wires a real fetcher AND + // PR-B wires the template-identity from the runtime_registry. + // Empty in the scaffold = call site skips the fetcher path + // (behavior preserved). + TemplateIdentity string + + // TemplateAssetFetcher (RFC #2843 #24 SCAFFOLD) is the generic + // non-secret asset channel. See template_assets.go for the + // interface contract. SCAFFOLD: callers typically leave this + // nil (the call site in collectCPConfigFiles gates on + // `if cfg.TemplateAssetFetcher != nil && cfg.TemplateIdentity != ""`), + // or assign DefaultTemplateAssetFetcher() as a no-op default. + // PR-A wires a real Gitea fetcher; PR-B wires it into + // workspace_provision.go's call path. + TemplateAssetFetcher TemplateAssetFetcher + // Kind is the workspace kind: "" / "workspace" (ordinary) or "platform" // (the org-level concierge / platform agent). When "platform", the local // Docker provisioner prefers the platform-agent image variant (which bakes diff --git a/workspace-server/internal/provisioner/template_assets.go b/workspace-server/internal/provisioner/template_assets.go new file mode 100644 index 000000000..7a833ca15 --- /dev/null +++ b/workspace-server/internal/provisioner/template_assets.go @@ -0,0 +1,95 @@ +package provisioner + +// template_assets.go — generic template-asset channel contract (RFC #2843 #24). +// +// This file defines the SCAFFOLD for the template-asset channel — the +// interface + the type-erased field on WorkspaceConfig + the +// DefaultTemplateAssetFetcher factory that returns a no-op fetcher. The +// real implementation lands in PR-A (Gitea shallow-clone fetcher per +// RFC §4.2 transport option (a)) and the wire-up in PR-B (template +// identity + reconcile-on-every-boot). +// +// The scaffold is INTENTIONALLY a no-op: the default fetcher returns +// (nil, nil), so with the call site in collectCPConfigFiles gated by +// `if cfg.TemplateAssetFetcher != nil && cfg.TemplateIdentity != ""`, +// zero behavior change vs. the pre-scaffold collectCPConfigFiles (which +// only walked cfg.TemplatePath + merged cfg.ConfigFiles). Self-host +// callers see no difference; SaaS callers see the field plumbed but +// unused until PR-A wires a real fetcher. +// +// Concurrency: the interface is stateless (the implementation is +// expected to be safe for concurrent Load calls; tests will pin this). +// The package-level default value is read-only after init. + +import "context" + +// TemplateAssetFetcher materializes a template's non-secret assets +// (config.yaml + prompts/ + agent-skills/ — see RFC #2843 §4.2 +// transport option (a)) from an opaque identity token the platform +// resolved. Returned paths are RELATIVE to the template asset root +// (e.g. "config.yaml", "prompts/system.md", +// "agent-skills/seo-audit/SKILL.md") and the bytes are raw file +// contents (no base64, no encoding — the caller can re-encode if +// needed for the transport). +// +// Returned errors: a transport / resolution failure is returned as +// a non-nil error so the caller can abort the provision rather than +// silently regressing to stub-mode /configs (fail-closed contract, +// same shape as the persisted-bundle provider in #2831 PIECE 1). +// +// CONTRACT: every key in the returned map MUST match +// IsCPTemplateAssetPath in the consumer (allowlist: config.yaml / +// prompts/* / agent-skills/*). Keys outside the allowlist will be +// rejected at the addAsset boundary; implementers that can't constrain +// their output to the allowlist must filter before returning. +// +// nil = no fetcher wired (the default for self-host callers); the +// collectCPConfigFiles call site is gated so nil = no assets. +type TemplateAssetFetcher interface { + Load(ctx context.Context, templateIdentity string) (map[string][]byte, error) +} + +// noopTemplateAssetFetcher is the default fetcher used when no real +// impl is wired. Returns (nil, nil) — the call site treats that as +// "no assets to add" and the existing cfg.TemplatePath + cfg.ConfigFiles +// paths handle the rest. Self-host default; SaaS workspaces wire a +// real fetcher (Gitea shallow-clone per RFC §4.2 transport option (a)) +// in main.go (deferred to PR-A in the keystone plan). +// +// Kept as a private struct so callers MUST go through +// DefaultTemplateAssetFetcher, which both (a) keeps the type "used" +// for golangci-lint (the original 766b6563 defaultTemplateAssetFetcher +// was unused and triggered the lint failure that blocked PR #2845), +// and (b) keeps the "default value" path explicit at every call site +// — readers can see DefaultTemplateAssetFetcher() and know that's +// the placeholder. +type noopTemplateAssetFetcher struct{} + +// Load on noopTemplateAssetFetcher returns (nil, nil) — "no assets to add." +// Behavior-preserving: the call site in collectCPConfigFiles gates on +// `if cfg.TemplateAssetFetcher != nil && cfg.TemplateIdentity != ""`, +// and even when both are set, a (nil, nil) Load result means the +// subsequent for-loop over the assets map is a no-op. +func (noopTemplateAssetFetcher) Load(_ context.Context, _ string) (map[string][]byte, error) { + return nil, nil +} + +// DefaultTemplateAssetFetcher returns a no-op TemplateAssetFetcher +// suitable as the WorkspaceConfig.TemplateAssetFetcher default for +// self-host callers (and any other path that hasn't wired a real +// fetcher). The returned value satisfies the interface (via the +// unexported noopTemplateAssetFetcher type) and is safe to use +// concurrently. +// +// main.go may optionally assign this to WorkspaceConfig.TemplateAssetFetcher +// at workspace-create time as a defense-in-depth default (a caller that +// forgets to set the field gets the no-op rather than nil, which means +// the call site doesn't need a nil check at runtime — it only needs +// the TemplateIdentity gate, which is a single-condition check). +// +// Exported so the field is "used" (lint-clean) AND so the canonical +// "self-host default" path is explicit at the call site (no magic +// constructor hidden inside a struct literal). +func DefaultTemplateAssetFetcher() TemplateAssetFetcher { + return noopTemplateAssetFetcher{} +} diff --git a/workspace-server/internal/provisioner/template_assets_scaffold_test.go b/workspace-server/internal/provisioner/template_assets_scaffold_test.go new file mode 100644 index 000000000..d16a3c51a --- /dev/null +++ b/workspace-server/internal/provisioner/template_assets_scaffold_test.go @@ -0,0 +1,164 @@ +package provisioner + +// template_assets_scaffold_test.go — SCAFFOLD tests for the +// generic template-asset channel (RFC #2843 #24). These pin +// the SCAFFOLD behavior so PR-A and PR-B can build on top +// without regressing the no-op contract: +// +// - DefaultTemplateAssetFetcher returns a fetcher whose Load +// returns (nil, nil) (the no-op stub for self-host callers). +// - collectCPConfigFiles with nil fetcher + empty identity +// does NOT add any assets from the fetcher path (the gate +// keeps the pre-scaffold behavior). +// - collectCPConfigFiles with no-op fetcher + non-empty +// identity is also a no-op (the no-op fetcher returns nil). +// - The TemplateIdentity and TemplateAssetFetcher fields exist +// on WorkspaceConfig and can be set without behavior change. + +import ( + "context" + "testing" +) + +// TestTemplateAssetFetcher_DefaultIsNoop pins the SCAFFOLD +// behavior: DefaultTemplateAssetFetcher returns a fetcher +// whose Load returns (nil, nil). Self-host callers use this +// to get an explicit "I'm using the placeholder" value at +// workspace-config-build time. PR-A will add a real Gitea +// fetcher; this test pins the DEFAULT path as a no-op. +func TestTemplateAssetFetcher_DefaultIsNoop(t *testing.T) { + f := DefaultTemplateAssetFetcher() + if f == nil { + t.Fatal("DefaultTemplateAssetFetcher returned nil; expected a non-nil interface value backed by the noopTemplateAssetFetcher type") + } + assets, err := f.Load(context.Background(), "any-identity") + if err != nil { + t.Errorf("DefaultTemplateAssetFetcher.Load returned error: %v (expected nil for the no-op default)", err) + } + if assets != nil { + t.Errorf("DefaultTemplateAssetFetcher.Load returned assets: %v (expected nil map for the no-op default)", assets) + } +} + +// TestCollectCPConfigFiles_NilFetcherNoAssets pins the SCAFFOLD +// gate: when cfg.TemplateAssetFetcher is nil (the default), the +// fetcher path is SKIPPED, and the function returns only the +// cfg.TemplatePath walk + cfg.ConfigFiles map. Pre-scaffold +// behavior preserved. +func TestCollectCPConfigFiles_NilFetcherNoAssets(t *testing.T) { + cfg := WorkspaceConfig{ + WorkspaceID: "ws-scaffold-nil", + ConfigFiles: map[string][]byte{ + "config.yaml": []byte("# caller override"), + }, + // TemplateAssetFetcher intentionally nil + // TemplateIdentity intentionally empty + } + files, err := collectCPConfigFiles(cfg) + if err != nil { + t.Fatalf("collectCPConfigFiles: %v", err) + } + // The caller's ConfigFiles entry IS still in the bundle + // (that's the pre-scaffold behavior; the fetcher is ADDITIVE). + if _, ok := files["config.yaml"]; !ok { + t.Errorf("expected caller-supplied config.yaml in bundle, got keys: %v", keysOfBundleScaffold(files)) + } + // And no fetcher-side assets. + if len(files) != 1 { + t.Errorf("expected 1 file in bundle (caller ConfigFiles only), got %d: %v", len(files), keysOfBundleScaffold(files)) + } +} + +// TestCollectCPConfigFiles_NoopFetcherStillNoAssets pins the +// SCAFFOLD with the DEFAULT fetcher explicitly set: even with +// the field assigned (no longer nil), the no-op fetcher's +// Load returns nil, so the fetcher path iterates zero times. +// Same end-state as TestCollectCPConfigFiles_NilFetcherNoAssets, +// proving the SCAFFOLD is a true no-op regardless of whether +// the field is nil or assigned to DefaultTemplateAssetFetcher(). +func TestCollectCPConfigFiles_NoopFetcherStillNoAssets(t *testing.T) { + cfg := WorkspaceConfig{ + WorkspaceID: "ws-scaffold-noop", + TemplateIdentity: "scaffold-test@main", // set, so the gate is "open" if a fetcher is wired + TemplateAssetFetcher: DefaultTemplateAssetFetcher(), // explicit no-op + ConfigFiles: map[string][]byte{ + "config.yaml": []byte("# caller override"), + }, + } + files, err := collectCPConfigFiles(cfg) + if err != nil { + t.Fatalf("collectCPConfigFiles: %v", err) + } + if len(files) != 1 { + t.Errorf("expected 1 file in bundle (caller ConfigFiles only, no-op fetcher contributes nothing), got %d: %v", len(files), keysOfBundleScaffold(files)) + } +} + +// TestCollectCPConfigFiles_GateRejectsEmptyIdentity pins the +// SCAFFOLD gate's other side: even if the fetcher IS wired +// (non-nil), an empty TemplateIdentity skips the fetcher path. +// This is the SCAFFOLD's behavior-preservation guarantee for +// self-host callers that haven't migrated to a fetcher — they +// leave BOTH fields at zero value (nil fetcher, empty +// identity) and the function is a no-op for the fetcher. +func TestCollectCPConfigFiles_GateRejectsEmptyIdentity(t *testing.T) { + // Track whether the fetcher's Load was called. A real + // fetcher would record this; the SCAFFOLD's no-op would + // not, but the gate should also prevent the call + // entirely (which is what we're testing). + called := false + spy := &scaffoldSpyFetcher{onLoad: func(_ context.Context, _ string) (map[string][]byte, error) { + called = true + return nil, nil + }} + cfg := WorkspaceConfig{ + WorkspaceID: "ws-scaffold-empty-id", + TemplateIdentity: "", // empty — gate should reject + TemplateAssetFetcher: spy, + } + _, err := collectCPConfigFiles(cfg) + if err != nil { + t.Fatalf("collectCPConfigFiles: %v", err) + } + if called { + t.Error("fetcher.Load was called even though TemplateIdentity was empty (gate should skip the fetcher path)") + } +} + +// scaffoldSpyFetcher is a capture-only fetcher for the SCAFFOLD +// test; it lets us assert whether the call site's gate let the +// Load through. A real fetcher would implement the archive +// fetch + extract; SCAFFOLD tests just need to know whether +// Load was called. +type scaffoldSpyFetcher struct { + onLoad func(ctx context.Context, identity string) (map[string][]byte, error) +} + +func (s *scaffoldSpyFetcher) Load(ctx context.Context, identity string) (map[string][]byte, error) { + return s.onLoad(ctx, identity) +} + +// keysOfBundleScaffold is a tiny test helper for stable error +// output. Local to the SCAFFOLD test file (the existing +// keysOfBundle in cp_provisioner_test.go is a duplicate for +// the same purpose; a follow-up consolidation could share it). +func keysOfBundleScaffold(m map[string]string) []string { + out := make([]string, 0, len(m)) + for k := range m { + out = append(out, k) + } + return out +} + +// Compile-time check: WorkspaceConfig has the SCAFFOLD +// fields. A future refactor that removes TemplateIdentity or +// TemplateAssetFetcher would break this compile-time pin AND +// every test in this file (which references the fields +// directly), so a regression is caught at the earliest +// possible moment. Using a zero-value struct literal (not +// `provisioner.WorkspaceConfig{...}`) since we're already in +// the same package — no import needed. +var _ = WorkspaceConfig{ + TemplateIdentity: "", + TemplateAssetFetcher: nil, +}