fix: bug: generator emits invalid env var name FORGE_BOT_USER_<service>^^ when service name contains hyphen (#852)
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_<NAME>` 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) <noreply@anthropic.com>
This commit is contained in:
parent
46b3d96410
commit
564e89e445
2 changed files with 89 additions and 0 deletions
|
|
@ -85,8 +85,22 @@ if mirrors:
|
||||||
# environment. The TOML carries host-perspective values (localhost, /home/admin/…)
|
# environment. The TOML carries host-perspective values (localhost, /home/admin/…)
|
||||||
# that would break container API calls and path resolution. Skip overriding
|
# that would break container API calls and path resolution. Skip overriding
|
||||||
# any env var that is already set when running inside the container.
|
# 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_<NAME> 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
|
while IFS='=' read -r _key _val; do
|
||||||
[ -z "$_key" ] && continue
|
[ -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
|
if [ "${DISINTO_CONTAINER:-}" = "1" ] && [ -n "${!_key:-}" ]; then
|
||||||
continue
|
continue
|
||||||
fi
|
fi
|
||||||
|
|
@ -152,8 +166,16 @@ for name, config in agents.items():
|
||||||
" "$_PROJECT_TOML" 2>/dev/null) || true
|
" "$_PROJECT_TOML" 2>/dev/null) || true
|
||||||
|
|
||||||
if [ -n "$_AGENT_VARS" ]; then
|
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
|
while IFS='=' read -r _key _val; do
|
||||||
[ -z "$_key" ] && continue
|
[ -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"
|
export "$_key=$_val"
|
||||||
done <<< "$_AGENT_VARS"
|
done <<< "$_AGENT_VARS"
|
||||||
fi
|
fi
|
||||||
|
|
|
||||||
|
|
@ -184,3 +184,70 @@ EOF
|
||||||
[ "$status" -ne 0 ]
|
[ "$status" -ne 0 ]
|
||||||
[[ "$output" == *"invalid agent name"* ]]
|
[[ "$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" <<EOF
|
||||||
|
name = "test"
|
||||||
|
repo = "test-owner/test-repo"
|
||||||
|
forge_url = "http://localhost:3000"
|
||||||
|
|
||||||
|
[mirrors]
|
||||||
|
good = "https://example.com/good"
|
||||||
|
bad-name = "https://example.com/bad"
|
||||||
|
EOF
|
||||||
|
|
||||||
|
run bash -c "
|
||||||
|
set -euo pipefail
|
||||||
|
source '${ROOT}/lib/load-project.sh' '$TOML' 2>&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"* ]]
|
||||||
|
}
|
||||||
|
|
|
||||||
Loading…
Add table
Add a link
Reference in a new issue