First-pass review (Netero) only. The full reviewer panel has not yet reviewed this PR; it will run once the mechanical findings below are addressed. This is a clean, well-scoped scaffold: the base package is embedded through the existing //go:embed bases directive, the port from examples/templates/quickstart is faithful, and the open questions are correctly held inside one draft PR rather than spread across several. Law assessed the 1049-LOC headline and recommends against splitting: about 13 lines are novel logic and the rest is verbatim-ported template data plus a byte-identical golden duplicate.

Severity count: 1 P0, 1 Note.

Blocking issue: the quickstart-first ordering pin deterministically breaks the pre-existing TestTemplateBuilderBases/Sorted test in package coderd. Verified by running the test in a clean worktree. The PR introduces a new ordering contract (quickstart first, rest alphabetical) but leaves the test asserting the old contract (strictly alphabetical across all bases).

Note (process): the PR description reports go test ./coderd/templatebuilder/ passing, but the failing Sorted assertion lives in package coderd (coderd/templatebuilder_handler_test.go), which that command never exercised. Running the package that contains the handler test would have surfaced this. There is also no positive test asserting the new quickstart-first ordering; add one alongside fixing Sorted.

For the panel's next round, Law flagged that testdata/quickstart.tf.golden is byte-for-byte identical to bases/quickstart/main.tf.tmpl because the template carries no Go-template directives, so as Law put it, the golden test asserts only "the file we embedded equals the file we embedded." Weak but not vacuous; the panel will take a closer look after the P0 is fixed.

🤖 This review was automatically generated with Coder Agents.

Full panel review (round 2). The prior-round fixes are verified closed: the Sorted test now asserts resp.Bases[0].ID == "quickstart" and checks the remainder is name-sorted from index 2, which addresses the root cause of CRF-1, and the CRF-2 positive assertion is present. The ordering pin itself is clean and minimal, the port is faithful, and the open questions are disclosed honestly in a draft marked do-not-merge. Law's verdict stands: don't split.

Severity count: 1 P1, 2 P2, 1 P3, and several Notes/Nits.

The central issue is that the panel converged, from five independent angles, on one root cause: this is a fully-composed, batteries-included template shipped in the base slot, and the builder's model is base (infra) + modules composed on top. As Luffy put it, "It's not a base. It's a whole finished template wearing a base costume." That mismatch produces the P1 collision (CRF-4), the P2 registry hardcoding (CRF-6), the P2 dead JetBrains selector (CRF-5), and the P3 verbatim-copy drift (CRF-10). Open question #1, option (a) (strip modules/presets so the base is pure infra) dissolves all four at once. Verified in a worktree: quickstart is the only base that embeds catalog-named module blocks; the siblings declare at most azure_region/gcp_region, which are not catalog IDs, so this hazard is introduced by this PR, not pre-existing.

Independently of the #1 decision, the compose pipeline has no guard for base-declared module names colliding with wizard-selected ones. validateModules only dedupes the requested list. Even if this base is trimmed, the next base that legitimately bundles a catalog module hits the same wall. Pariston and Meruem propose the durable fix: declare the base's included module IDs (e.g. included_modules in base.json) and seed validateModules' seen-map from it, so the disjoint-namespace invariant is enforced by the same mechanism that already rejects duplicate requests.

The draft status and the open-questions block do not close these findings. There is no ticket for #1, #2, or #3, and a paragraph in the description is not a resolution. Each needs a human decision: fix it here, file a tracking issue, or explicitly accept the gap.

Process notes (not blocking): the Testing section still lists only go test ./coderd/templatebuilder/, which cannot exercise the ordering logic or its new assertion (both live in package coderd); the author acknowledged this in a reply but the PR body is unchanged, so update it to cite go test ./coderd/. The PR description uses emdashes throughout; the shipped files are clean so make lint/emdash will not fail, but the body violates the house standard. Minor accuracy: the README is not byte-verbatim (prettier reformatted the table separators), so "verbatim + markers" slightly overstates it.

coderd/templatebuilder_handler_test.go:44

Note [CRF-11] Quickstart is absent from the handler-level specs table, so its API surface is only checked by generic loops (Bisky).

TestTemplateBuilderBases/OK asserts name, icon, OS, and variable presence for a curated specs list; quickstart is not in it, so the handler never asserts which variables it exposes (currently none). Given open question #3 (whether Quickstart should expose a container_image selector), an explicit spec row would lock the current answer and flag drift. Worth knowing, not blocking.

🤖

🤖 This review was automatically generated with Coder Agents.

