feat: add template builder module catalog structure and go:embed wiring by jeremyruppel · Pull Request #25909 · coder/coder

jeremyruppel · 2026-06-01T14:18:13Z

Scaffolds the coderd/templatebuilder package for the guided template builder (DEVEX-272, RFC).

Adds the module catalog types and go:embed wiring that the template builder endpoints will use:

codersdk.TemplateBuilderModule, TemplateBuilderModuleVariable, and related types matching the RFC schema
Internal ModuleManifest type with go:embed wiring to bundle module.json files from coderd/templatebuilder/modules/
LoadModules() with defensive copy, unexported parseModulesFromFS(fs.FS) for test isolation, ToSDK() conversion
Real code-server module manifest as the first catalog entry
Strict validation: ID uniqueness, version non-empty, variable type/name validation, DisallowUnknownFields, and requiring module.json in every module directory
Tests via internal catalog_internal_test.go (for parseModulesFromFS with fstest.MapFS fixtures) and external catalog_test.go (for LoadModules and ToSDK), covering multi-module parsing, all variable types, validation errors, nil-slice normalization, and full SDK field assertions

Note

Generated with Coder Agents by @jeremyruppel

jeremyruppel · 2026-06-01T19:26:38Z

/coder-agents-review

coder-agents-review

Clean scaffolding with good structural decisions: the two-type-layer design (internal manifest vs. SDK response) is well-considered, the ToSDK() conversion is explicit, and the README documents the design rationale thoroughly. Tests are parallel and use subtests properly. The go:embed wiring is correct.

4 P2, 7 P3, 3 Nit, 2 Note. The P2s cluster around two themes: the cached slice is mutable (CRF-2), and the parse/validation pipeline trusts its input too much (CRF-3, CRF-4, CRF-5). Most findings resolve with a validation pass in parseModules() and a defensive copy in LoadModules().

Ryosuke put it well: "parseModules() should validate and LoadModules() should return immutable data. If the parse step rejects malformed manifests and the return path clones or pre-converts the slice, the mutable-cache, ghost-entry, duplicate-ID, and invalid-type findings all disappear."

PS. Gon audited all 10 comments in the PR and found 8 of them restate the type name without adding information. The godoc convention for exported types is worth following, but consider trimming the ones that decompose the identifier into words (e.g., "TemplateBuilderVariableType represents the type of a template builder variable").

🤖 This review was automatically generated with Coder Agents.

coder-agents-review · 2026-06-01T19:37:34Z

+  these; they become bare Terraform `variable` blocks so values are supplied
+  at workspace creation time.
+- **`pinned_version`**: The exact module version shipped with this Coder
+  release. The compose endpoint always emits this version; `latest` is never


Note [CRF-16] Statements like "The compose endpoint always emits this version", "Matched against the base template's OS to filter incompatible modules", and "The UI surfaces a warning but does not block selection" describe behavior that doesn't exist yet. Reasonable for a scaffolding design doc, but someone reading this README cold will think these features work now. (Leorio)

🤖

Fixed. README now uses future tense ("will inject", "will be matched", etc.) for behavior that depends on unimplemented endpoints.

this does not bode well for the README idea

coder-agents-review

Round 2 is blocked. No new commits since R1 (head SHA unchanged) and no author responses on any of the 17 open findings.

Unaddressed findings (4 P2, 7 P3, 3 Nit, 2 Note):

CRF-2 (P2) LoadModules returns mutable cached slice
CRF-3 (P2) Unvalidated type cast to TemplateBuilderVariableType
CRF-4 (P2) Stub module ships in production binary
CRF-5 (P2) ToSDK test assertions cover 4 of 14 fields
CRF-1 (P3) Nil-to-empty normalization missing
CRF-6 (P3) "First successful call" comment lie
CRF-7 (P3) Pre-1.21 sync.Once pattern
CRF-8 (P3) No ID uniqueness/validation
CRF-9 (P3) Fixture doesn't exercise all schema fields
CRF-10 (P3) TestToSDK coupled to LoadModules
CRF-11 (P3) ReadFile error swallowed
CRF-18 (P3) RFC schema claim unverifiable
CRF-12 (Nit) Error message jargon
CRF-13 (Nit) Vague embed variable name
CRF-14 (Nit) Zero-value godoc comments
CRF-16 (Note) README describes unimplemented behavior
CRF-17 (Note) sync.Once blocks test isolation

