refactor(handlers): apply simplify findings on PR #2094

- Extract walkTemplateConfigs(configsDir, fn) shared helper. Both
  templates.List and loadRuntimeProvisionTimeouts walked configsDir
  + parsed config.yaml — same boilerplate twice. Now centralised so
  a future template-discovery rule (subdir naming, README sentinel,
  etc.) lands in one place.
- templates.List uses the walker — net -10 lines.
- loadRuntimeProvisionTimeouts uses the walker — net -10 lines.
- Document runtimeProvisionTimeoutsCache as 'NOT SAFE for
  package-level reuse' so a future change doesn't accidentally
  promote it to a singleton (sync.Once can't be reset → tests
  would lock out other fixtures).

Skipped (review finding): atomic.Pointer[map[string]int] for
future hot-reload. The doc comment already documents the
limitation; YAGNI-promoting the primitive now would buy a
not-yet-built feature at the cost of more code today.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

workspace-server/internal/handlers/runtime_provision_timeouts.go

@@ -9,16 +9,21 @@ import (
 	"gopkg.in/yaml.v3"
 )
 
-// runtimeProvisionTimeouts caches the per-runtime provision-timeout values
-// declared in template config.yaml manifests (#2054 phase 2). Lazy-init so
-// the first workspace API request after process start pays the read cost
-// (a few KB of yaml across ~50 templates) and every subsequent one is a
-// map lookup.
+// runtimeProvisionTimeoutsCache holds the per-runtime provision-timeout
+// values declared in template config.yaml manifests (#2054 phase 2).
+// Lazy-init so the first workspace API request after process start pays
+// the read cost (a few KB of yaml across ~50 templates) and every
+// subsequent one is a map lookup.
 //
 // Cache lifetime = process lifetime. Templates only change on container
 // rebuild + workspace-server restart, which already invalidates the
 // in-memory state. A future template-hot-reload feature would need to
 // refresh this cache; today there is no such hook.
+//
+// NOT SAFE for package-level reuse — sync.Once cannot be reset, so a
+// shared singleton would lock tests out of trying multiple template
+// fixtures. Tests construct fresh struct values; production code holds
+// one per WorkspaceHandler instance.
 type runtimeProvisionTimeoutsCache struct {
 	once sync.Once
 	m    map[string]int // runtime → seconds
@@ -31,9 +36,38 @@ func (c *runtimeProvisionTimeoutsCache) get(configsDir string, runtime string) i
 	return c.m[runtime]
 }
 
-// loadRuntimeProvisionTimeouts walks `configsDir`, parses every immediate
-// subdirectory's `config.yaml`, and returns a map of runtime → seconds
-// for templates that declared `runtime_config.provision_timeout_seconds`.
+// walkTemplateConfigs invokes fn(id, configBytes) for every immediate
+// subdirectory of configsDir that has a readable config.yaml. Bad reads
+// and empty dirs are silently skipped — the caller decides whether
+// missing fields warrant the slot. A bad configsDir logs once and
+// returns without invoking fn, matching the "start clean with no
+// templates" semantics.
+//
+// Shared between the templates.List handler (which decodes the full
+// templateSummary) and loadRuntimeProvisionTimeouts (which decodes the
+// narrow runtime-timeout subset). Centralising the walk means a future
+// template-discovery rule (subdir naming convention, README sentinel,
+// etc.) lands in one place.
+func walkTemplateConfigs(configsDir string, fn func(id string, configBytes []byte)) {
+	entries, err := os.ReadDir(configsDir)
+	if err != nil {
+		log.Printf("walkTemplateConfigs: read configsDir %s: %v", configsDir, err)
+		return
+	}
+	for _, e := range entries {
+		if !e.IsDir() {
+			continue
+		}
+		data, err := os.ReadFile(filepath.Join(configsDir, e.Name(), "config.yaml"))
+		if err != nil {
+			continue
+		}
+		fn(e.Name(), data)
+	}
+}
+
+// loadRuntimeProvisionTimeouts returns a map of runtime → seconds for
+// templates that declared `runtime_config.provision_timeout_seconds`.
 //
 // Templates without the field aren't represented (lookup returns zero,
 // which the caller treats as "fall through to canvas runtime profile").
