feat(registry/coder/modules/coder-utils): make install_script and start_script optional by 35C4n0r · Pull Request #842 · coder/registry

35C4n0r · 2026-04-13T16:02:38Z

Description

Rework the coder-utils module to make start_script optional while keeping install_script required. Adds mkdir -p to every script block, updates the module directory path to a namespaced location, moves all sync dependency resolution into locals, and bumps Terraform to >= 1.9.

Type of Change

Feature/enhancement

Module Information

Path: registry/coder/modules/coder-utils
New version: v1.1.0
Breaking change: [x] Yes [ ] No

install_script is now required (previously had default = null). Callers that omitted it will need to provide a value. start_script now defaults to null (previously required). The module directory path changed from $HOME/<name> to $HOME/.coder-modules/coder/coder-utils/<name>.

Testing & Validation

Tests pass (terraform test — 7 passed, 0 failed)
Code formatted
Changes tested locally

Changes

`main.tf`

install_script: now required (no default). Resource always created (no count).
start_script: added default = null. Resource conditionally created with count.
All four script blocks now include mkdir -p to ensure the module directory exists.
module_dir_path updated to $HOME/.coder-modules/coder/coder-utils/<module_dir_name>.
Sync deps moved to locals (install_sync_deps, start_sync_deps). Post-install always waits for install. Start waits for install (+ post_install when present). Install waits for pre_install when present.
install_script_name output is always set. Other outputs return "" when variable is not provided.
Required Terraform version bumped to >= 1.9.

`main.tftest.hcl`

Updated for required install_script and optional start_script.
Added test_install_syncs_with_pre_install and test_start_syncs_with_post_install to verify sync dependency chains.

`main.test.ts`

install_script added back to required variables.

`README.md`

Version bumped to 1.1.0.
Execution order updated: install is required, start is optional.

Related Issues

Closes: #848

Generated with Coder Agents

…rt_script optional Both variables now default to null. Their coder_script resources are conditionally created with count, matching the existing pattern for pre_install_script and post_install_script. Sync dependency resolution is computed in locals so each stage waits for its closest existing predecessor in the chain: pre_install -> install -> post_install -> start. Outputs return null when the corresponding variable is not provided.

Copilot

Pull request overview

Makes install_script and start_script optional in the registry/coder/modules/coder-utils Terraform module, aligning behavior with the existing optional pre/post install scripts and updating tests accordingly.

Changes:

Make start_script optional (adds default = null) and gate install_script/start_script resources behind count.
Update sync dependency resolution to handle missing stages in the pre→install→post→start chain.
Update Terraform/Bun tests to reflect count-based resources and nullable outputs, adding additional test scenarios.

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.

File	Description
`registry/coder/modules/coder-utils/main.tf`	Makes install/start scripts optional, adds sync-dependency locals, and makes outputs nullable.
`registry/coder/modules/coder-utils/main.tftest.hcl`	Updates tests for count-based resources and adds new test scenarios for missing scripts.
`registry/coder/modules/coder-utils/main.test.ts`	Updates required-variable test to no longer require `start_script`.

Comments suppressed due to low confidence (1)

registry/coder/modules/coder-utils/main.tf:188

start_script is now optional and can be the only script provided, but this script doesn't create ${local.module_dir_path} before writing ${local.start_path} / ${local.start_log_path}. In a start-only configuration (which is now explicitly supported by tests), the script will fail at runtime because the directory doesn't exist. Add mkdir -p ${local.module_dir_path} near the top of this script (or otherwise ensure the directory exists) so start-only works end-to-end.

  script       = <<-EOT
    #!/bin/bash
    set -o errexit
    set -o pipefail

    trap 'coder exp sync complete ${local.start_script_name}' EXIT

    %{if local.start_sync_deps != null~}
    coder exp sync want ${local.start_script_name} ${local.start_sync_deps}
    %{endif~}
    coder exp sync start ${local.start_script_name}

    echo -n '${local.encoded_start_script}' | base64 -d > ${local.start_path}
    chmod +x ${local.start_path}

