diff --git a/.agents/skills/lib.sh b/.agents/skills/lib.sh index dadb3d2..f8e6488 100644 --- a/.agents/skills/lib.sh +++ b/.agents/skills/lib.sh @@ -2343,7 +2343,12 @@ sys.exit(1) # 448 and fail on 0900), and min <= n <= 999999999. Prints n and returns 0, or # returns 1. The per-knob minimum lives in tests/test_issue26_ints.py. mam_int() { - [[ "${1-}" =~ ^[[:space:]]*([0-9]+)[[:space:]]*$ ]] || return 1 + # Spelled-out ASCII sets, not [[:space:]]/[0-9]: those follow the locale, and + # the runner's _positive_int takes ASCII only (review R2: U+2003 30 U+2003 + # was 30 here under en_US.UTF-8 and invalid there). + local _ws=$' \t\n\r\v\f' _re + _re="^[${_ws}]*([0123456789]+)[${_ws}]*\$" + [[ "${1-}" =~ $_re ]] || return 1 local d="${BASH_REMATCH[1]}" d="${d#"${d%%[!0]*}"}" [ ${#d} -le 9 ] || return 1 diff --git a/.agents/skills/lib_py/delegation/envfile.py b/.agents/skills/lib_py/delegation/envfile.py index 696a67f..6efbefb 100644 --- a/.agents/skills/lib_py/delegation/envfile.py +++ b/.agents/skills/lib_py/delegation/envfile.py @@ -5,8 +5,11 @@ the value stripped; one matching pair of surrounding quotes is removed; no shell interpretation (`$(...)`, `${...}` and backticks stay literal). A key must match ``^[A-Za-z_][A-Za-z0-9_]*$`` and not be one of bash's own variables (RESERVED); a line that breaks either, or that has no `=`, is skipped with a -warning naming the line (never the value — the file holds secrets). -Variables already in the environment win over the file. +warning naming the line (never the value — the file holds secrets). A line +holding a NUL byte is skipped the same way (no environment can carry it). +Variables already in the environment win over the file. A file that exists +but cannot be read or decoded as a whole is an error (EnvFileError; the +``--shell`` command exits 1), not an empty configuration. Stdlib only: the delegate-job wrapper runs this file by path (``envfile.py --shell ``) and evals the ``export`` lines it prints, so @@ -38,23 +41,31 @@ RESERVED = frozenset(""" RESERVED_PREFIXES = ("BASH_", "COMP_", "READLINE_") +class EnvFileError(RuntimeError): + """The env file exists but cannot be read or decoded as a whole.""" + + def is_reserved(key: str) -> bool: return key in RESERVED or key.startswith(RESERVED_PREFIXES) def parse(path: str) -> Tuple[List[Tuple[str, str]], List[str]]: - """Return ([(key, value)], [warning]) for the file at ``path``.""" + """Return ([(key, value)], [warning]) for the file at ``path``; raise + EnvFileError when the file cannot be read or decoded as a whole.""" pairs: List[Tuple[str, str]] = [] warnings: List[str] = [] try: with open(path, "r", encoding="utf-8") as f: lines = f.read().splitlines() except (OSError, UnicodeDecodeError) as e: - return pairs, [f"{path}: cannot read ({type(e).__name__})"] + raise EnvFileError(f"{path}: cannot read ({type(e).__name__})") from None for n, raw in enumerate(lines, 1): line = raw.strip() if not line or line.startswith("#"): continue + if "\0" in line: + warnings.append(f"{path}:{n}: skipped (NUL byte)") + continue key, sep, val = line.partition("=") key = key.strip() if not sep: @@ -91,7 +102,11 @@ def main(argv: List[str]) -> int: path = argv[1] if not os.path.isfile(path): return 0 - applied, warnings = apply(path, dict(os.environ)) + try: + applied, warnings = apply(path, dict(os.environ)) + except EnvFileError as e: + print(f"ERROR: {e}", file=sys.stderr) + return 1 for w in warnings: print(f"WARNING: {w}", file=sys.stderr) for key, val in applied: diff --git a/.agents/skills/lib_py/delegation/mqtt_common.py b/.agents/skills/lib_py/delegation/mqtt_common.py index cad9466..89445bf 100644 --- a/.agents/skills/lib_py/delegation/mqtt_common.py +++ b/.agents/skills/lib_py/delegation/mqtt_common.py @@ -96,7 +96,10 @@ def _load_dotenv(workspace_dir: Optional[str] = None) -> None: _parse_env_file(legacy_env_path) def _parse_env_file(path: str) -> None: - """Issue #26: the rules live in envfile.py, shared with the wrapper.""" + """Issue #26: the rules live in envfile.py, shared with the wrapper. A + file that exists but cannot be read raises envfile.EnvFileError, so every + process entry point stops as the wrapper does, instead of running + unconfigured.""" for warning in envfile.apply(path, os.environ)[1]: logger.warning("%s", warning) diff --git a/.agents/skills/multi-agent-mux-delegate-job/multi-agent-mux-delegate-job b/.agents/skills/multi-agent-mux-delegate-job/multi-agent-mux-delegate-job index afa62a5..e70f7cb 100755 --- a/.agents/skills/multi-agent-mux-delegate-job/multi-agent-mux-delegate-job +++ b/.agents/skills/multi-agent-mux-delegate-job/multi-agent-mux-delegate-job @@ -41,7 +41,7 @@ fi if [[ -n "$TARGET_ENV" && -f "$TARGET_ENV" ]]; then _env_exports="$(SCRIPT_DIR="$SCRIPT_DIR" REPO_ROOT="$REPO_ROOT" TARGET_ENV="$TARGET_ENV" \ "${DELEGATE_JOB_PYTHON:-python3}" -I "$SCRIPT_DIR/../lib_py/delegation/envfile.py" --shell "$TARGET_ENV")" \ - || { echo "ERROR: cannot read $TARGET_ENV" >&2; exit 1; } + || { echo "ERROR: cannot load $TARGET_ENV (see the line above)" >&2; exit 1; } eval "$_env_exports"; unset _env_exports fi @@ -164,6 +164,7 @@ COUNTERPART_ROLE_EXPLICIT=0 PARENT_JOB="" parse_opts() { + _INT_FLAGS_GIVEN="" while [[ $# -gt 0 ]]; do case "$1" in --agent) AGENT="$2"; shift 2;; @@ -171,11 +172,11 @@ parse_opts() { --prompt-file) PROMPT_FILE="$2"; shift 2;; --workdir) WORKDIR="$2"; shift 2;; --agent-session) AGENT_SESSION="$2"; shift 2;; - --timeout) TIMEOUT="$2"; shift 2;; - --idle-timeout) IDLE_TIMEOUT="$2"; shift 2;; - --heartbeat) HEARTBEAT_SEC="$2"; shift 2;; - --heartbeat-misses) HEARTBEAT_MISSES="$2"; shift 2;; - --ack-timeout) ACK_TIMEOUT="$2"; shift 2;; + --timeout) TIMEOUT="$2"; _INT_FLAGS_GIVEN+=" TIMEOUT"; shift 2;; + --idle-timeout) IDLE_TIMEOUT="$2"; _INT_FLAGS_GIVEN+=" IDLE_TIMEOUT"; shift 2;; + --heartbeat) HEARTBEAT_SEC="$2"; _INT_FLAGS_GIVEN+=" HEARTBEAT_SEC"; shift 2;; + --heartbeat-misses) HEARTBEAT_MISSES="$2"; _INT_FLAGS_GIVEN+=" HEARTBEAT_MISSES"; shift 2;; + --ack-timeout) ACK_TIMEOUT="$2"; _INT_FLAGS_GIVEN+=" ACK_TIMEOUT"; shift 2;; --validate) VALIDATE="$2"; shift 2;; --job) JOB_ID="$2"; shift 2;; --registry-dir) REGISTRY_DIR="$2"; shift 2;; @@ -191,10 +192,13 @@ parse_opts() { *) echo "unknown option: $1" >&2; usage; exit 1;; esac done - # Issue #26: a flag value that is not a positive decimal integer is refused. + # Issue #26: a flag value that is not a positive decimal integer is refused - + # an explicitly given empty value too (review R3); only an absent flag keeps + # its default. local _flag _var _n for _flag in timeout:TIMEOUT idle-timeout:IDLE_TIMEOUT heartbeat:HEARTBEAT_SEC heartbeat-misses:HEARTBEAT_MISSES ack-timeout:ACK_TIMEOUT; do - _var="${_flag#*:}"; [ -n "${!_var}" ] || continue + _var="${_flag#*:}" + [ -n "${!_var}" ] || [[ " $_INT_FLAGS_GIVEN " == *" $_var "* ]] || continue _n=$(mam_int "${!_var}" 1) || { echo "ERROR: --${_flag%%:*} ${!_var}: not a positive decimal integer" >&2; exit 1; } printf -v "$_var" '%s' "$_n" done diff --git a/.mam.env.example b/.mam.env.example index 57ace7d..02d6939 100644 --- a/.mam.env.example +++ b/.mam.env.example @@ -11,6 +11,19 @@ # (shown after each `#default:` line), so an unset/commented variable just keeps # the built-in behaviour. Uncomment + edit only the ones you want to override. # +# FORMAT (Issue #26 - one parser, lib_py/delegation/envfile.py, for the +# delegate-job wrapper and the Python tools alike; the file is never `source`d): +# - one KEY=VALUE per line; blank lines and lines starting with `#` are skipped; +# - spaces around the key and the value are removed, and one pair of matching +# quotes around the value is removed ("a b" -> a b); +# - nothing is expanded: `$HOME`, `${X}`, `$(cmd)` and backticks stay literal, +# so write absolute paths; a trailing `# ...` is part of the value; +# - `export KEY=...`, a key that is not [A-Za-z_][A-Za-z0-9_]*, a line +# without `=`, a line with a NUL byte, and bash's own variables (IFS, PS4, +# RANDOM, BASH_*, ...) are skipped with a warning naming the line number; +# - a variable already set in the environment wins over this file; +# - a file that exists but cannot be read is an error (the wrapper exits 1). +# # SECURITY: never put real secrets in this template. Secret-bearing vars use a # `replace_me` placeholder — fill them in only in your local `.mam.env`. # SECURITY NOTE: diff --git a/tests/test_issue26_envfile.py b/tests/test_issue26_envfile.py index 3965850..190451f 100644 --- a/tests/test_issue26_envfile.py +++ b/tests/test_issue26_envfile.py @@ -143,7 +143,54 @@ def test_a1_an_unreadable_parser_fails_loudly(tmp_path): env=_env(tmp_path, f, {"DELEGATE_JOB_PYTHON": str(tmp_path / "no-python")}), cwd=str(tmp_path), timeout=60) assert res.returncode == 1 - assert f"ERROR: cannot read {f}" in res.stderr + assert f"ERROR: cannot load {f}" in res.stderr + + +def _python_load(env: dict) -> subprocess.CompletedProcess: + code = "from lib_py.delegation import mqtt_common; mqtt_common.ensure_env_loaded(); print('LOADED')" + return subprocess.run([sys.executable, "-c", code], capture_output=True, text=True, env=env, timeout=60) + + +@pytest.mark.parametrize("kind", ["permission", "utf8"]) +def test_a1_a_file_that_cannot_be_read_stops_both_loaders(tmp_path, kind): + """Review R1: a file that exists but cannot be read or decoded as a whole + is an error in the wrapper (rc 1, as before #26) and in the Python entry + points - never an empty configuration that runs on defaults.""" + f = tmp_path / "broken.env" + if kind == "permission": + if os.geteuid() == 0: + pytest.skip("root reads a 000 file") + f.write_text("PROBE_A=one\n") + f.chmod(0) + cause = "PermissionError" + else: + f.write_bytes(b"PROBE_A=one\nPROBE_B=\xff\xfe\n") + cause = "UnicodeDecodeError" + try: + res = subprocess.run(["bash", str(WRAPPER), "--help"], capture_output=True, text=True, + env=_env(tmp_path, f), cwd=str(tmp_path), timeout=60) + assert res.returncode == 1, res.stdout + res.stderr + assert f"ERROR: {f}: cannot read ({cause})" in res.stderr, res.stderr + assert f"ERROR: cannot load {f}" in res.stderr, res.stderr + assert "one" not in res.stderr # no value in any message + py = _python_load(_env(tmp_path, f)) + assert py.returncode != 0 and "LOADED" not in py.stdout, py.stdout + py.stderr + assert f"{f}: cannot read ({cause})" in py.stderr and "EnvFileError" in py.stderr, py.stderr[-500:] + finally: + f.chmod(0o600) + + +def test_a1_a_nul_line_is_skipped_by_both_loaders(tmp_path): + """Review R4: no environment can carry a NUL byte. Both loaders skip that + line with a warning naming it (not its value) and keep the lines around it.""" + f = tmp_path / "nul.env" + f.write_bytes(b"GOOD=ok\nVALUE=left\x00right\nAFTER=yes\n") + env = _env(tmp_path, f) + got = _wrapper_values(env, ["GOOD", "VALUE", "AFTER"], tmp_path) + assert got.returncode == 0, got.stderr + assert _split(got.stdout) == {"GOOD": "ok", "VALUE": "", "AFTER": "yes"} + assert f"{f}:2: skipped (NUL byte)" in got.stderr and "left" not in got.stderr, got.stderr + assert _python_values(env, ["GOOD", "VALUE", "AFTER"]) == {"GOOD": "ok", "VALUE": "", "AFTER": "yes"} # --------------------------------------------------------------------------- diff --git a/tests/test_issue26_ints.py b/tests/test_issue26_ints.py index 3c40fb4..1e05ab7 100644 --- a/tests/test_issue26_ints.py +++ b/tests/test_issue26_ints.py @@ -86,8 +86,8 @@ NOT_ARITHMETIC = { def _expected(value: str, minimum: int): - s = value.strip() - if not s.isdigit(): + s = value.strip(" \t\n\r\v\f") # ASCII blanks only, in every locale (review R2) + if not s.isascii() or not s.isdigit(): return None s = s.lstrip("0") or "0" if len(s) > 9: @@ -182,6 +182,28 @@ def test_b1_mam_int_and_positive_int_are_one_rule(tmp_path, minimum): assert want[INPUTS.index("0700")] == 700 +# Review R2: bash's [[:space:]] and [0-9] follow the locale; the rule is ASCII. +UNICODE_INPUTS = ["\u200330\u2003", "\u00a030", "30\u3000", "\u0663\u0660", "\uff13\uff10"] + + +@pytest.mark.parametrize("locale", ["C", "en_US.UTF-8"]) +@pytest.mark.parametrize("value", UNICODE_INPUTS, ids=lambda v: v.encode("unicode_escape").decode()) +def test_b1_unicode_blanks_and_digits_are_invalid_in_every_locale(tmp_path, locale, value): + if str(SKILLS) not in sys.path: + sys.path.insert(0, str(SKILLS)) + from lib_py.loop.runner import _positive_int + + loc = {"LC_ALL": locale, "LANG": locale} + res = _bash('mam_int "$PROBE" 1 || echo invalid', tmp_path, {**loc, "PROBE": value}) + assert res.stdout.strip() == "invalid", (res.stdout, res.stderr) + assert _positive_int(value) is None and _expected(value, 1) is None + rec = _record(tmp_path, _dry_submit(tmp_path, {**loc, "MAM_JOB_TIMEOUT_S": value})) + rres = _runner(tmp_path, {**loc, "MAM_JOB_TIMEOUT_S": value}) + assert rres.returncode == 0, rres.stderr[-500:] + got = json.loads(rres.stdout.strip().splitlines()[-1]) + assert rec["timeout_sec"] == got["budget"] == 3600, (rec["timeout_sec"], got) + + @pytest.mark.parametrize("knob", ["SKS_DIALOG_TIMEOUT", "SKS_BUSY_TIMEOUT", "SKS_DELIVERY_GRACE_S"]) def test_b1_sks_knobs_keep_zero_and_read_decimal(tmp_path, knob): minimum, default = INT_KNOBS[knob] @@ -204,7 +226,7 @@ def test_b2_wrapper_flags_are_decimal_or_refused(tmp_path): ok = _dry_submit(tmp_path, None, "--timeout", "30") assert _record(tmp_path, ok)["timeout_sec"] == 30 for flag in ("--timeout", "--idle-timeout", "--heartbeat", "--heartbeat-misses", "--ack-timeout"): - for bad in ("abc", "0", "-5", "9999999999", "9" * 5000): + for bad in ("abc", "0", "-5", "9999999999", "9" * 5000, ""): # "" given explicitly (review R3) res = _dry_submit(tmp_path, None, flag, bad) assert res.returncode == 1, (flag, bad[:20], res.stdout, res.stderr[-500:]) assert f"ERROR: {flag} {bad}: not a positive decimal integer" in res.stderr, res.stderr[-500:]