From 564e89e445816f508416c79d7e4fb45ad06b8a99 Mon Sep 17 00:00:00 2001 From: Claude Date: Thu, 16 Apr 2026 13:23:18 +0000 Subject: [PATCH] fix: bug: generator emits invalid env var name FORGE_BOT_USER_^^ when service name contains hyphen (#852) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Acceptance items 1-4 landed previously: the primary compose emission (FORGE_BOT_USER_*) was fixed in #849 by re-keying on forge_user via `tr 'a-z-' 'A-Z_'`, and the load-project.sh AGENT_* Python emitter was normalized via `.upper().replace('-', '_')` in #862. Together they produce `FORGE_BOT_USER_DEV_QWEN2` and `AGENT_DEV_QWEN2_BASE_URL` for `[agents.dev-qwen2]` with `forge_user = "dev-qwen2"`. This patch closes acceptance item 5 — the defence-in-depth warn-and-skip in load-project.sh's two export loops. Hire-agent's up-front reject is the primary line of defence (a validated `^[a-z]([a-z0-9]|-[a-z0-9])*$` agent name can't produce a bad identifier), but a hand-edited TOML can still smuggle invalid keys through: - `[mirrors] my-mirror = "…"` — the `MIRROR_` emitter only upper-cases, so `MY-MIRROR` retains its dash and fails `export`. - `[agents."weird name"]` — quoted TOML keys bypass the bare-key grammar entirely, so spaces and other disallowed shell chars reach the export loop unchanged. Before this change, either case would abort load-project.sh under `set -euo pipefail` — the exact failure mode the original #852 crash-loop was diagnosed from. Now each loop validates `$_key` against `^[A-Za-z_][A-Za-z0-9_]*$` and warn-skips offenders so siblings still load. - `lib/load-project.sh` — regex guard + WARNING on stderr in both `_PROJECT_VARS` and `_AGENT_VARS` export loops. - `tests/lib-load-project.bats` — two regressions: dashed mirror key, quoted agent section with space. Both assert (a) the load does not abort and (b) sane siblings still load. Co-Authored-By: Claude Opus 4.6 (1M context) --- lib/load-project.sh | 22 ++++++++++++ tests/lib-load-project.bats | 67 +++++++++++++++++++++++++++++++++++++ 2 files changed, 89 insertions(+) diff --git a/lib/load-project.sh b/lib/load-project.sh index 5ad23cc..e42d6dc 100755 --- a/lib/load-project.sh +++ b/lib/load-project.sh @@ -85,8 +85,22 @@ if mirrors: # environment. The TOML carries host-perspective values (localhost, /home/admin/…) # that would break container API calls and path resolution. Skip overriding # any env var that is already set when running inside the container. +# +# #852 defence: validate that $_key is a legal shell identifier before +# `export`. A hand-edited TOML can smuggle in keys that survive the +# Python emitter but fail `export`'s identifier rule — e.g. +# `[mirrors] my-mirror = "..."` becomes `MIRROR_MY-MIRROR` because the +# MIRROR_ emitter only upper-cases, it does not dash-to-underscore. +# Without this guard `export "MIRROR_MY-MIRROR=…"` returns non-zero, and +# under `set -euo pipefail` in the caller the whole file aborts — which +# is how the original #852 crash-loop presented. Warn-and-skip keeps +# the rest of the TOML loadable. while IFS='=' read -r _key _val; do [ -z "$_key" ] && continue + if ! [[ "$_key" =~ ^[A-Za-z_][A-Za-z0-9_]*$ ]]; then + echo "WARNING: load-project: skipping invalid shell identifier from TOML: $_key" >&2 + continue + fi if [ "${DISINTO_CONTAINER:-}" = "1" ] && [ -n "${!_key:-}" ]; then continue fi @@ -152,8 +166,16 @@ for name, config in agents.items(): " "$_PROJECT_TOML" 2>/dev/null) || true if [ -n "$_AGENT_VARS" ]; then + # #852 defence: same warn-and-skip guard as the main loop above. The + # Python emitter already normalizes dashed agent names (#862), but a + # quoted TOML section like `[agents."weird name"]` could still produce + # an invalid identifier. Fail loudly but keep other agents loadable. while IFS='=' read -r _key _val; do [ -z "$_key" ] && continue + if ! [[ "$_key" =~ ^[A-Za-z_][A-Za-z0-9_]*$ ]]; then + echo "WARNING: load-project: skipping invalid shell identifier from [agents.*]: $_key" >&2 + continue + fi export "$_key=$_val" done <<< "$_AGENT_VARS" fi diff --git a/tests/lib-load-project.bats b/tests/lib-load-project.bats index 89e82be..f0c583a 100644 --- a/tests/lib-load-project.bats +++ b/tests/lib-load-project.bats @@ -184,3 +184,70 @@ EOF [ "$status" -ne 0 ] [[ "$output" == *"invalid agent name"* ]] } + +# ------------------------------------------------------------------------- +# #852 defence: the export loops must warn-and-skip invalid identifiers +# rather than tank `set -euo pipefail`. Hire-agent's up-front reject +# (tests above) is the primary line of defence, but a hand-edited TOML — +# e.g. [mirrors] my-mirror = "…" or a quoted [agents."weird name"] — can +# still produce invalid shell identifiers downstream. The guard keeps +# the factory loading the rest of the file instead of crash-looping. +# ------------------------------------------------------------------------- + +@test "[mirrors] dashed key: warn-and-skip, does not crash under set -e" { + cat > "$TOML" <&1 + echo \"GOOD=\${MIRROR_GOOD:-MISSING}\" + " + + # Whole load did not abort under set -e. + [ "$status" -eq 0 ] + # The valid mirror still loads. + [[ "$output" == *"GOOD=https://example.com/good"* ]] + # The invalid one triggers a warning; load continues instead of crashing. + [[ "$output" == *"skipping invalid shell identifier"* ]] + [[ "$output" == *"MIRROR_BAD-NAME"* ]] +} + +@test "[agents.*] quoted section with space: warn-and-skip, does not crash" { + # TOML permits quoted keys with arbitrary characters. A hand-edited + # `[agents."weird name"]` would survive the Python .replace('-', '_') + # (because it has no dash) but still contains a space, which would + # yield AGENT_WEIRD NAME_BASE_URL — not a valid identifier. + cat > "$TOML" <<'EOF' +name = "test" +repo = "test-owner/test-repo" +forge_url = "http://localhost:3000" + +[agents.llama] +base_url = "http://10.10.10.1:8081" +model = "qwen" + +[agents."weird name"] +base_url = "http://10.10.10.1:8082" +model = "qwen-bad" +EOF + + run bash -c " + set -euo pipefail + source '${ROOT}/lib/load-project.sh' '$TOML' 2>&1 + echo \"LLAMA=\${AGENT_LLAMA_BASE_URL:-MISSING}\" + " + + # The sane sibling must still be loaded despite the malformed neighbour. + [ "$status" -eq 0 ] + [[ "$output" == *"LLAMA=http://10.10.10.1:8081"* ]] + # The invalid agent's identifier triggers a warning and is skipped. + [[ "$output" == *"skipping invalid shell identifier"* ]] +}