Round 3 status: review is on hold. The churn guard classified the prior findings and the panel did not re-run this round, because the code is essentially unchanged (only CRF-9 landed) and the substantive findings are waiting on a decision that has not happened yet.

What moved:

• CRF-9 (unstable/non-total sort): fixed in 76aabd3. Confirmed in a worktree: the comparator now tiebreaks on ID (if bases[i].Name != bases[j].Name { ... } return bases[i].ID < bases[j].ID), so the order is total and deterministic even if two bases share a display name. Closed. Switching to slices.SortFunc + cmp.Compare remains optional.

Held pending the open-question-#1 scope decision (escalated to the scope owner):

• CRF-4 (P1, module collision), CRF-5 (P2, dead JetBrains selector), CRF-6 (P2, hardcoded registry), CRF-10 (P3, verbatim-copy drift). You confirmed these share one root and committed to a concrete fix under either option (a) strip to infra or (b) collision guard. That is the right call, and not shipping a first-position base that breaks on the wizard's core action is correct. These stay open findings, not resolved, until the chosen option is implemented or a tracking ticket is filed. A human decision is what unblocks them.

Acknowledged with reasoning, no action expected now:

• CRF-7 (container image, open q#3), CRF-8 (ID pin in comparator, open q#2), CRF-3 (golden equals source).

One finding is unaddressed with no response and no code change:

• CRF-11 (Note): quickstart is absent from the handler-level specs table in coderd/templatebuilder_handler_test.go, so its API surface (currently no variables) is only checked by generic loops. It was folded into the round-2 review body, so there is no thread to reply on. Either add a specs row for quickstart or say why it should not be locked. This is minor, but it is the one item with no engagement.

The round-2 CHANGES_REQUESTED still stands. The panel will re-review once you push the open-question-#1 implementation (or file a ticket for it) and address or respond to CRF-11. Re-request review then.

🤖 This review was automatically generated with Coder Agents.

We chatted, follow ups for you -- move it to be next to the Docker template and also remove the IDE selector

Full panel review (round 4). The scope decision landed and was implemented well. The base is trimmed to infra plus a language selector and optional git-clone, the dead IDE selector is gone (CRF-5), the ordering is a clean group-before-Docker with a total-order comparator (CRF-8/CRF-9), the baseSpec row is in (CRF-11), and the collision guard is real: it rejects a base-included module in compose and hides it from the modules endpoint, with tests on both paths. Verified in a worktree. CRF-4's git-clone instance is genuinely fixed. As Robin put it, "You found the seam and cut it. But the poneglyph next to it still has an exact twin."

Severity count: 2 P2, 5 P3, 1 Note, 1 Nit. Two prior findings re-raised (CRF-6, CRF-10).

The two P2s both trace to one decision: keeping a catalog module (git-clone) embedded in the base.

• CRF-6 (re-raised, six reviewers): the base's git-clone still hardcodes registry.coder.com, and this is structural, not a stray literal. BaseRenderContext has no RegistryBase field and renderBase is never passed the deployment RegistryURL, so a base cannot honor CODER_TEMPLATE_BUILDER_REGISTRY_URL the way wizard-composed modules do. The same git-clone module resolves through the mirror when the wizard renders it but reaches the public registry when the base embeds it, on the same deployment. Fair context from Hisoka and Knov: this base is unusable fully air-gapped regardless (it pulls the enterprise image, apt, nodesource, go.dev, rustup), so the real value here is mirror consistency on internet-connected deployments, and three shipped bases already hardcode region-module sources, so this is a base-layer class. It still needs a human decision (no ticket): thread RegistryBase into base rendering, drop git-clone from the base and let the wizard provide it (which also removes the guard's need for git-clone, per Knov/Melody/Pariston), or track it.
• CRF-13 (two reviewers): the README promises languages are "installed on first start and cached for subsequent starts," but the installs land in the ephemeral container (only /home/coder persists), so they reinstall from the network on every start, and start_blocks_login = true holds the user's login during that reinstall. On mirror/air-gapped deployments the reinstall fails and login never unblocks. Fix the doc to match the behavior, or persist the toolchains.

The collision guard works but its invariant is not enforced (CRF-12, eight reviewers): included_modules is a hand-maintained copy of the base's actual module blocks with nothing binding them. Add a module "code-server" block to any base and forget the JSON line, and CRF-4's P1 duplicate-module break returns silently. The fix addressed the git-clone instance; the durable fix is mechanical, a test that parses each base's rendered module "<id>" labels and asserts they equal that base's included_modules (or derive the set from the template and delete the field). AGENTS.md prefers mechanical enforcement over a remembered rule, and this guard exists to prevent a P1.

