From 7d95c8df0f45f82fdbd8899e8b4b18f13fe6f453 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Oskar=20Sch=C3=B6ldstr=C3=B6m?= Date: Fri, 4 Sep 2026 09:40:38 -0300 Subject: [PATCH 1/3] A worktree configures itself on first start MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `ddev hostshift init` was a step every worktree needed and nobody could guess. The add-on now derives .ddev/.env from a pre-start hook, so the flow is `git worktree add`, install deps, `ddev add-on get`, `ddev start`. Three things decide the shape, and each was the wrong answer first. **In the add-on, not in a file the project commits.** config.hostshift.yaml already exists for a hook alone, carries #ddev-generated, and ships in the same tarball as the command it calls, so the two cannot drift. A committed bootstrap would be the one file `ddev add-on get` can never replace — the exact shape of the two costliest upgrades this add-on has had, made structural and copied into every client repo. **`init --no-restart`, never `env`.** Every refusal that makes init safe is gated on `[ "$cmd" = "init" ]`: the one that stops a worktree writing a map built from its own hostnames when the parent cannot be read, and the note about hostnames inherited from the parent. `env` prints past all of them, and `check` would then agree with the result, because it recomputes the same thing. Measured on the real thing: the hook prints the inherited-hostnames note that `env` would have skipped. **Gated on a linked worktree**, because `.git` is a file there and a directory in the parent. Running this in the parent would write it a map of its own hostnames and take the canonical name the database holds. "not a linked worktree" is a warning, not a refusal, so the gate is load-bearing rather than tidiness. The guard is a HOSTSHIFT_ line rather than the file existing: init writes through a temp file and a rename so a failure leaves nothing, but an empty .ddev/.env from any other source would otherwise skip the derive on every start while the post-start check short-circuits silently on the same file. Two test bugs found on the way, both of which had made assertions stop asserting: - the tag loop compared each tag's command against the working tree's and read "differs" as "old". That was a proxy for the property — whether the command carries #ddev-generated, which is what DDEV actually tests — and it broke the moment a third tag existed. v0.2.1 and v0.2.2 differ from HEAD and carry the marker, so they neither warn nor should. - the hook extraction took the first exec-host in the file. Adding a pre-start block above post-start made it test the wrong hook rather than fail. --- ddev/commands/host/hostshift | 11 +++++ ddev/config.hostshift.yaml | 33 +++++++++++++ test/addon-command.sh | 93 +++++++++++++++++++++++++++++++----- 3 files changed, 126 insertions(+), 11 deletions(-) diff --git a/ddev/commands/host/hostshift b/ddev/commands/host/hostshift index f6234d7..c900976 100755 --- a/ddev/commands/host/hostshift +++ b/ddev/commands/host/hostshift @@ -58,6 +58,7 @@ usage: ddev hostshift [flags] --slug NAME name this worktree's hostnames; recorded, and it sticks --slug-from-branch forget a recorded slug and follow the branch again --dry-run, -n say what would happen, write nothing + --no-restart init: write the file and stop, for a pre-start hook --force copy-db: overwrite a database that has tables --quiet, -q say nothing when there is nothing to check USAGE @@ -77,6 +78,7 @@ slug_explicit="" forget_slug="" branch="" dry_run="" +no_restart="" force="" quiet="" claimed="" @@ -96,6 +98,7 @@ while [ $# -gt 0 ]; do # naming is to hand-edit .ddev/.env. --slug-from-branch) forget_slug=1; shift ;; --dry-run|-n) dry_run=1; shift ;; + --no-restart) no_restart=1; shift ;; --quiet|-q) quiet=1; shift ;; --force) force=1; shift ;; # Silently discarding an unknown flag is how `--dry-run` came to write the @@ -2896,6 +2899,14 @@ hostshift map --slug "$slug" "${map_args[@]+"${map_args[@]}"}" >&2 # Only when actually invoked through `ddev hostshift` — DDEV sets DDEV_APPROOT # for a host command, and nothing else does. Run directly, from a test or by # hand, init stays what it says on the tin: it writes a file. +# --no-restart is for the pre-start hook, which is already inside the start it +# would otherwise ask for: restarting there either deadlocks or loops. Everything +# above this line has run, so the refusals and the warnings are the same ones a +# hand-run init prints. +if [ -n "$no_restart" ]; then + exit 0 +fi + if [ -n "${DDEV_APPROOT:-}" ] && command -v ddev >/dev/null; then echo "hostshift: restarting to pick it up" >&2 # Not `exec`: that hands the exit status to `ddev restart`, and DDEV does not diff --git a/ddev/config.hostshift.yaml b/ddev/config.hostshift.yaml index 6a9a870..3b33176 100644 --- a/ddev/config.hostshift.yaml +++ b/ddev/config.hostshift.yaml @@ -67,6 +67,39 @@ # following DDEV's own advice arms the trap on a project that was fine. Nagging # on every start until the command is replaced is the proportionate answer. hooks: + # A worktree configures itself on its first start, so `ddev hostshift init` is + # not a step anyone has to know about. It ships here rather than in a file the + # project commits, for the reason the rest of this file is about: this one + # carries #ddev-generated and travels in the same tarball as the command it + # calls, so the two cannot drift. A committed bootstrap would be the one file + # `ddev add-on get` can never replace — which is exactly the shape of the two + # costliest upgrades this add-on has had. + # + # `init --no-restart`, never `env`. Every refusal that makes init safe is gated + # on the subcommand being init: the one that stops a worktree writing a map + # built from its own hostnames when the parent checkout cannot be read, and the + # note about hostnames inherited from the parent. `env` prints past all of + # them, and `check` would then agree with the result, because it recomputes the + # same thing. + # + # Only in a linked worktree. `.git` is a file there and a directory in the + # parent, and that is the distinction the whole design rests on — running this + # in the parent would write it a map of its own hostnames and take the + # canonical name the database holds. + # + # The guard is a HOSTSHIFT_ line, not the file existing. init writes through a + # temp file and a rename, so a failure leaves no file at all — but an operator + # who ran `: > .ddev/.env`, or any future writer that is not init, leaves an + # empty one, and `[ -f ]` would then skip the derive on every start while the + # post-start check short-circuits silently on the same file. + pre-start: + - exec-host: | + if [ -f .git ] && grep -q '/worktrees/' .git 2>/dev/null; then + grep -q '^HOSTSHIFT_' .ddev/.env 2>/dev/null || { + .ddev/commands/host/hostshift init --no-restart || true + } + fi + post-start: - exec-host: | head -5 .ddev/commands/host/hostshift 2>/dev/null | grep -q '#ddev-generated' || { diff --git a/test/addon-command.sh b/test/addon-command.sh index c5fe77f..19e3586 100755 --- a/test/addon-command.sh +++ b/test/addon-command.sh @@ -460,8 +460,14 @@ check "init is idempotent" "$first" "$(cat "$wt/.ddev/.env")" # against every tag there is — a stub only proves what its author remembered. # The hook is a YAML block scalar now, so take the whole indented body rather # than one line after the key. +# The post-start block specifically. This used to take the first exec-host in +# the file, which was the same thing until a pre-start hook was added above it — +# and then this silently tested the wrong hook, which is a test that asserts +# nothing rather than one that fails. hook="$(awk ' - /^ *- *exec-host: *\|/ { inblock = 1; next } + /^ *post-start:/ { inpost = 1; next } + inpost && /^ *[a-z-]+:/ && !/^ *- / { exit } + inpost && /^ *- *exec-host: *\|/ { inblock = 1; next } inblock && /^ / { sub(/^ /, ""); print; next } inblock { exit } ' "$repo/ddev/config.hostshift.yaml")" @@ -488,18 +494,21 @@ for tag in $(cd "$repo" && git tag -l 'v*'); do # one line in an `add-on get` output is not enough, and DDEV itself tells # developers to strip the marker that keeps the command replaceable. # - # Only for a tag that is actually older. The loop takes every tag there is, - # and the newest one is usually the command in the working tree byte for byte - # — cutting a release is what makes that true. Such a copy is not stale and - # must not warn, so asserting the warning against it fails the moment a - # release is tagged, which is exactly when this suite most needs to be green. - # The discriminant is the bytes, not the version: a tag whose command differs - # from the current one is an old command, whatever it is called. - if cmp -s "$old_cmd" "$repo/ddev/commands/host/hostshift"; then + # The warning fires on one thing and one thing only: whether the command on + # disk carries #ddev-generated, because that is what DDEV tests before it will + # replace a file. So that is what decides which assertion applies. + # + # An earlier version of this compared the tag's command against the working + # tree's and treated "differs" as "old". That held while the only tags were + # v0.1.0 and the release the tree was on, and broke the moment a third existed: + # v0.2.1 and v0.2.2 differ from HEAD *and* carry the marker, so they neither + # warn nor should. Comparing bytes was a proxy for the property; this asks the + # property. + if head -5 "$old_cmd" | grep -q '#ddev-generated'; then case "$out" in *"predates this add-on"*) - fail "$tag's command is the current one and must not call itself stale" "$out" ;; - *) pass "$tag's command is the current one and does not call itself stale" ;; + fail "$tag's command carries the marker and must not call itself stale" "$out" ;; + *) pass "$tag's command carries the marker and does not call itself stale" ;; esac continue fi @@ -2828,6 +2837,68 @@ contains "and says the env is written and the restart is what failed" \ "the restart is what failed" "$out" rm -rf "$work/failddev" +echo "== --no-restart, which is what the pre-start hook runs" + +# The hook is inside the start it would otherwise ask for, so init must be able +# to write the file and stop. Everything above the restart still runs, which is +# the whole reason the hook calls `init --no-restart` rather than `env`: every +# refusal that makes init safe is gated on the subcommand being init, and `env` +# prints past all of them. +rm -f "$exw/.ddev/.env" +(cd "$exw" && DDEV_APPROOT="$exw" PATH="$work/failddev:$PATH" \ + "$cmd" init --no-restart --slug wt-x >/dev/null 2>&1) && rc=0 || rc=$? +[ "$rc" = 0 ] && pass "--no-restart exits 0 without restarting" \ + || fail "--no-restart exits 0 without restarting" "exit $rc" +if grep -q '^HOSTSHIFT_VARIANTS=' "$exw/.ddev/.env" 2>/dev/null; then + pass "--no-restart still writes the file" +else + fail "--no-restart still writes the file" "no HOSTSHIFT_VARIANTS" +fi + +# The refusal that stops a worktree writing a map of its own hostnames is +# `[ "$cmd" = "init" ]`-gated. If --no-restart ever became its own subcommand, +# or the hook were switched back to `env`, this is what would go quiet. +rm -f "$exw/.ddev/.env" +mv "$ex" "$ex.moved" +out="$(cd "$exw" && "$cmd" init --no-restart --slug wt-x 2>&1 || true)" +mv "$ex.moved" "$ex" +contains "--no-restart keeps init's refusals" "refusing to write a map" "$out" +if [ -e "$exw/.ddev/.env" ]; then + fail "a refused --no-restart writes nothing" "the file exists" +else + pass "a refused --no-restart writes nothing" +fi + +echo "== the pre-start hook only fires in a linked worktree" + +# `.git` is a file in a linked worktree and a directory in the parent. Running +# the hook in the parent would write it a map of its own hostnames and take the +# canonical name the database holds, so the gate is load-bearing rather than +# tidiness. +gate() { + [ -f "$1/.git" ] && grep -q '/worktrees/' "$1/.git" 2>/dev/null +} +gate "$exw" && pass "the gate fires in a worktree" \ + || fail "the gate fires in a worktree" "$exw" +gate "$ex" && fail "the gate must not fire in the parent" "$ex" \ + || pass "the gate does not fire in the parent" + +# And the hook the add-on actually ships has to be the one just tested. +hook="$(awk '/pre-start:/{f=1} f&&/exec-host/{g=1} g&&/^$/{exit} g' \ + "$repo/ddev/config.hostshift.yaml")" +case "$hook" in + *"init --no-restart"*) pass "the shipped hook calls init --no-restart" ;; + *) fail "the shipped hook calls init --no-restart" "$hook" ;; +esac +case "$hook" in + *"/worktrees/"*) pass "the shipped hook gates on a linked worktree" ;; + *) fail "the shipped hook gates on a linked worktree" "$hook" ;; +esac +case "$hook" in + *"grep -q '^HOSTSHIFT_'"*) pass "the shipped hook guards on a HOSTSHIFT_ line, not the file" ;; + *) fail "the shipped hook guards on a HOSTSHIFT_ line, not the file" "$hook" ;; +esac + echo "== a proxy flag in HOSTSHIFT_ARGS survives check and init" # --max-body, --strict-origins, --compress and --no-sweep are real proxy flags From 6f400317551304f6d70cdd7f074d81d368fd3e78 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Oskar=20Sch=C3=B6ldstr=C3=B6m?= Date: Fri, 4 Sep 2026 10:14:49 -0300 Subject: [PATCH 2/3] Address the audit: report failures, stop repeating a fixed bug, run the hook MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Six defects, and two claims in the previous commit message were wrong. **The inherited-hostnames note is back off.** :1252 records why it was taken off `check`: the post-start hook printed "until `ddev restart`, those serve this worktree" from inside the very restart that handed them back. The pre-start hook is that hook again, and on a first-ever start nothing was ever serving them. The previous commit offered this as *evidence the design was right*. Gated on --no-restart; every refusal still runs. **`|| true` is gone.** DDEV prints `Task failed:` on a non-zero hook and only aborts under fail_on_hook_fail, so swallowing the status swallowed the only marker. And when the derive refuses there is no .ddev/.env, so the post-start check exits 0 at :148 without a word — the refusal scrolled past mid-start and nothing downstream reported it. Now it says so and still exits 0, because a project running fail_on_hook_fail must be able to start when init legitimately refuses. **The exit status is a real loss, now stated.** --no-restart skips the block where init re-runs check and answers with it — added because "an agent that checks $? was told everything was fine". `ddev start` exits 0 whatever check finds. The README says to run `ddev hostshift check` when a status is wanted. **The drift claim was too strong.** "Cannot drift" is false in the one case this file documents at length: DDEV will not replace a pre-marker command, so such a worktree runs this hook against a parser that exits 2 on --no-restart. The HOSTSHIFT_ guard covers every *configured* project, since v0.1.0 wrote those lines too. **The temp file could outrank the file it replaces.** `mktemp .ddev/.env.XXXXXX` matches DDEV's EnvFiles() glob, and DDEV passes each match as a *later* --env-file. The EXIT trap normally clears it; init now runs inside `ddev start`, where an interrupted start is routine. Renamed so it dodges the glob and falls under the existing ignore rule. **The hook is executed by the tests now, not grepped.** Three greps agreeing with a string nobody ran is exactly how the post-start extraction came to select the wrong block and still pass. Five cases: derives when absent, derives when *empty* — the whole reason the guard is a HOSTSHIFT_ line — no-ops when configured, writes nothing in the parent, and the temp file stays outside DDEV's glob. Two corrections to my own account: - "silently tested the wrong hook … asserts nothing rather than fails" — it fails loudly; the v0.1.0 iteration goes red. - "v0.2.1 and v0.2.2 differ from HEAD" — all four v0.2.x commands are byte-identical to master. What breaks the old assertion is this branch editing the command, i.e. the ordinary state between releases. The fix was still needed; the reason given for it was not. README, the flag table, both worked examples and install.yaml's closing line all said `ddev hostshift init` was required per worktree. It is not any more. --- README.md | 32 +++++++++++------ ddev/commands/host/hostshift | 17 +++++++-- ddev/config.hostshift.yaml | 22 +++++++++--- ddev/install.yaml | 7 ++-- test/addon-command.sh | 68 ++++++++++++++++++++++++++++++++++++ 5 files changed, 127 insertions(+), 19 deletions(-) diff --git a/README.md b/README.md index d564222..bc58274 100644 --- a/README.md +++ b/README.md @@ -111,15 +111,19 @@ twice. A local clone still works if you are developing the add-on itself: `ddev add-on get ~/src/hostshift/ddev`. -Then, in the worktree, one command: +Then `ddev start`. A worktree configures itself: a pre-start hook derives +`.ddev/.env` before compose reads it, so there is nothing to run and nothing to +remember. It writes exactly one file and prints the map it resolved. -``` -ddev hostshift init -``` +`ddev hostshift init` still exists and is still the way to *change* that — +`--slug`, or re-deriving after a branch rename. It writes the same file and then +restarts to pick it up. `--no-restart` writes and stops, which is what the hook +runs. -It writes exactly one file, `.ddev/.env`, and nothing else — then restarts the -project to pick it up, and prints the URLs it is serving. That is the whole -required path. +The hook fires only in a linked worktree, and only when `.ddev/.env` carries no +`HOSTSHIFT_` line, so it is a no-op on every later start. One thing it cannot do +that `init` can: set the exit status. `ddev start` exits 0 whatever `check` +finds, so a script that wants a status must run `ddev hostshift check` itself. ### Worked example @@ -131,15 +135,18 @@ DDEV project of its own with nothing configured. $ git worktree add ../acme-wt-a -b wt-a $ cd ../acme-wt-a $ ddev add-on get https://github.com/generoi/hostshift/releases/latest/download/hostshift-ddev.tar.gz -$ ddev hostshift init +$ ddev start hostshift: slug "wt-a", from the git branch wt-a hostshift: canonical hostnames from /Users/you/Projects/acme, the checkout this was made from hostshift: wrote .ddev/.env map from --from/--to site1 https://acme.ddev.site -> https://wt-a--acme.ddev.site -hostshift: restarting to pick it up ``` +The hostshift lines come from a pre-start hook, which runs before compose reads +`.ddev/.env` — so the project comes up already serving the variant. There is no +second start. + Nothing was committed and no map was declared. The hostnames the database holds are the **parent checkout's** — whatever pulled it search-replaced to `acme.ddev.site`, not to the worktree's hostname — so the command reads them @@ -261,7 +268,7 @@ $ git worktree add ../acme-wt-a -b wt-a $ cd ../acme-wt-a $ printf '#ddev-silent-no-warn\nname: acme-wt-a\n' > .ddev/config.hostshift-name.yaml $ ddev add-on get https://github.com/generoi/hostshift/releases/latest/download/hostshift-ddev.tar.gz -$ ddev hostshift init +$ ddev start ``` The name only has to be unique; hostshift derives the preview hostnames from the @@ -450,7 +457,10 @@ hostshift diff crawl a site two ways and compare, to verify a deployment ``` ``` -ddev hostshift init write .ddev/.env (required, per worktree) +ddev hostshift init write .ddev/.env (automatic on start; run it + to change --slug, or after a + branch rename) + --no-restart write it and stop (what the pre-start hook runs) ddev hostshift check is the deployed map still current (also the post-start hook) ddev hostshift copy-db copy the parent checkout's database into this worktree (refuses to overwrite a non-empty one without --force) diff --git a/ddev/commands/host/hostshift b/ddev/commands/host/hostshift index c900976..9a0ba22 100755 --- a/ddev/commands/host/hostshift +++ b/ddev/commands/host/hostshift @@ -1287,7 +1287,13 @@ fi # `loopback` printed it on an already-restarted, entirely healthy worktree, # telling the developer their worktree was stealing a hostname it was not and # pointing them at remedial action they should not take. -if [ -n "$parent_hosts" ] && [ "$cmd" = "init" ]; then +# ...and not from the pre-start hook either, for the same reason one step further +# on: the hook *is* a start. It would print "until `ddev restart`, those serve +# this worktree" from inside the restart that hands them back, and on a +# first-ever start nothing was ever serving them. The refusals below still run +# under --no-restart; this note is the one thing that does not, because its +# remedy is the very command that is already happening. +if [ -n "$parent_hosts" ] && [ "$cmd" = "init" ] && [ -z "$no_restart" ]; then shared="" while IFS= read -r h; do [ -n "$h" ] || continue @@ -2877,7 +2883,14 @@ touch .ddev/.env # In .ddev/ so the mv is a rename on one filesystem rather than a copy+unlink # across two, which is not atomic — a concurrent compose read must never catch # this half-written and see an empty VIRTUAL_HOST. -tmp="$(mktemp .ddev/.env.XXXXXX)" +# Not `.ddev/.env.XXXXXX`: DDEV globs `.ddev/.env.*` in EnvFiles() and passes +# each one to compose as a *later* `--env-file`, so a leftover temp would +# override the real file rather than be ignored — and the exclude block names +# `.ddev/.env` exactly, so it would show up in `git status` too. This spelling +# dodges the glob and falls inside the `.ddev/**/*hostshift*` rule. The EXIT trap +# still removes it; the point is that a start killed mid-write leaves nothing +# that can outrank the file it was writing. +tmp="$(mktemp .ddev/.hostshift-env.XXXXXX)" trap 'rm -f "$tmp"' EXIT # cp -p first, then truncate: that gives the temp file the mode the real one # already had, and redirection keeps it. .ddev/.env is where DDEV documents diff --git a/ddev/config.hostshift.yaml b/ddev/config.hostshift.yaml index 3b33176..8913520 100644 --- a/ddev/config.hostshift.yaml +++ b/ddev/config.hostshift.yaml @@ -71,9 +71,13 @@ hooks: # not a step anyone has to know about. It ships here rather than in a file the # project commits, for the reason the rest of this file is about: this one # carries #ddev-generated and travels in the same tarball as the command it - # calls, so the two cannot drift. A committed bootstrap would be the one file - # `ddev add-on get` can never replace — which is exactly the shape of the two - # costliest upgrades this add-on has had. + # calls, so they upgrade together in the ordinary case. Not "cannot drift": + # DDEV will not replace the *command* on a project whose copy predates the + # marker, so a pre-marker worktree that was never configured runs this hook + # against a parser that exits 2 on `--no-restart`. The HOSTSHIFT_ guard covers + # every such project that was configured, because v0.1.0 wrote HOSTSHIFT_ lines + # too. A committed bootstrap would still be worse: it would be the one file + # `ddev add-on get` can never replace, in every client repo at once. # # `init --no-restart`, never `env`. Every refusal that makes init safe is gated # on the subcommand being init: the one that stops a worktree writing a map @@ -96,7 +100,17 @@ hooks: - exec-host: | if [ -f .git ] && grep -q '/worktrees/' .git 2>/dev/null; then grep -q '^HOSTSHIFT_' .ddev/.env 2>/dev/null || { - .ddev/commands/host/hostshift init --no-restart || true + .ddev/commands/host/hostshift init --no-restart || { + # Not `|| true`. DDEV prints `Task failed:` on a non-zero hook and + # only aborts under fail_on_hook_fail, so swallowing the status + # swallowed the one marker that said anything went wrong — and + # when the derive refuses there is no .ddev/.env, so the + # post-start check exits 0 without a word. Exit 0 deliberately: a + # project running fail_on_hook_fail must still be able to start + # when init legitimately refuses. + echo "hostshift: could not configure this worktree." >&2 + echo " \`ddev hostshift init\` on its own says why." >&2 + } } fi diff --git a/ddev/install.yaml b/ddev/install.yaml index 18c71d2..17cdca3 100644 --- a/ddev/install.yaml +++ b/ddev/install.yaml @@ -146,9 +146,12 @@ post_install_actions: # writes into .ddev/, and an add-on that edits a project's configuration # without being asked is how a surprise gets installed. echo - echo "hostshift installed. One step remains:" + echo "hostshift installed. Nothing else to run:" echo - echo " ddev hostshift init" + echo " ddev restart" + echo + echo "A worktree derives .ddev/.env on start. \`ddev hostshift init\` is still" + echo "how you change it — --slug, or re-deriving after a branch rename." echo echo "It works out the slug from the git branch and merges the HOSTSHIFT_*" echo "variables into .ddev/.env — the only file it writes. Then" diff --git a/test/addon-command.sh b/test/addon-command.sh index 19e3586..17a040c 100755 --- a/test/addon-command.sh +++ b/test/addon-command.sh @@ -2899,6 +2899,74 @@ case "$hook" in *) fail "the shipped hook guards on a HOSTSHIFT_ line, not the file" "$hook" ;; esac +# And run it. Asserting on the hook's *text* is how the post-start extraction +# came to select the wrong block and still pass: three greps agreed with a string +# nobody executed. DDEV runs a pre-start hook through `bash -c` from the approot, +# the same way test 486 runs the post-start one. +prehook="$(awk ' + /^ *pre-start:/ { inpre = 1; next } + inpre && /^ *[a-z-]+:/ && !/^ *- / { exit } + inpre && /^ *- *exec-host: *\|/ { inblock = 1; next } + inblock && /^ / { sub(/^ /, ""); print; next } + inblock { exit } +' "$repo/ddev/config.hostshift.yaml")" +[ -n "$prehook" ] || fail "the pre-start hook body could not be read" "" + +# The hook invokes `.ddev/commands/host/hostshift`, which is where `ddev add-on +# get` puts it. The harness runs the repo copy from elsewhere, so put one there. +mkdir -p "$exw/.ddev/commands/host" "$ex/.ddev/commands/host" +cp "$cmd" "$exw/.ddev/commands/host/hostshift" +cp "$cmd" "$ex/.ddev/commands/host/hostshift" +chmod +x "$exw/.ddev/commands/host/hostshift" "$ex/.ddev/commands/host/hostshift" + +rm -f "$exw/.ddev/.env" +(cd "$exw" && bash -c "$prehook") >/dev/null 2>&1 || true +if grep -q '^HOSTSHIFT_VARIANTS=' "$exw/.ddev/.env" 2>/dev/null; then + pass "the pre-start hook derives .ddev/.env when there is none" +else + fail "the pre-start hook derives .ddev/.env when there is none" "no HOSTSHIFT_VARIANTS" +fi + +# The empty file is the whole reason the guard is a HOSTSHIFT_ line rather than +# `[ -f ]`, and nothing exercised it. An operator's `: > .ddev/.env`, or any +# writer that is not init, must not disable the derive for every later start. +: > "$exw/.ddev/.env" +(cd "$exw" && bash -c "$prehook") >/dev/null 2>&1 || true +if grep -q '^HOSTSHIFT_VARIANTS=' "$exw/.ddev/.env" 2>/dev/null; then + pass "an empty .ddev/.env is still derived, not skipped" +else + fail "an empty .ddev/.env is still derived, not skipped" "left empty" +fi + +# Configured already: the hook must be a no-op, or every start re-runs the +# collision scan and re-derives hostnames after a branch switch. +before="$(cat "$exw/.ddev/.env")" +(cd "$exw" && bash -c "$prehook") >/dev/null 2>&1 || true +if [ "$before" = "$(cat "$exw/.ddev/.env")" ]; then + pass "the pre-start hook is a no-op once configured" +else + fail "the pre-start hook is a no-op once configured" "the file changed" +fi + +# In the parent it must do nothing at all. This is the failure the gate exists +# for: a self-map written to the one checkout whose canonical hostname the +# database holds. +rm -f "$ex/.ddev/.env" +(cd "$ex" && bash -c "$prehook") >/dev/null 2>&1 || true +if [ -e "$ex/.ddev/.env" ]; then + fail "the pre-start hook writes nothing in the parent" "it wrote .ddev/.env" +else + pass "the pre-start hook writes nothing in the parent" +fi + +# The temp file init writes must not be one DDEV will later hand to compose. +# EnvFiles() globs .ddev/.env.* and passes each as a *later* --env-file, so a +# leftover would outrank the file it was written to replace. +case "$(grep -m1 'mktemp .ddev/' "$repo/ddev/commands/host/hostshift")" in + *".ddev/.env."*) fail "the env temp file is outside DDEV's .env.* glob" "matches .env.*" ;; + *) pass "the env temp file is outside DDEV's .env.* glob" ;; +esac + echo "== a proxy flag in HOSTSHIFT_ARGS survives check and init" # --max-body, --strict-origins, --compress and --no-sweep are real proxy flags From a8bd7cfecdaa969c6ec1904270c39d64386fb545 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Oskar=20Sch=C3=B6ldstr=C3=B6m?= Date: Fri, 4 Sep 2026 10:43:06 -0300 Subject: [PATCH 3/3] Second audit: two tests that could not fail, one that could run ddev MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Eleven findings. The two that mattered are both in the tests I added to prove the last round of fixes. **A deleted stub left on PATH.** `rm -rf "$work/failddev"` ran *above* the --no-restart block that prepends it, so `command -v ddev` fell through to the real binary. On CI, with no ddev installed, the assertion passed whether or not the flag did anything; on a developer machine a regression would have run an actual `ddev restart` from a suite the Makefile calls "no Docker and no DDEV needed". Moved below. **The leftover-temp-file glob no longer matched the template.** Renaming to `.hostshift-env.XXXXXX` left `ls .ddev/.env.??????` matching nothing, so "a failed init leaves no temp file behind" became true however the code behaved — the EXIT trap could have been deleted. It now globs both. Two more tests passed for the wrong reason: - "the pre-start hook is a no-op once configured" compared .ddev/.env byte for byte, which holds with the guard removed, because init is idempotent anyway. It asserts no *output* now: the cost of running is a collision scan and a docker inspect on every start. - "the env temp file is outside DDEV's glob" grepped the source, which fails open — no match lands in the catch-all arm — and `mktemp -p .ddev .env.XXXXXX` would reintroduce the bug while defeating the pattern. It runs init against an unwritable file and globs the directory instead. **The hook conflated unreadable with unconfigured.** `grep -q … 2>/dev/null` on `.ddev/.env` is the thing the command rejects `grep -qs` for by name (:143). A chmod-000 file made the hook re-derive a configured project and print a raw `sed: Permission denied` ahead of `check`'s precise diagnosis. **A worktree without the CLI nagged on every start, permanently** — the guard never becomes satisfied, and install.yaml declares the CLI optional. That one failure is silent now; `check` still reports it. **The note gate keyed on the wrong thing.** `-z "$no_restart"` suppressed the inherited-hostnames note for `init --no-restart` run by hand, where nothing restarts and the note is *true*. HOSTSHIFT_HOOK is the property — "I am inside a start" — and the post-start hook already uses it. **The feature had no integration coverage.** Every integration case ran `ddev hostshift init` before the worktree's first start, making the hook a no-op in all of them. The premise — that pre-start lands before compose reads the file — is a claim about DDEV's Start() ordering that only a real `ddev start` can test. One case now starts unconfigured and asserts .ddev/.env appears. Three doc contradictions: README:177 still said "after `ddev restart`" thirty lines after saying there is no second start; README:194, the only place the ddev/ddev#5486 hazard is explained, still credited the note to `init`; and install.yaml's closing message was wrong for a non-worktree install, where the hook does not fire and init is still required. --- README.md | 10 ++++--- ddev/commands/host/hostshift | 12 ++++++--- ddev/config.hostshift.yaml | 21 ++++++++++++--- ddev/install.yaml | 21 ++++++++++----- test/addon-command.sh | 51 +++++++++++++++++++++++++----------- test/integration-ddev.sh | 15 ++++++++++- 6 files changed, 96 insertions(+), 34 deletions(-) diff --git a/README.md b/README.md index bc58274..369ecd9 100644 --- a/README.md +++ b/README.md @@ -174,8 +174,8 @@ You do not need to gitignore it. Installing the add-on adds its files to the ignore travels with the machine rather than with the branch. Removing the add-on takes the entry back out. -After `ddev restart`, `https://wt-a--acme.ddev.site` serves the worktree and -`https://acme.ddev.site` goes on serving the parent. +That first `ddev start` is the only one needed: `https://wt-a--acme.ddev.site` +serves the worktree and `https://acme.ddev.site` goes on serving the parent. ### What happens by itself @@ -191,8 +191,10 @@ After `ddev restart`, `https://wt-a--acme.ddev.site` serves the worktree and to the worktree's own hostnames. DDEV derives `name` from the directory but not `additional_hostnames`, so a worktree inherits the parent's extra hostnames verbatim and — traefik breaking the tie by rule length — silently - wins them from its first `ddev start` until the next restart. `init` says so - when it sees the overlap. Upstream: [ddev/ddev#5486][]. + wins them from its first `ddev start` until the next restart. `check` says so + on every start while the overlap lasts — the pre-start hook deliberately does + not, because its remedy is "run `ddev restart`" and the hook is inside one. + Upstream: [ddev/ddev#5486][]. - **Staleness.** A `post-start` hook runs `ddev hostshift check` on every `ddev start`, prints what is being served, and says so when `.ddev/.env` no longer matches what the project resolves to — which happens on its own, since diff --git a/ddev/commands/host/hostshift b/ddev/commands/host/hostshift index 9a0ba22..e9cee5a 100755 --- a/ddev/commands/host/hostshift +++ b/ddev/commands/host/hostshift @@ -1290,10 +1290,14 @@ fi # ...and not from the pre-start hook either, for the same reason one step further # on: the hook *is* a start. It would print "until `ddev restart`, those serve # this worktree" from inside the restart that hands them back, and on a -# first-ever start nothing was ever serving them. The refusals below still run -# under --no-restart; this note is the one thing that does not, because its -# remedy is the very command that is already happening. -if [ -n "$parent_hosts" ] && [ "$cmd" = "init" ] && [ -z "$no_restart" ]; then +# first-ever start nothing was ever serving them. +# +# HOSTSHIFT_HOOK, not `-z "$no_restart"`. The property is "I am inside a start", +# and --no-restart is a public flag: run by hand it restarts nothing, so the +# hostnames really do keep serving this worktree and the note is true. Keying on +# the flag suppressed it for the one caller it is correct for. The post-start +# hook already uses this variable for the same distinction. +if [ -n "$parent_hosts" ] && [ "$cmd" = "init" ] && [ -z "${HOSTSHIFT_HOOK:-}" ]; then shared="" while IFS= read -r h; do [ -n "$h" ] || continue diff --git a/ddev/config.hostshift.yaml b/ddev/config.hostshift.yaml index 8913520..a5baf0a 100644 --- a/ddev/config.hostshift.yaml +++ b/ddev/config.hostshift.yaml @@ -99,8 +99,23 @@ hooks: pre-start: - exec-host: | if [ -f .git ] && grep -q '/worktrees/' .git 2>/dev/null; then - grep -q '^HOSTSHIFT_' .ddev/.env 2>/dev/null || { - .ddev/commands/host/hostshift init --no-restart || { + # `-r`, not `2>/dev/null` on the grep. Suppressing the error conflates + # "no HOSTSHIFT_ line" with "cannot read the file" — the exact + # distinction the command rejects `grep -qs` for. Unreadable means + # already-configured-but-broken, and `check` says so precisely a moment + # later; deriving over the top of it prints a raw `sed: Permission + # denied` first and re-derives a project that needs no deriving. + if [ -r .ddev/.env ] && grep -q '^HOSTSHIFT_' .ddev/.env; then + : + elif [ -e .ddev/.env ] && [ ! -r .ddev/.env ]; then + : + else + # The CLI is optional (install.yaml), and without it init cannot + # derive anything. Saying so once per start forever is noise on a + # project that may never want the CLI, so this is the one failure + # that stays quiet — `check` reports it a moment later either way. + command -v hostshift >/dev/null 2>&1 || exit 0 + HOSTSHIFT_HOOK=1 .ddev/commands/host/hostshift init --no-restart || { # Not `|| true`. DDEV prints `Task failed:` on a non-zero hook and # only aborts under fail_on_hook_fail, so swallowing the status # swallowed the one marker that said anything went wrong — and @@ -111,7 +126,7 @@ hooks: echo "hostshift: could not configure this worktree." >&2 echo " \`ddev hostshift init\` on its own says why." >&2 } - } + fi fi post-start: diff --git a/ddev/install.yaml b/ddev/install.yaml index 17cdca3..a3e35fa 100644 --- a/ddev/install.yaml +++ b/ddev/install.yaml @@ -146,17 +146,24 @@ post_install_actions: # writes into .ddev/, and an add-on that edits a project's configuration # without being asked is how a surprise gets installed. echo - echo "hostshift installed. Nothing else to run:" + echo "hostshift installed." + echo + echo "In a git worktree, run:" echo echo " ddev restart" echo - echo "A worktree derives .ddev/.env on start. \`ddev hostshift init\` is still" - echo "how you change it — --slug, or re-deriving after a branch rename." + echo "A pre-start hook works the slug out from the git branch and writes the" + echo "HOSTSHIFT_* variables into .ddev/.env — the only file it writes —" + echo "before compose reads it, so one start is enough." + echo + echo "Anywhere else — the parent checkout, or a project mapping production" + echo "hostnames — the hook does not fire and there is one step:" + echo + echo " ddev hostshift init" echo - echo "It works out the slug from the git branch and merges the HOSTSHIFT_*" - echo "variables into .ddev/.env — the only file it writes. Then" - echo "'ddev restart', which prints the URLs it is serving. Pass --dry-run" - echo "first to see what it would write, or --slug to name it yourself." + echo "That is also how you change a worktree's map afterwards: --slug to name" + echo "it yourself, --slug-from-branch after a rename, --dry-run to see what" + echo "it would write." echo echo "Two things it cannot do for you:" echo diff --git a/test/addon-command.sh b/test/addon-command.sh index 17a040c..da672df 100755 --- a/test/addon-command.sh +++ b/test/addon-command.sh @@ -587,14 +587,16 @@ check ".env keeps the mode it had" "-rw-------" \ # The write goes through a temp file in .ddev/ so the mv is an atomic rename. # That temp file is only safe because of the EXIT trap: without it, every failed -# init leaves a `.ddev/.env.XXXXXX` behind — a file DDEV's own .ddev/.gitignore -# does not cover, so it shows up as an untracked file in the developer's repo, -# holding whatever credentials .env held. chmod 000 makes the `cp -p` fail, -# which is the failure path. +# init leaves one behind, holding whatever credentials .env held. chmod 000 makes +# the `cp -p` fail, which is the failure path. +# +# The glob has to track the template. It said `.env.??????` after the template +# became `.hostshift-env.XXXXXX`, so it matched nothing and the assertion was +# true however the code behaved — the trap could have been deleted outright. chmod 000 "$wt/.ddev/.env" (cd "$wt" && "$cmd" init --slug wt-a >/dev/null 2>&1) || true chmod 600 "$wt/.ddev/.env" -leftover="$(ls "$wt"/.ddev/.env.?????? 2>/dev/null || true)" +leftover="$(ls "$wt"/.ddev/.hostshift-env.?????? "$wt"/.ddev/.env.?????? 2>/dev/null || true)" if [ -n "$leftover" ]; then fail "a failed init leaves no temp file behind" "$leftover" else @@ -2835,8 +2837,6 @@ out="$(cd "$exw" && DDEV_APPROOT="$exw" PATH="$work/failddev:$PATH" \ "$cmd" init --slug wt-x 2>&1 || true)" contains "and says the env is written and the restart is what failed" \ "the restart is what failed" "$out" -rm -rf "$work/failddev" - echo "== --no-restart, which is what the pre-start hook runs" # The hook is inside the start it would otherwise ask for, so init must be able @@ -2869,6 +2869,14 @@ else pass "a refused --no-restart writes nothing" fi +# Only now. Deleting the stub before the --no-restart block left that block +# prepending a directory that no longer exists, so `command -v ddev` fell through +# to the real binary: on a developer machine a regression in --no-restart would +# have run an actual `ddev restart` from a suite the Makefile calls "no Docker +# and no DDEV needed", and on CI the assertion passed whether or not the flag +# did anything. +rm -rf "$work/failddev" + echo "== the pre-start hook only fires in a linked worktree" # `.git` is a file in a linked worktree and a directory in the parent. Running @@ -2940,12 +2948,15 @@ fi # Configured already: the hook must be a no-op, or every start re-runs the # collision scan and re-derives hostnames after a branch switch. -before="$(cat "$exw/.ddev/.env")" -(cd "$exw" && bash -c "$prehook") >/dev/null 2>&1 || true -if [ "$before" = "$(cat "$exw/.ddev/.env")" ]; then +# Byte-equality of .ddev/.env is not the property: init is idempotent, so that +# assertion passes with the guard deleted. What must be true is that the hook +# does not *run* — no output, and every start otherwise pays for a collision +# scan and a docker inspect. +noop="$(cd "$exw" && bash -c "$prehook" 2>&1)" || true +if [ -z "$noop" ]; then pass "the pre-start hook is a no-op once configured" else - fail "the pre-start hook is a no-op once configured" "the file changed" + fail "the pre-start hook is a no-op once configured" "$noop" fi # In the parent it must do nothing at all. This is the failure the gate exists @@ -2962,10 +2973,20 @@ fi # The temp file init writes must not be one DDEV will later hand to compose. # EnvFiles() globs .ddev/.env.* and passes each as a *later* --env-file, so a # leftover would outrank the file it was written to replace. -case "$(grep -m1 'mktemp .ddev/' "$repo/ddev/commands/host/hostshift")" in - *".ddev/.env."*) fail "the env temp file is outside DDEV's .env.* glob" "matches .env.*" ;; - *) pass "the env temp file is outside DDEV's .env.* glob" ;; -esac +# Run it, do not read it. Grepping the source fails open twice over: no match +# lands in the catch-all arm and passes, and `mktemp -p .ddev .env.XXXXXX` +# reintroduces the bug while defeating the pattern. Hold the temp file open by +# making the write fail, then look at what is on disk. +chmod 000 "$exw/.ddev/.env" +(cd "$exw" && "$cmd" init --slug wt-x >/dev/null 2>&1) || true +chmod 644 "$exw/.ddev/.env" +envglob="$(ls "$exw"/.ddev/.env.* 2>/dev/null || true)" +if [ -n "$envglob" ]; then + fail "no temp file matches DDEV's .env.* glob" \ + "$envglob would be passed to compose as a later --env-file" +else + pass "no temp file matches DDEV's .env.* glob" +fi echo "== a proxy flag in HOSTSHIFT_ARGS survives check and init" diff --git a/test/integration-ddev.sh b/test/integration-ddev.sh index 028268e..3beeb9e 100755 --- a/test/integration-ddev.sh +++ b/test/integration-ddev.sh @@ -166,10 +166,23 @@ installaddon "$wt" projects+=("$main" "$wt") out="$(cd "$main" && ddev start -y 2>&1)" || fail "the parent starts" "$out" -out="$(cd "$wt" && ddev hostshift init 2>&1)" || fail "init succeeds in the worktree" "$out" + +# No `ddev hostshift init` here, deliberately. The pre-start hook derives +# .ddev/.env, and the claim that it lands before compose reads the file is a +# claim about DDEV's ordering inside Start() — which only a real `ddev start` +# can test. Running init first made the hook a no-op in every integration case, +# so the feature had coverage nowhere: the shell logic is exercised in +# addon-command.sh through `bash -c`, and that proves nothing about when DDEV +# runs it. If DDEV ever moves ProcessHooks after WriteDockerComposeYAML, this is +# the test that goes red. +[ -e "$wt/.ddev/.env" ] && fail "the worktree starts unconfigured" "already has .ddev/.env" start_out="$(cd "$wt" && ddev start -y 2>&1)" || fail "the worktree starts" "$start_out" +grep -q '^HOSTSHIFT_VARIANTS=' "$wt/.ddev/.env" 2>/dev/null \ + && pass "ddev start alone configures the worktree" \ + || fail "ddev start alone configures the worktree" "no HOSTSHIFT_VARIANTS in .ddev/.env" + # The hook ran, and did not blow up. Exit 127 here was invisible to every other # test in the repo. #