…locks and update module_dir_path Every script block now ensures the module directory exists, since any script can be the first (and only) one to run. Updated module_dir_path to use the namespaced path $HOME/.coder-modules/coder/coder-utils/<module_dir_name>.

…of null for unused outputs

matifali · 2026-04-13T17:44:01Z

Deep Review

The core goal — making install_script and start_script optional via count gating, matching the existing pattern for pre_install_script / post_install_script — is sound. The sync dependency chain logic (post_install_sync_deps, start_sync_deps) is correct and handles all combinations gracefully. Tests are thorough with good coverage of edge cases.

A few things to address before merging:

1. Undocumented breaking change: `module_dir_path` moved

This is the most significant concern. The PR silently changes module_dir_path from:

module_dir_path = "$HOME/${var.module_dir_name}"

to:

module_dir_path = "$HOME/.coder-modules/coder/coder-utils/${var.module_dir_name}"

This changes the on-disk path for all scripts, logs, and artifacts. It's not mentioned anywhere in the PR description. If intentional, it should be called out explicitly and justified. If not, it should be reverted from this PR and handled separately.

2. PR description says outputs return `null` — they return `""`

The description states:

"All four outputs now return null when the corresponding variable is not provided."

But the actual code (and all test assertions) return "" (empty string). Looks like this was fixed after the Copilot review but the description wasn't updated.

3. Missing newline at end of `main.tf`

The diff ends with \ No newline at end of file. Trivial fix.

4. Versioning: `1.1.0` but marked as breaking

The README bumps the version to 1.1.0, but the PR description checks [x] Yes for breaking change. If it's truly breaking (and the module_dir_path change makes it so), semver convention would be 2.0.0. If the argument is that existing callers who provide these variables are unaffected, then uncheck the breaking change box.

5. Sync dependency chain — verified correct

I traced through all combinations:

pre_install	install	post_install	start	`post_install_sync_deps`	`start_sync_deps`
Y	Y	Y	Y	install	install + post_install
N	Y	N	Y	—	install
Y	N	Y	Y	pre_install	post_install
N	N	N	Y	—	null
Y	N	N	Y	—	pre_install

All correct. The chain degrades gracefully when any middle step is absent.

6. `mkdir -p` added to `post_install_script` and `start_script` — good

With install_script now optional, the directory may not exist when later scripts run. The mkdir -p addition is the right fix.

7. Minor: test coverage gap

No test for post_install_only (just post_install_script, no other scripts). This would verify post_install_sync_deps resolves to null when both pre_install_script and install_script are absent.

Area	Status
Core optionality logic	✅ Good
Sync dependency chain	✅ Correct
Test coverage	✅ Good (minor gap)
`mkdir -p` fix	✅ Good
`module_dir_path` change	⚠️ Undocumented — needs discussion
PR description accuracy	⚠️ Needs update (`null` → `""`)
Version bump vs breaking flag	⚠️ Inconsistent
Missing newline at EOF	🔧 Trivial fix

🤖 This response was generated by Coder Agents.

…uired, keep others optional install_script is now mandatory (no default). pre_install_script, post_install_script, and start_script remain optional with default null. Since install is always present, sync deps simplify: post_install always waits for install, start always waits for install (plus post_install when present). Removed validation blocks and fallback chains that are no longer needed. Bumped required Terraform version to >= 1.9.

…to locals

…and update README Add test_install_syncs_with_pre_install to verify install waits for pre_install via install_sync_deps local. Add sync assertion to test_with_all_scripts. Update README execution order to reflect install_script is required.

…post_install and start Both scripts sync-wait on install, which always creates the directory.

…_directory for clarity

…above

…o single script_names object Replace four individual outputs with a single script_names output containing pre_install, install, post_install, and start fields.

DevelopmentCats

LGTM provided we have tested this, but if not I can finagle something together to test it all

35C4n0r · 2026-04-14T18:09:59Z

LGTM provided we have tested this, but if not I can finagle something together to test it all

Currently testing it in the new boundary module. Will merge this right before #840.
Thanks for the review @DevelopmentCats

