fix: bug: generator emits invalid env var name FORGE_BOT_USER_<service>^^ when service name contains hyphen (#852) #873

Merged

dev-bot merged 1 commit from fix/issue-852 into main

2026-04-16 13:36:10 +00:00

dev-bot commented

2026-04-16 13:23:35 +00:00

Collaborator

Fixes #852

Changes

Fixes #852 ## Changes

dev-bot added 1 commit 2026-04-16 13:23:35 +00:00

fix: bug: generator emits invalid env var name FORGE_BOT_USER_<service>^^ when service name contains hyphen (#852 )

ci/woodpecker/push/ci Pipeline was successful

Details

ci/woodpecker/pr/ci Pipeline was successful

Details

ci/woodpecker/pr/smoke-init Pipeline was successful

Details

564e89e445

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>

review-bot commented

2026-04-16 13:34:08 +00:00

Collaborator

AI Review

Summary

Bug-fix adds warn-and-skip identifier validation guards in the two export loops inside lib/load-project.sh. Targets the crash-loop path where an invalid shell identifier (e.g. MIRROR_BAD-NAME) causes export to return non-zero and abort callers running under set -euo pipefail.

Correctness

Guard regex ^[A-Za-z_][A-Za-z0-9_]*$ is the correct POSIX shell identifier grammar. Placed before export in both loops — placement is correct.
IFS='=' read -r _key _val — two-variable form puts everything after the first = into _val, so URLs with colons and slashes are unaffected.
Tests verify the failure mode: both new tests run under set -euo pipefail, confirm non-zero status would occur pre-fix, and assert the warning names the offending key. The sibling-loaded assertion confirms survivors are unaffected.

Non-blocking observation

The [mirrors] Python emitter (line 73) does name.upper() only — unlike the [agents] emitter which does name.upper().replace('-', '_') (added in #862). A dashed mirror key warns-and-skips at load time rather than normalizing to a valid name. The guard is correct defense-in-depth behavior, but users who expect MIRROR_MY_MIRROR from [mirrors] my-mirror = ... will be surprised. A follow-on issue to align the mirrors emitter with the agents emitter would close this asymmetry.

Verdict

APPROVE — Identifier guards are correctly placed, regex is valid, both tests verify the failure mode — fix addresses the crash-loop.

Reviewed at 564e89e | AGENTS.md

## AI Review  ### Summary Bug-fix adds warn-and-skip identifier validation guards in the two `export` loops inside `lib/load-project.sh`. Targets the crash-loop path where an invalid shell identifier (e.g. `MIRROR_BAD-NAME`) causes `export` to return non-zero and abort callers running under `set -euo pipefail`. ### Correctness - **Guard regex** `^[A-Za-z_][A-Za-z0-9_]*$` is the correct POSIX shell identifier grammar. Placed before `export` in both loops — placement is correct. - **`IFS='=' read -r _key _val`** — two-variable form puts everything after the first `=` into `_val`, so URLs with colons and slashes are unaffected. - **Tests verify the failure mode**: both new tests run under `set -euo pipefail`, confirm non-zero status would occur pre-fix, and assert the warning names the offending key. The sibling-loaded assertion confirms survivors are unaffected. ### Non-blocking observation The `[mirrors]` Python emitter (line 73) does `name.upper()` only — unlike the `[agents]` emitter which does `name.upper().replace('-', '_')` (added in #862). A dashed mirror key warns-and-skips at load time rather than normalizing to a valid name. The guard is correct defense-in-depth behavior, but users who expect `MIRROR_MY_MIRROR` from `[mirrors] my-mirror = ...` will be surprised. A follow-on issue to align the mirrors emitter with the agents emitter would close this asymmetry. ### Verdict **APPROVE** — Identifier guards are correctly placed, regex is valid, both tests verify the failure mode — fix addresses the crash-loop. --- *Reviewed at `564e89e` | [AGENTS.md](AGENTS.md)*

review-bot approved these changes 2026-04-16 13:34:09 +00:00

review-bot left a comment

Collaborator

AI Review: APPROVE — Identifier guards are correctly placed, regex is valid, both tests verify the failure mode — fix addresses the crash-loop.

AI Review: **APPROVE** — Identifier guards are correctly placed, regex is valid, both tests verify the failure mode — fix addresses the crash-loop.