Further review is blocked until the author responds to or pushes fixes for these findings.

🤖 This review was automatically generated with Coder Agents.

jeremyruppel · 2026-06-01T20:26:28Z

/coder-agents-review

coder-agents-review

Substantial improvement. 16 of 17 R1 findings verified fixed. The ParseModulesFromFS(fs.FS) refactor, type validation, fstest.MapFS test fixtures, and real code-server module are all well-executed. Test density went from 31% to 50%. Bisky on the new test suite: "That one was costume jewelry. This one is real."

One R1 finding re-opened: CRF-2 (mutable cached slice). Five reviewers independently verified that sync.OnceValues returns the same slice reference on every call. The initialization ergonomics improved (CRF-7 fix), but the mutation safety issue persists. Four new P3s from the panel.

R3 severity count: 1 P3 re-opened (CRF-2), 4 new P3, 2 new Nit.

CRF-24 (P3, not posted inline): The PR description still says "Stub module exercising all schema fields" but the stub was replaced with code-server in this revision. If squash-merged, the permanent commit body describes code that no longer exists. Update the description. (Leorio)

🤖 This review was automatically generated with Coder Agents.

code-asher

I just gave a cursory glance for now but I left some thoughts on the RFC too

aslilac · 2026-06-03T17:41:01Z

+		return nil, xerrors.Errorf("list module catalog entries: %w", err)
+	}
+
+	seen := make(map[string]bool)


Suggested change

seen := make(map[string]bool)

seen := make(map[string]struct{})

I sometimes feel like this is an over optimization, but I have seen other places in the code where when using a map like a set we use struct{} as the value type, the benefit being slightly better memory efficiency because struct{} is 0 bytes but bool is like 4 or 8 or something.

then to add something to the set is seen[manifest.ID] = struct{}{} which is hilariously ugly

Ouch my eyes! 😅 If we currently do this everywhere else (The Coder Way™️) then I'm down to make this change, but imo this reads pretty poorly, and unless we are really counting our bytes it doesn't feel like this will even move the needle. Strong opinion loosely held, though!

aslilac · 2026-06-03T17:41:12Z

+		}
+		seen[manifest.ID] = true
+
+		seenVars := make(map[string]bool)


same nit here

…ed wiring Scaffolds the coderd/templatebuilder package for the guided template builder (DEVEX-272). Adds: - codersdk.TemplateBuilderModule and related types matching the RFC schema - ModuleManifest internal type with go:embed wiring for bundled catalog - LoadModules() to parse module.json files from embedded FS - ToSDK() conversion from on-disk manifest to API response type - Stub module exercising all schema fields (builder_managed, sensitive, conflicts_with, compatible_os, pinned_version) - Tests for catalog loading, field parsing, and SDK conversion

- Replace stub module with real code-server module manifest - Export ParseModulesFromFS(fs.FS) for test isolation (no sync.Once coupling) - Use sync.OnceValues for the cached loader - Validate module ID (non-empty, unique), pinned_version, and variable types during parsing; reject unknown types at load time - Normalize nil slices to empty in ToSDK() to prevent null in JSON responses - Distinguish fs.ErrNotExist from other ReadFile errors - Improve error messages to describe what failed, not internal plumbing - Rewrite tests with fstest.MapFS fixtures covering all variable types, default pointer, validation errors, and full SDK field assertions - Trim zero-value godoc comments - Add future tense to README for unimplemented behavior; link RFC

it was a bad idea

Co-authored-by: McKayla はな <mckayla@hey.com>

…ternal test - Unexport ParseModulesFromFS to parseModulesFromFS since it is only used internally and in tests. - Move TestParseModulesFromFS into catalog_internal_test.go (package templatebuilder) so it can call the unexported function directly. - Fix go:embed directive: remove invalid trailing slash from pattern.

Co-authored-by: McKayla はな <mckayla@hey.com>

Rename BuilderManaged/builder_managed to Computed/computed across Go structs, JSON tags, module manifests, and tests. Regenerate TypeScript types.

…ctory A directory under modules/ without a module.json now returns an error instead of being silently skipped. The embed directive already excludes empty directories, so a dir without a manifest likely indicates a missing or misnamed file.

jeremyruppel · 2026-06-05T21:20:52Z