@@ -46,22 +80,7 @@ func (c *runtimeProvisionTimeoutsCache) get(configsDir string, runtime string) i
 // safer default.
 func loadRuntimeProvisionTimeouts(configsDir string) map[string]int {
 	out := map[string]int{}
-	entries, err := os.ReadDir(configsDir)
-	if err != nil {
-		// Logged but not fatal — workspace-server starts cleanly with
-		// no templates (dev / fresh-clone). The result is an empty map
-		// so every runtime falls through to canvas's profile default.
-		log.Printf("loadRuntimeProvisionTimeouts: read configsDir %s: %v", configsDir, err)
-		return out
-	}
-	for _, e := range entries {
-		if !e.IsDir() {
-			continue
-		}
-		data, err := os.ReadFile(filepath.Join(configsDir, e.Name(), "config.yaml"))
-		if err != nil {
-			continue
-		}
+	walkTemplateConfigs(configsDir, func(_ string, data []byte) {
 		var raw struct {
 			Runtime       string `yaml:"runtime"`
 			RuntimeConfig struct {
@@ -69,15 +88,15 @@ func loadRuntimeProvisionTimeouts(configsDir string) map[string]int {
 			} `yaml:"runtime_config"`
 		}
 		if err := yaml.Unmarshal(data, &raw); err != nil {
-			continue
+			return
 		}
 		secs := raw.RuntimeConfig.ProvisionTimeoutSeconds
 		if secs <= 0 || raw.Runtime == "" {
-			continue
+			return
 		}
 		if existing, ok := out[raw.Runtime]; !ok || secs > existing {
 			out[raw.Runtime] = secs
 		}
-	}
+	})
 	return out
 }

workspace-server/internal/handlers/templates.go

@@ -87,24 +87,8 @@ func (h *TemplatesHandler) resolveTemplateDir(wsName string) string {
 
 // List handles GET /templates
 func (h *TemplatesHandler) List(c *gin.Context) {
-	entries, err := os.ReadDir(h.configsDir)
-	if err != nil {
-		c.JSON(http.StatusOK, []templateSummary{})
-		return
-	}
-
 	templates := make([]templateSummary, 0)
-	for _, entry := range entries {
-		if !entry.IsDir() {
-			continue
-		}
-
-		configPath := filepath.Join(h.configsDir, entry.Name(), "config.yaml")
-		data, err := os.ReadFile(configPath)
-		if err != nil {
-			continue
-		}
-
+	walkTemplateConfigs(h.configsDir, func(id string, data []byte) {
 		var raw struct {
 			Name          string   `yaml:"name"`
 			Description   string   `yaml:"description"`
@@ -113,14 +97,14 @@ func (h *TemplatesHandler) List(c *gin.Context) {
 			Model         string   `yaml:"model"`
 			Skills        []string `yaml:"skills"`
 			RuntimeConfig struct {
-				Model                  string      `yaml:"model"`
-				Models                 []modelSpec `yaml:"models"`
-				RequiredEnv            []string    `yaml:"required_env"`
+				Model                   string      `yaml:"model"`
+				Models                  []modelSpec `yaml:"models"`
+				RequiredEnv             []string    `yaml:"required_env"`
 				ProvisionTimeoutSeconds int         `yaml:"provision_timeout_seconds"`
 			} `yaml:"runtime_config"`
 		}
 		if err := yaml.Unmarshal(data, &raw); err != nil {
-			continue
+			return
 		}
 
 		// Model comes from either top-level (legacy) or runtime_config.model (current).
@@ -130,7 +114,7 @@ func (h *TemplatesHandler) List(c *gin.Context) {
 		}
 
 		templates = append(templates, templateSummary{
-			ID:                      entry.Name(),
+			ID:                      id,
 			Name:                    raw.Name,
 			Description:             raw.Description,
 			Tier:                    raw.Tier,
@@ -142,7 +126,7 @@ func (h *TemplatesHandler) List(c *gin.Context) {
 			SkillCount:              len(raw.Skills),
 			ProvisionTimeoutSeconds: raw.RuntimeConfig.ProvisionTimeoutSeconds,
 		})
-	}
+	})
 
 	c.JSON(http.StatusOK, templates)
 }