Smaller items: CRF-10 re-raised, the drift class survived the trim in install-languages.sh.tftpl (still byte-identical to the example); CRF-14 unanchored grep language dispatch (safe today, latent); CRF-15 a vacuous negative test loop; CRF-16 the collision rationale is duplicated ~6 times across the diff (also at bases.go:268); CRF-17 the "bidirectional" conflict comment overpromises for base-included modules; CRF-18 stdlib modernization for the comparator/regroup. No action needed on CRF-3 (the directive-free golden still asserts source==source; all reviewers concur it is low-value but not vacuous). CRF-7 (container image) remains deferred; the scope owner did not require it, but there is no ticket, so it needs an explicit accept-or-track. Luffy also noted two of the four presets ("Backend (Go)", "Data Science") select a single language the dropdown already offers.

coderd/templatebuilder/compose.go:236

Note [CRF-17] The "Check conflicts bidirectionally" loop iterates only requested modules, so a base-included module's own ConflictsWith is never enforced (Mafuuu, Melody).

Base module IDs are seeded into seen, so a requested module that declares a conflict with a base module is caught, but the base module's own ConflictsWith list is never consulted. If a base ever includes module A where A declares a conflict with B but B does not reciprocate, selecting B composes cleanly. Latent today (git-clone's conflicts_with is empty and catalog conflicts appear symmetric); flagged because the comment claims a bidirectionality that does not extend to base-included modules.

🤖

🤖 This review was automatically generated with Coder Agents.

Full panel review (round 5), a verification round. This is in good shape: all seven R4 fixes hold up under the panel's checks, and I'm dismissing the prior CHANGES_REQUESTED because no blocking (P0-P2) findings remain.

Verified fixed: CRF-12 (the invariant test TestBaseIncludedModulesMatchRendered renders each base, extracts module "<id>" labels via ExtractModuleNames, and asserts they equal included_modules, so the manifest can no longer silently drift, this closes the class, not the instance), CRF-13 (README no longer promises caching), CRF-14 (has_language whole-entry match), CRF-15 (require.NotEmpty), CRF-16 (comment dedup), CRF-17 (honest ConflictsWith comment), CRF-18 (slices.SortFunc/IndexFunc/Insert). As Bisky put it, "Costume jewelry does not survive this cut."

Contested finding closed in the author's favor: CRF-10 (base install-languages.sh drift). Eight reviewers independently diffed the base against the example and confirmed the base has genuinely diverged (base uses has_language, the example still uses grep -q), so a base==example test would wrongly block legitimate divergence, and the real manifest-vs-rendered drift is now covered by CRF-12. Defense valid.

Deferred/accepted with tickets, not re-raised: CRF-6 (git-clone registry, DOCS-610 with a stacked follow-up PR) and CRF-7 (container image, DOCS-611). The in-code CRF-6 note was verified accurate.

Remaining items are all P3 or below, none blocking:

• CRF-20 (P3): the language selector's option values and the install script's has_language branches are the same kind of hand-maintained two-list pairing you just made mechanical for included_modules, but with no cross-check test. Verified: six values, six branches, they agree today, nothing binds them. Drift silently no-ops a selected language (the CRF-5 shape). The same ExtractModuleNames-style test would close it.
• CRF-23 (P3): CRF-13's doc fix is honest, but the behavior it now documents (every start reinstalls toolchains from the network and blocks login) is a real limitation on network-restricted deployments with no ticket. DOCS-610 covers the git-clone registry, not this. Per the no-follow-up rule, confirm it is tracked or explicitly accepted.
• CRF-21 (Note): the CRF-12 invariant test renders with DefaultBaseRenderContext only, but production renderBase overlays merged variables, so a base that gated a catalog module block on a variable would evade the guard. Dormant today (quickstart declares no variables), worth a comment or a variable-context case if bases ever gain variables.
• CRF-22 (Note): the README's leading "reinstalled on every start" is not literally true for Rust ($HOME/.cargo persists); the trailing "most" hedges it, and Leorio (docs) considers the current wording acceptable. Optional refinement.
• CRF-24 (Nit): the sort-site comment still repeats the groupQuickstartBeforeDocker rationale that the helper's own doc carries (a residual of the CRF-16 dedup).

