Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
51 changes: 35 additions & 16 deletions common/install.sh
Original file line number Diff line number Diff line change
Expand Up @@ -65,10 +65,10 @@ fetch_release_asset() {
fi

id="$(curl -fsSL \
-H "Authorization: Bearer ${GITHUB_TOKEN}" \
-H 'Accept: application/vnd.github+json' \
"https://api.github.com/repos/${RepoSlug}/releases/latest" \
| release_asset_id "$name")" || return 1
-H "Authorization: Bearer ${GITHUB_TOKEN}" \
-H 'Accept: application/vnd.github+json' \
"https://api.github.com/repos/${RepoSlug}/releases/latest" |
release_asset_id "$name")" || return 1

curl -fsSL \
-H "Authorization: Bearer ${GITHUB_TOKEN}" \
Expand Down Expand Up @@ -186,11 +186,11 @@ start_stack() {
# package refuses an anonymous pull with `unauthorized`.
# --password-stdin, not an argument: argv is visible to every other user on the host.
if [ -n "${GITHUB_TOKEN:-}" ]; then
printf '%s' "${GITHUB_TOKEN}" \
| docker login ghcr.io -u "${RepoSlug%%/*}" --password-stdin >/dev/null || {
echo 'could not sign in to ghcr.io; the token needs read:packages' >&2
return 1
}
printf '%s' "${GITHUB_TOKEN}" |
docker login ghcr.io -u "${RepoSlug%%/*}" --password-stdin >/dev/null || {
echo 'could not sign in to ghcr.io; the token needs read:packages' >&2
return 1
}
fi
docker compose up --remove-orphans -d || return 1
}
Expand All @@ -200,7 +200,8 @@ start_stack() {
# pipeline as failed. Measured at roughly one run in seven — a stack that
# refused to migrate, at random, with a message about a service that was there.
compose_has_service() {
local -r service="$1"; shift
local -r service="$1"
shift
local services

services="$(docker compose "$@" config --services)" || return 1
Expand All @@ -225,15 +226,33 @@ run_migrations() {
}

main() {
command -v curl >/dev/null || { echo 'curl is required'; return 1; }
docker compose version >/dev/null 2>&1 || { echo 'docker compose is required'; return 1; }
command -v curl >/dev/null || {
echo 'curl is required'
return 1
}
docker compose version >/dev/null 2>&1 || {
echo 'docker compose is required'
return 1
}
require_private_tools || return 1

create_directory || { echo 'could not create the target directory'; return 1; }
download_release_assets || { echo 'could not download the release assets'; return 1; }
create_directory || {
echo 'could not create the target directory'
return 1
}
download_release_assets || {
echo 'could not download the release assets'
return 1
}
require_configured_image || return 1
start_stack || { echo 'could not start the stack; check the output above'; return 1; }
run_migrations || { echo 'could not run migrations; check the output above'; return 1; }
start_stack || {
echo 'could not start the stack; check the output above'
return 1
}
run_migrations || {
echo 'could not run migrations; check the output above'
return 1
}

# One line per application (ADR-0022), read out of .env so it reflects any
# port the operator changed.
Expand Down
81 changes: 81 additions & 0 deletions docs/superpowers/plans/2026-09-16-clean-shell-and-comments.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,81 @@
# Clean Shell and Comments Implementation Plan

> **For agentic workers:** REQUIRED SUB-SKILL: Use superpowers:subagent-driven-development (recommended) or superpowers:executing-plans to implement this plan task-by-task. Steps use checkbox (`- [ ]`) syntax for tracking.

**Goal:** every tracked file meets `comment-code`; every shell file meets the Clean Bash rules; lint keeps it that way.

**Architecture:** five independent pull requests by area, each merged before the next branches. No behaviour change anywhere; the existing bats suites and real-project checklists are the regression net.

**Tech Stack:** bash 5, shellcheck 0.11.0, shfmt 3.14.1, bats 1.13.0, mise.

**Spec:** `docs/superpowers/specs/2026-09-16-clean-shell-and-comments-design.md`

## Global Constraints

- Rules: the `comment-code` skill (`~/.claude/skills/comment-code/SKILL.md`) and `~/.dotfiles/agentic/rules/bash.md`. Read both before editing.
- No behaviour change: error messages, stdout/stderr lines, exit codes and generated-file contents stay byte-identical. Never edit a test assertion to make a code change pass.
- No file-level `readonly`; `local -r` inside functions is fine.
- No `set -euo pipefail` added to sourced files (`lib/*.sh`, `services/**/*.sh`, `tests/helpers/*.bash`, fixture drivers). Executables (`scaffold`, `scripts/*.sh`, `common/install.sh`) must have it.
- Bats `@test` bodies keep `[ … ]`; helpers and non-test shell use `[[ ]]` / `(( ))`.
- `[ ]` → `[[ ]]`: quote the RHS of `=`, `==`, `!=` unless a glob is intended. Numeric tests → `(( ))`.
- `echo` → `printf '%s\n'` only where a variable/expansion is printed.
- Never `local x="$(cmd)"`: declare, then assign.
- Split a function over 20 lines only at a nameable seam. Linear sequences (big `case`, heredoc, render loop) stay; record each kept function and why in the task report.
- Function names that appear in `docs/` or `README.md` keep their name and contract: `add_app_service apply_adapter apply_service_dockerfile apply_service_drivers assemble_compose assert_known_tiers cmd_add cmd_new cmd_wizard config_roots generate_service_passwords init_project lint_adapters lint_services load_adapter load_toolchain_env project_name_is_usable register_config_root register_image_target role_path service_compose_key service_driver_dockerfile service_healthy sync_ci_roots tui_name_is_usable tui_prompt_name wizard_actions wizard_command wizard_new_args wizard_options wizard_prompt_for wizard_questions write_env_lines splice_flask_probe init_flask_alembic assert_nest_probe_spliced`.
- Keep: `#!` lines, `# shellcheck …` pragmas (each carrying a same-line reason), `# noqa`, `eslint-disable`, `@phpstan-`, `ponytail:` markers, and splice anchors `# @SERVICE_SETUP@`, `# @DB_ENGINE@`, `# @DB_PROBE@` (column and spelling are matched by awk/sed).
- Do not touch: `verbatim` rows of `docs/PROVENANCE.md` (`common/.editorconfig`), `common/docs/.vitepress/theme/vendor/**`, `docs/**`, `*.md`, `LICENSE`, `mise.lock`, `UPSTREAM`.
- Script headers: purpose + usage only. No `Description :`/`Author`/banner blocks.
- Commits: conventional (`chore:`, `refactor:`, `style:`), no co-author noise beyond repo convention. Each PR from a fresh branch off up-to-date `main`.
- Verification per PR: `mise run lint` clean; `mise run test-runner` exits 0.
- Real-project check (Tasks 2, 3, 5): in the scratchpad, `./scaffold new <dir>/p1 --api flask --web nextjs --db postgres --cache redis` and `./scaffold new <dir>/p2 --api nestjs --db mongodb`, then `mise run checklist` inside each. Delete both afterwards. Never run `scaffold publish`.

---

### Task 1: Tooling

**Files:**
- Modify: `mise.toml` (`[tools]`, `[tasks.lint]`)
- Modify: `services/**/*.sh`, `tests/helpers/setup.bash`, `tests/**/*.sh` (shellcheck findings only)
- Modify: every shell file (the `shfmt -w` commit)

- [ ] **Step 1:** add `shfmt = "3.14.1"` under `[tools]`, run `mise install` so `mise.lock` records it.
- [ ] **Step 2:** change `[tasks.lint]` to lint every tracked shell file with both tools:
```toml
[tasks.lint]
run = [
"git ls-files -z -- scaffold '*.sh' '*.bash' '*.bats' | xargs -0 -r shellcheck",
"git ls-files -z -- scaffold '*.sh' '*.bash' '*.bats' | xargs -0 -r shfmt -i 2 -ci -d",
]
```
Shrink the task's existing comment to what still holds (the `git ls-files` discovery rationale), or delete it.
- [ ] **Step 3:** run `mise run lint`; expect shellcheck findings in `services/`, `tests/` and `.bats` files. Fix each: real bugs get fixed; intentional ones (e.g. SC2016 single-quoted `$` written into generated code) get `# shellcheck disable=SCxxxx # <reason>` on the narrowest scope. SC1091/SC1090 for sourced paths: prefer a `# shellcheck source=` directive over disable. Commit: `chore(lint): shellcheck every tracked shell file`.
- [ ] **Step 4:** `git ls-files -z -- scaffold '*.sh' '*.bash' '*.bats' | xargs -0 shfmt -i 2 -ci -w`. Verify `git diff -w --stat` is empty or whitespace-only in meaning (e.g. `die() { a; b; }` expanded to multiple lines is fine). Run `mise run test-runner`. Commit ONLY this: `style: shfmt -i 2 -ci every shell file`.
- [ ] **Step 5:** `mise run lint` exits 0; `mise run test-runner` exits 0. Push, open PR, wait for CI, merge with `gh pr merge --rebase --delete-branch` (not squash: squash would fold the pure shfmt commit into the lint fixes).

### Task 2: Core — `scaffold`, `lib/*.sh`, `scripts/*.sh`, `common/install.sh`

**Files:** those 17 files. `common/install.sh` ships to every generated project.

- [ ] **Step 0:** find the shfmt commit on `main` (`git log --format='%H %s' | grep 'style: shfmt'`) and create `.git-blame-ignore-revs` with that hash under a one-line `#` comment naming its subject. A rebase merge rewrites hashes, which is why this waits until Task 1 is on `main`.
- [ ] **Step 1:** file by file, apply the Global Constraints: delete comments that fail the gate; shorten the ones that pass; convert tests/`echo`/`local`/`let`/`expr`; name positional args into locals; fix headers (`scripts/check-provenance.sh` has a banner-style `# Description :` header).
- [ ] **Step 2:** split functions over 20 lines at nameable seams. Candidates by size: `cmd_add` (68), `tui_select` (55), `record_release_age_exceptions` (55), `cmd_wizard` (51), `add_app_service` (49), `apply_adapter` (49), `cmd_update` (47), `cmd_publish` (42), `lint_adapter_env` (40), `_tui_render` (39). List all with `awk 'FNR==1{fn=""} /^[a-z_]+\(\) \{/{fn=$1;s=FNR} fn&&/^\}/{if(FNR-s-1>20)print FNR-s-1, FILENAME, fn;fn=""}' scaffold lib/*.sh scripts/*.sh common/install.sh`.
- [ ] **Step 3:** after each file or two, `mise run test-unit`; commit per file group (`refactor(lib): …`).
- [ ] **Step 4:** `mise run lint`, `mise run test-runner`, real-project check. Push, PR, CI green, `gh pr merge --squash --delete-branch`.

### Task 3: Services — `services/**/*.sh`

- [ ] **Step 1:** same treatment as Task 2 for the 15 driver and shared files. The splice guards (`assert_nest_probe_spliced`, the flask guard in `splice_flask_probe`) keep their logic exactly; their comments may shrink but the reason a guard checks the *absence* of the fallback must survive in one line.
- [ ] **Step 2:** `mise run lint`, `mise run test-runner`, real-project check. PR, CI, squash-merge.

### Task 4: Tests — `tests/**` shell

- [ ] **Step 1:** comments and helpers in `.bats`, `tests/helpers/setup.bash`, fixture `.sh`. Test names (`@test "…"`) are unchanged. Fixture drivers under `tests/fixtures/lint-services/` exist to be *wrong* in one specific way each — keep the defect each fixture is named for.
- [ ] **Step 2:** `mise run lint`, `mise run test-runner`. PR, CI, squash-merge.

### Task 5: Non-shell comments

**Files:** every tracked file outside Tasks 1–4 that carries comments, excluding the do-not-touch list. List with:
`git ls-files | grep -vE '\.(sh|bash|bats|md|lock)$|^docs/|^tests/|vendor/|^scaffold$|^LICENSE$|^UPSTREAM$|^common/\.editorconfig$' | xargs grep -lE '^\s*(#|//|/\*|\*|<!--)'`

- [ ] **Step 1:** apply `comment-code` only (no code changes beyond what makes a deleted comment unnecessary). Comments inside files copied into generated projects (`adapters/`, `common/`) are read by client developers: keep the ones that explain a non-obvious choice they would otherwise undo.
- [ ] **Step 2:** `mise run lint`, `mise run test-runner`, real-project check. PR, CI, squash-merge.
116 changes: 116 additions & 0 deletions docs/superpowers/specs/2026-09-16-clean-shell-and-comments-design.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,116 @@
# Clean Shell and Comments Design

**Goal:** bring every tracked file to the `comment-code` standard and every
shell file to the Clean Bash rules (`~/.dotfiles/agentic/rules/bash.md`), and
lock the shell half in with lint so it does not drift back.

**Scope:** 64 shell files (`scaffold`, `lib/`, `scripts/`, `services/`,
`tests/`, `common/install.sh`) and every non-shell file that carries comments
(`adapters/`, `common/`, `.github/`, root `mise.toml`, `lefthook.yml`). No
behaviour changes. `docs/` prose is out of scope: it is documentation, not
comments.

## What was measured

Run against `main` at `39c78be`:

| Area | Files | Lines | Comment lines |
| --- | --- | --- | --- |
| `scaffold`, `lib/`, `scripts/`, `common/install.sh` | 17 | 4368 | 977 |
| `services/` | 15 | 715 | 261 |
| `tests/` (`.bats`, `.bash`, fixture `.sh`) | 32 | 4542 | 835 |
| non-shell files with comments | 80 | — | 592 |

| Rule | Occurrences |
| --- | --- |
| `[ ]` tests | 553 (`[[ ]]` already: 207) |
| `echo` | 173 |
| functions over 20 lines | 53 (largest: `cmd_add` 68, `tui_select` 55, `record_release_age_exceptions` 55) |
| `local x="$(…)"` | 4 |
| `let` / `expr` | 1 |
| shellcheck on `services/` + `tests/*.bash` (never linted today) | 30 findings: SC2016 ×17, SC2015 ×6, SC1003 ×4, SC2154 ×3 |
| `shfmt -i 2 -ci -d` (shfmt 3.14.1, not yet pinned) | 39 files, ~1800 changed lines |

## Decisions

**Where the repository overrides the rule.** Three rule items are wrong for
this codebase and are not applied:

- *File-level `readonly`.* `lib/*.sh` is sourced again into child processes;
a second `readonly` of the same name is a fatal error. `local -r` stays.
- *`set -euo pipefail` in sourced files.* `lib/*.sh`, `services/**/*.sh` and
`tests/helpers/setup.bash` inherit their options from the entrypoint that
sources them; setting options there silently changes the caller.
Executables (`scaffold`, `scripts/*.sh`, `common/install.sh`) must have it.
- *`[[ ]]` inside bats test bodies.* `[ "$status" -eq 0 ]` is bats' own idiom
and reports the failing expression; the rule applies to helpers and to any
shell outside a `@test` body.

**Byte-identical files are untouched.** Every `verbatim` row in
`docs/PROVENANCE.md` (today `common/.editorconfig`) and everything under
`common/docs/.vitepress/theme/vendor/` is excluded — `scripts/check-provenance.sh`
fails on any drift.

**Functional comments stay.** Anchors (`# @SERVICE_SETUP@`, `# @DB_ENGINE@`,
`# @DB_PROBE@`), toolchain pragmas (`# shellcheck …`, `# noqa`,
`// eslint-disable…`, `@phpstan-…`), `ponytail:` markers, and the `#!` line.
Splice anchors are matched byte-for-byte by awk/sed, so their column and
spelling are fixed.

**Functions are split only at a clear seam.** A function over 20 lines is split
when a contiguous block can be named for what it does. One that is long because
it is a single linear sequence — a large `case`, a heredoc, a TUI render loop —
stays whole, and the ledger records why. Every function name that appears in
`docs/` or `README.md` keeps its name and its contract.

**`[ ]` → `[[ ]]` quotes the right-hand side.** Inside `[[ ]]` an unquoted RHS
of `=`/`==`/`!=` is a glob pattern, so `[ "$a" = $b ]` and `[[ $a = $b ]]`
differ when `$b` contains `*`. Every converted comparison quotes its RHS unless
a pattern is intended. Numeric comparisons move to `(( ))`.

**`printf` replaces `echo` only where a variable is printed.** A literal string
through `echo` has none of the hazards the rule names.

**Lint enforces the shell half.** `mise.toml` pins `shfmt = "3.14.1"`; the
`lint` task runs shellcheck over every tracked shell file (including
`services/` and `tests/`) and `shfmt -i 2 -ci -d` over the same set. The first
`shfmt -w` lands as its own commit containing nothing else, and its hash goes
into `.git-blame-ignore-revs` if the repository has one (it ships one to
generated projects via `common/`; the toolbox itself gains one, in PR 2, once the rebase-merged hash is final).

**Nothing observable changes.** Error messages, output lines, exit codes and
generated-file contents stay identical, because tests assert them. A test that
has to change because the code changed is evidence of a behaviour change: stop
and fix the code, not the test.

## Work items

Five pull requests, in order, each merged green before the next branches from
`main`:

1. **Tooling** — pin shfmt, widen shellcheck, fix the 30 new shellcheck
findings, one pure `shfmt -w` commit.
2. **Core** — `.git-blame-ignore-revs`, then `scaffold`, `lib/*.sh`,
`scripts/*.sh`, `common/install.sh`.
3. **Services** — `services/**/*.sh`.
4. **Tests** — `tests/**` shell (`.bats`, `.bash`, fixture `.sh`).
5. **Non-shell comments** — `adapters/`, `common/`, `.github/`, root
`mise.toml`, `lefthook.yml`, every other tracked non-doc file.

## Verification

Per PR: `mise run lint`; `mise run test-runner` (both lanes); CI green.
PRs 2, 3 and 5 additionally generate two real projects —
`--api flask --web nextjs --db postgres --cache redis` and
`--api nestjs --db mongodb` — and run each project's `mise run checklist`.
All generated projects and scratch files are deleted afterwards; nothing is
published.

## Not doing

| Item | Why |
| --- | --- |
| Rewrite ADRs or `docs/` prose | dated records and documentation, not code comments |
| Move deleted rationale into new ADRs | a comment that fails the gate is deleted; one that passes stays short |
| Change any `verbatim` or `vendor/` file | provenance check requires byte identity |
| Split functions with no nameable seam | single-use helpers that only move lines add indirection |
18 changes: 11 additions & 7 deletions lib/adapter.sh
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,7 @@ load_adapter() {
# adapters/ — `--api ../../../tmp/evil` runs an arbitrary file. Checked before
# the path is built.
case "$name" in
''|*[!a-z0-9-]*|-*) die "not a usable adapter name: ${name} (run: scaffold list)" ;;
'' | *[!a-z0-9-]* | -*) die "not a usable adapter name: ${name} (run: scaffold list)" ;;
esac

local -r dir="${SCAFFOLD_ROOT}/adapters/${name}"
Expand All @@ -49,12 +49,15 @@ load_adapter() {
}

adapter_is_typescript() {
( load_adapter "$1"; [ "${ADAPTER_LANGUAGE:-}" = "typescript" ] )
(
load_adapter "$1"
[ "${ADAPTER_LANGUAGE:-}" = "typescript" ]
)
}

role_path() {
case "$1" in
web|api|app) printf '%s/%s\n' "$APPS_DIR" "$1" ;;
web | api | app) printf '%s/%s\n' "$APPS_DIR" "$1" ;;
*) die "unknown adapter role: ${1}" ;;
esac
}
Expand All @@ -66,7 +69,7 @@ merge_lefthook_fragment() {
[ -f "$fragment" ] || return 0

rendered="$(mktemp)"
sed "s|@APP_ROOT@|${rel}/|g" "$fragment" > "$rendered"
sed "s|@APP_ROOT@|${rel}/|g" "$fragment" >"$rendered"

# Suffix every command with the app it came from: the merge below is key-wise,
# so two apps of the same language — both laravel adapters define `pint` —
Expand Down Expand Up @@ -101,8 +104,8 @@ assert_workspace_filter_name() {
local found expected
found="$(jq -r '.name' "${dest}/package.json")"
expected="$(basename "$dest")"
[ "$found" = "$expected" ] \
|| die "${dest}/package.json is named '${found}', not '${expected}' — Dockerfile.workspace's 'pnpm --filter ${expected}' would match nothing"
[ "$found" = "$expected" ] ||
die "${dest}/package.json is named '${found}', not '${expected}' — Dockerfile.workspace's 'pnpm --filter ${expected}' would match nothing"
}

# Dockerfile.workspace ships @APP_FILTER@ where it needs the app's own directory
Expand Down Expand Up @@ -150,7 +153,8 @@ apply_adapter() {
load_adapter "$name"

local -r dest="${project}/${rel}"
local parent; parent="$(dirname "$dest")"
local parent
parent="$(dirname "$dest")"
mkdir -p "$parent"

# CI=true stays — it lets pnpm replace node_modules with no TTY to confirm on.
Expand Down
Loading