Downstream modules (e.g. claude-code) can now brand the four scripts this module emits so the Coder UI shows 'Claude Code: Install Script' with the Claude icon instead of the generic defaults. - display_name_prefix (string, default "") prepends '<prefix>: ' to every coder_script display_name. - icon (string, default null) sets icon on every coder_script. - Drive-by: fix stale 'module_dir_name' reference in README; the var was renamed to 'module_directory' earlier on this branch.

matifali · 2026-04-22T07:06:21Z

Pushed 6678b04 on top of this branch to add two optional variables so downstream modules (starting with claude-code) can brand the scripts this module emits:

display_name_prefix (string, default "") — prepends "<prefix>: " to every coder_script.display_name.
icon (string, default null) — sets icon on every coder_script.

Existing tests unchanged (defaults preserve the old behavior). Two new cases in main.tftest.hcl cover both vars. README updated with a "Customizing Script Display" section.

Also fixed a stale module_dir_name reference in the README example (the variable was renamed to module_directory earlier on this branch).

Let me know if you'd rather I split this into a follow-up PR once this one merges.

🤖 This response was generated by Coder Agents.

Pass display_name_prefix = "Claude Code" and icon = "/icon/claude.svg" through to the coder-utils module so the workspace shows "Claude Code: Install Script" with the Claude icon instead of the generic defaults. Relies on the two new variables added to coder-utils on PR #842's branch.

Close a coverage gap: existing tests only set every script variable and verify presence. Nothing checked that pre-install, post-install, and start coder_script resources stay absent when their inputs are unset. Downstream consumers (e.g. claude-code without pre/post scripts) rely on this.

Expose a new `output "scripts"` that returns a run-ordered list of the `coder exp sync` names this module actually emits. Absent scripts (pre_install, post_install, or start when their inputs are null) are omitted from the list entirely, not padded with empty strings. This lets downstream modules consume the list directly with `coder exp sync want <self> <each>` to serialize behind the install pipeline, instead of re-deriving the filter from `script_names` on every consumer. Adds 3 tftest cases covering all-scripts-present, install-only, and install+post configurations.

Replace the local `for name in [...] : name if name != ""` filter with a one-line passthrough of `module.coder-utils.scripts`, the new upstream output that returns the run-ordered, filtered list of `coder exp sync` names. The filtering logic is generic to any module that wraps coder-utils, so it lives better there than in every consumer. claude-code just forwards it under its own `output "scripts"` so downstream templates keep the same surface. Requires PR #842 on coder/registry (feat/coder-utils-optional-install-start).

matifali · 2026-04-22T12:40:21Z

One more addition on this branch (178c609): new output "scripts" that returns the run-ordered, filtered list of coder exp sync names this module actually creates, with absent scripts omitted.

This is generic to anything wrapping coder-utils, so it belongs here rather than being re-derived in every consumer. It unblocks a simplification in #859 (claude-code) where the same filter was being done locally. 3 new tftest cases cover all-scripts-present, install-only, and install+post.

🤖 This response was generated by Coder Agents.

Each coder_script wrapper used to run the user's script with ${script_path} > ${log_path} 2>&1 which redirects combined output straight to the log file. Workspace users watching the coder_script output in the Coder UI during workspace start would see the script's display name but nothing else until the script finished, because every line the script emitted went to the file on disk and never to the agent's stdout. Switch to ${script_path} 2>&1 | tee ${log_path} so combined stdout and stderr fans out to both the log file (for after-the-fact inspection and debugging) and the agent's stdout (for live progress in the UI). pipefail is already set, so a failing inner script still propagates its exit code through the pipeline, the errexit trap still fires, and coder exp sync complete still runs. Applied uniformly to pre_install, install, post_install, and start. Adds a regex-based tftest that fails if any of the four wrappers stops teeing in the future.

35C4n0r marked this pull request as ready for review April 13, 2026 16:41

35C4n0r self-assigned this Apr 13, 2026

35C4n0r requested a review from Copilot April 13, 2026 16:41

Copilot started reviewing on behalf of 35C4n0r April 13, 2026 16:42 View session

Copilot AI reviewed Apr 13, 2026

View reviewed changes

Comment thread registry/coder/modules/coder-utils/main.tf