Out of this PR's scope but worth a follow-up: the source example examples/templates/quickstart/install-languages.sh.tftpl still carries the old unanchored-grep substring dispatch that CRF-14 fixed in the base (flagged by Mafu-san, Hisoka, Razor). Port has_language to the example in its own change. ging-go also noted the adjacency loop in the Sorted test could be slices.IsSorted (loses the per-pair failure message, so optional).

🤖 This review was automatically generated with Coder Agents.

Full panel review (round 6), verification. All four R5 fixes hold, no blocking findings remain, and I'm dismissing the prior CHANGES_REQUESTED.

Verified closed: CRF-20 (the language-selector-vs-has_language drift guard is real, six reviewers independently mutation-tested it, renaming a script branch or a selector option fails the test loudly), CRF-21 (the default-context caveat on the invariant test is accurate), CRF-22 (the README now names Rust as the persist-and-reuse exception, matching the $HOME/.cargo guard), CRF-24 (the sort-site comment is a one-line pointer). Deferrals with tickets confirmed and not re-raised: CRF-6 (DOCS-610), CRF-7 (DOCS-611), CRF-23 (DOCS-614); the out-of-scope example-grep is tracked (DOCS-613). As Hisoka put it: "Boring, in the way a well-built thing is boring."

One new P3 and one Note, neither blocking:

• CRF-25 (P3): the same drift class the panel closed with CRF-5 and CRF-20 has a third consumer the new guard does not cover. The four coder_workspace_preset blocks declare languages a third time (web_dev, backend_go, data_science, full_stack), and nothing binds them to the selector. Today all preset values are valid, so it is latent, exactly as CRF-20 was when rated P3. Extend TestQuickstartLanguageSelectorMatchesInstallScript to also assert each preset's decoded languages is a subset of the selector options.
• CRF-19 (Note): ExtractParameterOptionValues and ExtractModuleNames hand-roll HCL parsing (a brace-depth scan that counts braces inside strings/heredocs, regexes that don't skip comments) with no direct edge-case test. Hisoka landed the probe rather than reasoning about it: an unbalanced { makes the extractor return nil, which trips require.NotEmpty and fails the test loudly, and these functions are test-only, so the blast radius is a red test on the developer who edits the base, not a mis-installed language in a user's workspace. It fails toward safety, which is why it stays a Note. Robin proposes the structural fix (use hashicorp/hcl/v2 hclsyntax, already a dependency and already used in tfparse.go to scope to a named coder_parameter block), which deletes the hand-rolled scanner and its assumption; a table test over crafted HCL is the lighter alternative. Zoro's counter is fair: a regex cannot balance braces, so the manual scan is the only regex-based option, but that is an argument for the parser, not for the scan.

The collision guard, the two drift invariants, the total-order sort, and the trimmed base all hold across the panel. Nothing here blocks merge.

🤖 This review was automatically generated with Coder Agents.

Non-blocking design suggestion on the base/module collision guard. The current approach keeps two lists in sync (the included_modules manifest field and the modules the base's Terraform actually renders) and adds an invariant test to police the drift. Since the base already declares the modules it bundles by literally rendering them, and Compose already renders the base before validating modules, we could derive the included set from the rendered HCL via the new ExtractModuleNames helper instead of maintaining a parallel manifest field. That removes the concept, the field, and the sync test, and promotes ExtractModuleNames from a test-only helper to a real production caller. Left inline notes on the specific lines. Details below.

// bases.go — replace the manifest field + lookup with a derived, cached set.
var baseIncludedModules = sync.OnceValues(func() (map[string][]string, error) {
    bases, err := loadBases()
    if err != nil {
        return nil, err
    }
    out := make(map[string][]string, len(bases))
    for id := range bases {
        mainTF, err := RenderBaseTemplate(id, "main.tf.tmpl", DefaultBaseRenderContext(id))
        if err != nil {
            return nil, xerrors.Errorf("render base %q: %w", id, err)
        }
        out[id] = ExtractModuleNames(mainTF)
    }
    return out, nil
})

func BaseIncludedModules(exampleID string) []string {
    all, err := baseIncludedModules()
    if err != nil {
        return nil
    }
    return all[exampleID]
}

Follow-up on the extraction helpers and test scope. The selector/script drift this test guards is genuinely worth catching (it's silent and cross-language), but it can be done with a small regex mirroring the existing has_language one, without the HCL extractor family. Bundling piecemeal HCL parsing into this base PR is what creates the test-only exported surface. Suggest keeping regex here and landing the HCL migration as its own focused effort. Inline notes below.

Generated by Coder Agents on behalf of @jeremyruppel.

looks great! this is an awesome addition to template builder ❤️