This stack of pull requests is managed by Graphite. Learn more about stacking.

github-actions Bot assigned jeremyruppel Jun 1, 2026

jeremyruppel changed the title ~~feat(coderd/templatebuilder): add module catalog structure and go:embed wiring~~ feat: add template builder module catalog structure and go:embed wiring Jun 1, 2026

jeremyruppel force-pushed the jeremy/devex-272-be-module-catalog-structure-goembed-wiring branch from 48e91e9 to bf131f3 Compare June 1, 2026 15:05

jeremyruppel commented Jun 1, 2026

View reviewed changes

Comment thread coderd/templatebuilder/README.md Outdated

jeremyruppel marked this pull request as ready for review June 1, 2026 19:23

coder deleted a comment from coder-agents-review Bot Jun 1, 2026

coder-agents-review Bot reviewed Jun 1, 2026

View reviewed changes

jeremyruppel force-pushed the jeremy/devex-272-be-module-catalog-structure-goembed-wiring branch from 8ee14bc to ebd6c99 Compare June 1, 2026 20:21

coder-agents-review Bot reviewed Jun 1, 2026

View reviewed changes

jeremyruppel force-pushed the jeremy/devex-272-be-module-catalog-structure-goembed-wiring branch 2 times, most recently from ccbe603 to cc381e1 Compare June 2, 2026 13:44

jeremyruppel requested review from aslilac, code-asher and dannykopping June 2, 2026 13:54

code-asher reviewed Jun 2, 2026

View reviewed changes

Comment thread coderd/templatebuilder/catalog.go Outdated

Comment thread coderd/templatebuilder/README.md Outdated

Comment thread coderd/templatebuilder/catalog.go Outdated

Comment thread coderd/templatebuilder/catalog_test.go Outdated

Comment thread codersdk/templatebuilder.go Outdated

code-asher reviewed Jun 2, 2026

View reviewed changes

Comment thread coderd/templatebuilder/modules/code-server/module.json

aslilac reviewed Jun 3, 2026

View reviewed changes

jeremyruppel removed the request for review from dannykopping June 5, 2026 16:09

jeremyruppel and others added 9 commits June 5, 2026 19:09

docs(coderd/templatebuilder): add package README

4548686

Delete coderd/templatebuilder/README.md

eeeabc7

it was a bad idea

Apply suggestion from @aslilac

5be0861

Co-authored-by: McKayla はな <mckayla@hey.com>

Update catalog.go

0880014

Co-authored-by: McKayla はな <mckayla@hey.com>

refactor(coderd/templatebuilder): rename builder_managed to computed

120ff9d

Rename BuilderManaged/builder_managed to Computed/computed across Go structs, JSON tags, module manifests, and tests. Regenerate TypeScript types.

jeremyruppel force-pushed the jeremy/devex-272-be-module-catalog-structure-goembed-wiring branch from 2a73c93 to 7bead3b Compare June 5, 2026 19:09

jeremyruppel requested review from aslilac and code-asher June 5, 2026 20:02

This was referenced Jun 5, 2026

feat(coderd/templatebuilder): add exampleID->OS map and base template .tf.tmpl files #26115

Open

feat: add GET /api/v2/templatebuilder/bases endpoint #26116

Open

feat: add GET /api/v2/templatebuilder/modules endpoint #26117

Open

	seen := make(map[string]bool)
	seen := make(map[string]struct{})

Conversation

jeremyruppel commented Jun 1, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

jeremyruppel commented Jun 1, 2026

Uh oh!

coder-agents-review Bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

coder-agents-review Bot Jun 1, 2026

Choose a reason for hiding this comment

Uh oh!

jeremyruppel Jun 1, 2026

Choose a reason for hiding this comment

Uh oh!

jeremyruppel Jun 2, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

coder-agents-review Bot left a comment

Choose a reason for hiding this comment

Uh oh!

jeremyruppel commented Jun 1, 2026

Uh oh!

coder-agents-review Bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

code-asher left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

aslilac Jun 3, 2026

Choose a reason for hiding this comment

Uh oh!

jeremyruppel Jun 5, 2026

Choose a reason for hiding this comment

Uh oh!

aslilac Jun 3, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

jeremyruppel commented Jun 5, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

jeremyruppel commented Jun 1, 2026 •

edited

Loading

jeremyruppel commented Jun 5, 2026 •

edited

Loading