Comment thread registry/coder/modules/coder-utils/main.tf Outdated

Jay Kumar added 3 commits April 13, 2026 17:19

fix(registry/coder/modules/coder-utils): return empty string instead …

8729122

…of null for unused outputs

docs(registry/coder/modules/coder-utils): bump version to 1.1.0

9127a11

35C4n0r force-pushed the feat/coder-utils-optional-install-start branch from 51089bd to 9127a11 Compare April 13, 2026 17:30

35C4n0r requested review from DevelopmentCats and matifali April 13, 2026 17:35

DevelopmentCats reviewed Apr 13, 2026

View reviewed changes

Comment thread registry/coder/modules/coder-utils/main.tf Outdated

Jay Kumar and others added 7 commits April 13, 2026 17:55

refactor(registry/coder/modules/coder-utils): move install sync deps …

8a9ab44

…to locals

fix(registry/coder/modules/coder-utils): remove redundant mkdir from …

e942c7c

…post_install and start Both scripts sync-wait on install, which always creates the directory.

refactor(coder/modules/coder-utils): rename module_dir_name to module…

e0a4670

…_directory for clarity

fix(terraform): update required_version to support Terraform 1.0 and …

22966c5

…above

chore: update test

8216a71

refactor(registry/coder/modules/coder-utils): consolidate outputs int…

94f1d3a

…o single script_names object Replace four individual outputs with a single script_names output containing pre_install, install, post_install, and start fields.

DevelopmentCats approved these changes Apr 14, 2026

View reviewed changes

35C4n0r mentioned this pull request Apr 16, 2026

Rework coder-utils module: make start_script optional, install_script required #848

Open

matifali approved these changes Apr 22, 2026

View reviewed changes

matifali mentioned this pull request Apr 22, 2026

refactor(registry/coder/modules/claude-code): slim to install-and-configure only #859

Closed

35C4n0r commented Apr 22, 2026

View reviewed changes

Comment thread registry/coder/modules/coder-utils/main.tf

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(registry/coder/modules/coder-utils): make install_script and start_script optional#842

feat(registry/coder/modules/coder-utils): make install_script and start_script optional#842
35C4n0r wants to merge 16 commits intomainfrom
feat/coder-utils-optional-install-start

35C4n0r commented Apr 13, 2026 •

edited

Loading

Uh oh!

Copilot AI left a comment

Uh oh!

Uh oh!

Uh oh!

matifali commented Apr 13, 2026

Uh oh!

Uh oh!

DevelopmentCats left a comment

Uh oh!

35C4n0r commented Apr 14, 2026

Uh oh!

matifali commented Apr 22, 2026

Uh oh!

Uh oh!

matifali commented Apr 22, 2026 •

edited

Loading

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Conversation

35C4n0r commented Apr 13, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Type of Change

Module Information

Testing & Validation

Changes

main.tf

main.tftest.hcl

main.test.ts

README.md

Related Issues

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Uh oh!

Uh oh!

matifali commented Apr 13, 2026

Deep Review

1. Undocumented breaking change: module_dir_path moved

2. PR description says outputs return null — they return ""

3. Missing newline at end of main.tf

4. Versioning: 1.1.0 but marked as breaking

5. Sync dependency chain — verified correct

6. mkdir -p added to post_install_script and start_script — good

7. Minor: test coverage gap

Uh oh!

Uh oh!

DevelopmentCats left a comment

Choose a reason for hiding this comment

Uh oh!

35C4n0r commented Apr 14, 2026

Uh oh!

matifali commented Apr 22, 2026

Uh oh!

Uh oh!

matifali commented Apr 22, 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!

4 participants

35C4n0r commented Apr 13, 2026 •

edited

Loading

`main.tf`

`main.tftest.hcl`

`main.test.ts`

`README.md`

1. Undocumented breaking change: `module_dir_path` moved

2. PR description says outputs return `null` — they return `""`

3. Missing newline at end of `main.tf`

4. Versioning: `1.1.0` but marked as breaking

6. `mkdir -p` added to `post_install_script` and `start_script` — good

matifali commented Apr 22, 2026 •

edited

Loading