Skip to content

refactor: name things once, and say the rest in code - #17

Merged
ttncode merged 2 commits into
mainfrom
refactor/name-things-once
Sep 12, 2026
Merged

ttncode merged 2 commits into
mainfrom
refactor/name-things-once

Conversation

@ttncode

@ttncode ttncode commented Sep 12, 2026

Copy link
Copy Markdown
Owner

What this changes

Two things, in two commits.

fix(install) β€” a real defect this pass turned up. common/install.sh
decided whether a stack had a migrate service with

docker compose --profile migrate config --services | grep -qx migrate

grep -q exits on its first match and closes the pipe; docker compose then
dies of SIGPIPE, and set -o pipefail reports the whole pipeline as failed.
Measured 6 failures in 40 runs against a real generated stack. The effect on
a client: roughly one install.sh run in seven refuses to migrate with
"a database service exists but no migrate service was found β€” refusing to start
with unapplied schema"
, about a service that is sitting right there. This ships
into every generated project. compose_has_service captures the list first and
matches against it β€” 0 failures in 40 after.

refactor β€” the rest. No behaviour change intended anywhere else.

  • A header block on every script, in one format.
  • Functions grouped by prefix, with section rules between groups.
  • One vocabulary for verbs: assert_ dies on a bad value (9), require_ is an
    environment precondition (3), resolve_ works a value out (5), record_
    writes it down (4), load_ reads a definition into variables (4), print_
    only prints (3), a bare noun is a getter. Twelve functions were renamed to fit
    it, including three whose names said the wrong thing:
    resolve_minimum_release_age β†’ record_release_age_exceptions (it records,
    it never resolved), and the near-identical verify_workspace_filter_name /
    resolve_workspace_filter_name β†’ assert_workspace_filter_name /
    substitute_workspace_filter.
  • Long functions split into named steps: cmd_new 143 β†’ 27 lines, and the same
    for cmd_add, cmd_update, cmd_publish, lint_adapters, lint_services,
    apply_service_drivers. scripts/deploy-check.sh and
    scripts/check-provenance.sh were straight-line scripts; both are now a
    main calling named steps. 126 β†’ 187 functions, longest 143 β†’ 69 lines.
  • Duplicated code pulled into one place: merge_compose_fragment replaces four
    copies of the same yq-merge-or-die block, adapter_env_value four copies of
    the same sed, relax_pnpm_workspace/restore_pnpm_workspace three copies of
    the same three-line sed, resolve_project two copies of a ten-line block.
  • Named constants for hard-coded values: FIRST_APP_PORT/APP_CONTAINER_PORT
    (which removed the magic 8079 and the paragraph explaining it),
    PNPM_RELAXATIONS, PUBLISH_UNSUPPORTED, SHARED_DRIVERS_DIR,
    PasswordPlaceholder, ESC_SEQUENCE_TIMEOUT, and others.
  • Comments cut where the code already said it: 18,505 β†’ 13,166 words (-29%),
    36% β†’ 27% of lines. Fourteen were pure signature echoes and are gone; the rest
    were compressed, not deleted β€” what is left is third-party landmines
    (mise exec trusting a parent config, pnpm turning on frozen lockfiles under
    CI, yq collapsing a document without -P).
  • Three docs pointed at functions that had moved to lib/manifest.sh; fixed.

Not done, deliberately: readonly on the constants. These libraries are
re-sourced into child processes by design, and a second readonly is an error
that set -e turns into a dead script. immich uses readonly in none of its
shell scripts either.

How it was verified

  • mise run lint β€” clean.
  • mise run test-runner β€” both lanes under the runner's own environment.
  • mise exec -- bats tests/provenance.bats β€” 4/4.
  • mise exec -- zizmor --min-severity medium .github/workflows/ β€” no findings.
  • ./scripts/deploy-check.sh nextjs nestjs --db postgres against real Docker β€”
    two images built, both containers healthy, liveness 200, readiness 200,
    migrations applied. Run three times over this branch; the one red run is what
    found the SIGPIPE bug above.
  • The SIGPIPE fix itself: the failing pipeline measured at 6/40, the replacement
    at 0/40, against a generated nextjs+nestjs+postgres project.
  • Every function name before and after was diffed: nothing was lost, twelve were
    renamed, the rest are new splits.

Checklist

  • mise run lint passes
  • mise run test-runner passes
  • New behaviour has a test that fails without the change
  • Docs that describe changed behaviour were updated in the same commit
  • No unrelated changes

iam-truongtrungnghia added 2 commits September 12, 2026 23:40
`docker compose --profile migrate config --services | grep -qx migrate` looks
correct and fails about one run in seven. grep -q exits on its first match and
closes the pipe; docker compose is still writing, dies of SIGPIPE, and this
script's own `set -o pipefail` reports the whole pipeline as failed.

The effect on a client is a stack that refuses to migrate, at random, with
"a database service exists but no migrate service was found β€” refusing to start
with unapplied schema" β€” about a service that is sitting right there. Measured
6 failures in 40 runs against a generated nextjs+nestjs+postgres project; 0 in
40 after.

compose_has_service captures the list first and matches against it, so nothing
closes a pipe early. The test stub writes past the match by more than one pipe
buffer, which makes the old shape fail deterministically.
No behaviour change. Everything below is naming, shape and comments.

Headers: one format on every script, giving the file, what it does and the
author; executables also carry a usage and an example.

Vocabulary: one verb per idea, so a reader can guess. assert_ dies on a bad
value, require_ is an environment precondition, resolve_ works a value out,
record_ writes it down, load_ reads a definition into variables, print_ only
prints, and a bare noun is a getter. Twelve functions were renamed to fit,
three of which said the wrong thing outright:

  resolve_minimum_release_age    -> record_release_age_exceptions
  verify_workspace_filter_name   -> assert_workspace_filter_name
  resolve_workspace_filter_name  -> substitute_workspace_filter

The last two were near-identical names for opposite actions.

Shape: long functions became named steps. cmd_new went from 143 lines to 27,
and cmd_add, cmd_update, cmd_publish, lint_adapters, lint_services and
apply_service_drivers followed. scripts/deploy-check.sh and
scripts/check-provenance.sh were straight-line scripts and are now a main
calling named steps. 126 functions became 187; the longest went from 143 lines
to 69.

Duplication: merge_compose_fragment replaces four copies of the same
yq-merge-or-die block, adapter_env_value four copies of the same sed,
relax_pnpm_workspace/restore_pnpm_workspace three copies of the same three-line
sed, resolve_project two copies of a ten-line block.

Constants: FIRST_APP_PORT and APP_CONTAINER_PORT (which removed the magic 8079
and the paragraph explaining it), PNPM_RELAXATIONS, PUBLISH_UNSUPPORTED,
SHARED_DRIVERS_DIR, PasswordPlaceholder, ESC_SEQUENCE_TIMEOUT and others.

Comments: 18,505 words to 13,166, 36% of lines to 27%. Fourteen were pure
signature echoes and are gone; the rest were compressed. What remains is
third-party landmines β€” mise exec trusting a parent config, pnpm turning on
frozen lockfiles under CI, yq collapsing a document without -P β€” which is why
the density stays above immich's 14%.

Not done: readonly on the constants. These libraries are re-sourced into child
processes by design, and a second readonly is an error that set -e turns into a
dead script.
@ttncode
ttncode merged commit b19dc09 into main Sep 12, 2026
19 checks passed
@ttncode
ttncode deleted the refactor/name-things-once branch September 12, 2026 16:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant