diff --git a/.agents/skills/lib_py/delegation/envfile.py b/.agents/skills/lib_py/delegation/envfile.py new file mode 100644 index 0000000..6fcce29 --- /dev/null +++ b/.agents/skills/lib_py/delegation/envfile.py @@ -0,0 +1,78 @@ +"""The one `.mam.env` parser (Issue #26). + +Rules: blank lines and `#` comments are skipped; `KEY=VALUE` with the key and +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_]*$``; a line that does not, 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. + +Stdlib only: the delegate-job wrapper runs this file by path +(``envfile.py --shell ``) and evals the ``export`` lines it prints, so +values travel over a pipe, never on a command line. +""" +import os +import re +import shlex +import sys +from typing import List, MutableMapping, Tuple + +_KEY = re.compile(r"[A-Za-z_][A-Za-z0-9_]*") + + +def parse(path: str) -> Tuple[List[Tuple[str, str]], List[str]]: + """Return ([(key, value)], [warning]) for the file at ``path``.""" + 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__})"] + for n, raw in enumerate(lines, 1): + line = raw.strip() + if not line or line.startswith("#"): + continue + key, sep, val = line.partition("=") + key = key.strip() + if not sep: + warnings.append(f"{path}:{n}: skipped (no '=')") + continue + if not _KEY.fullmatch(key): + warnings.append(f"{path}:{n}: skipped (invalid key)") + continue + val = val.strip() + if len(val) >= 2 and val[0] == val[-1] and val[0] in "\"'": + val = val[1:-1] + pairs.append((key, val)) + return pairs, warnings + + +def apply(path: str, environ: MutableMapping[str, str]) -> Tuple[List[Tuple[str, str]], List[str]]: + """Set each pair not already in ``environ``; return (the pairs set, warnings).""" + pairs, warnings = parse(path) + applied = [] + for key, val in pairs: + if key not in environ: + environ[key] = val + applied.append((key, val)) + return applied, warnings + + +def main(argv: List[str]) -> int: + if len(argv) != 2 or argv[0] != "--shell": + print("usage: envfile.py --shell ", file=sys.stderr) + return 2 + path = argv[1] + if not os.path.isfile(path): + return 0 + applied, warnings = apply(path, dict(os.environ)) + for w in warnings: + print(f"WARNING: {w}", file=sys.stderr) + for key, val in applied: + print(f"export {key}={shlex.quote(val)}") + return 0 + + +if __name__ == "__main__": + sys.exit(main(sys.argv[1:])) diff --git a/.agents/skills/lib_py/delegation/mqtt_common.py b/.agents/skills/lib_py/delegation/mqtt_common.py index d506b47..cad9466 100644 --- a/.agents/skills/lib_py/delegation/mqtt_common.py +++ b/.agents/skills/lib_py/delegation/mqtt_common.py @@ -31,6 +31,8 @@ from typing import Any, Callable, Dict, Iterable, List, Optional import paho.mqtt.client as mqtt +from lib_py.delegation import envfile + logger = logging.getLogger("delegate_job.mqtt_common") _warned_deprecated_env = False @@ -94,20 +96,9 @@ def _load_dotenv(workspace_dir: Optional[str] = None) -> None: _parse_env_file(legacy_env_path) def _parse_env_file(path: str) -> None: - try: - with open(path, "r", encoding="utf-8") as f: - for line in f: - line = line.strip() - if not line or line.startswith("#"): - continue - if "=" in line: - key, _, val = line.partition("=") - key = key.strip() - val = val.strip().strip('"').strip("'") - if key and key not in os.environ: - os.environ[key] = val - except Exception as e: - logger.warning("Failed to parse env file %s: %s", path, e) + """Issue #26: the rules live in envfile.py, shared with the wrapper.""" + for warning in envfile.apply(path, os.environ)[1]: + logger.warning("%s", warning) _env_loaded = False 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 103501b..2875cfd 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 @@ -34,8 +34,12 @@ if [[ -z "$TARGET_ENV" ]]; then fi fi +# Issue #26: parsed by envfile.py (the Python loader's rules; environment wins), +# never sourced. Values come back as shlex-quoted exports over the pipe. if [[ -n "$TARGET_ENV" && -f "$TARGET_ENV" ]]; then - set -a; source "$TARGET_ENV"; set +a + _env_exports="$("${DELEGATE_JOB_PYTHON:-python3}" -I "$SCRIPT_DIR/../lib_py/delegation/envfile.py" --shell "$TARGET_ENV")" \ + || { echo "ERROR: cannot read $TARGET_ENV" >&2; exit 1; } + eval "$_env_exports"; unset _env_exports fi # Source EARLY (before any herdr usage in run_agent) — this is what turns diff --git a/.mam.env.example b/.mam.env.example index 0b77b0f..57ace7d 100644 --- a/.mam.env.example +++ b/.mam.env.example @@ -29,11 +29,11 @@ # Root directory that holds Claude Code per-project conversation logs (*.jsonl). #default: $HOME/.claude/projects -# CLAUDE_PROJECT_DIR=$HOME/.claude/projects +# CLAUDE_PROJECT_DIR=/home/you/.claude/projects # Directory scanned for per-session launcher wrappers (~/.local/bin/). #default: $HOME/.local/bin -# LOCAL_BIN=$HOME/.local/bin +# LOCAL_BIN=/home/you/.local/bin # Isolated Herdr session socket name (`herdr --session `). #default: mam- @@ -46,7 +46,7 @@ # Path to the Herdr Unix domain socket. #default: $HOME/.config/herdr/herdr.sock -# HERDR_SOCKET_PATH=$HOME/.config/herdr/herdr.sock +# HERDR_SOCKET_PATH=/home/you/.config/herdr/herdr.sock # Explicit Herdr workspace scope identifier (`herdr pane list --workspace `). # Hard filters pane/agent resolution to this workspace. When unset, falls back @@ -107,7 +107,7 @@ # Optional override for Codex CLI state / session directory. #default: $HOME/.codex -# CODEX_HOME=$HOME/.codex +# CODEX_HOME=/home/you/.codex # Optional inline JSON permissions configuration for OpenCode. #default: (unset → defaults to --auto CLI bypass) diff --git a/tests/test_issue26_envfile.py b/tests/test_issue26_envfile.py new file mode 100644 index 0000000..d2bb1e2 --- /dev/null +++ b/tests/test_issue26_envfile.py @@ -0,0 +1,253 @@ +"""Issue #26 (A): one `.mam.env` parser for the wrapper and the Python loader. + +- A1: the issue's file no longer kills the real wrapper, runs no shell, warns + per bad line without leaking values. +- A2: the real wrapper and the real Python loader read one table the same way. +- A3: an environment variable beats the file, in both loaders. +- A4: the parser's command line carries the path only. +- A5: the files this repo ships or generates parse without a warning. +""" + +from __future__ import annotations + +import json +import os +import re +import subprocess +import sys +from pathlib import Path + +import pytest + +REPO_ROOT = Path(__file__).resolve().parent.parent +SKILLS = REPO_ROOT / ".agents" / "skills" +WRAPPER = SKILLS / "multi-agent-mux-delegate-job" / "multi-agent-mux-delegate-job" +ENVFILE = SKILLS / "lib_py" / "delegation" / "envfile.py" + +if str(SKILLS) not in sys.path: + sys.path.insert(0, str(SKILLS)) + +from lib_py.delegation import envfile # noqa: E402 + +ISSUE_FILE = ( + "AGENT_SESSIONS_YAML= .mam/agent-sessions.yaml\n" + "X=$(touch pwned)\n" + "BAD KEY=1\n" + "noequals\n" + 'MQTT_PASSWORD="s3cr3t-a1"\n' +) +CLEAN_FILE = ( + "# comment\n" + "\n" + "AGENT_SESSIONS_YAML=.mam/agent-sessions.yaml\n" + 'MQTT_PASSWORD="s3cr3t-a1"\n' +) +# key -> value both loaders must produce (A2) +TABLE_FILE = ( + " SPACED = padded value \n" + "# ignored=1\n" + "\n" + "DQ=\"double quoted\"\n" + "SQ='single quoted'\n" + "HALF=\"unmatched\n" + "MIXED=\"a'\n" + "EQ=a=b=c\n" + "EMPTY=\n" + "LIT=$(echo no) ${HOME} `echo no`\n" + "export EXPORTED=1\n" + "1BAD=1\n" +) +TABLE = { + "SPACED": "padded value", + "DQ": "double quoted", + "SQ": "single quoted", + "HALF": '"unmatched', + "MIXED": "\"a'", + "EQ": "a=b=c", + "EMPTY": "", + "LIT": "$(echo no) ${HOME} `echo no`", +} +TABLE_SKIPPED = {"EXPORTED", "export EXPORTED", "1BAD"} + + +def _env(home: Path, env_file: Path, extra=None) -> dict: + env = {k: v for k, v in os.environ.items() if k not in TABLE and k not in ("MAM_JOB_TIMEOUT_S", "MQTT_PASSWORD", + "AGENT_SESSIONS_YAML", "X")} + env.update({"HOME": str(home), "WORKSPACE_ROOT": str(home), "MAM_ENV_FILE": str(env_file), + "DELEGATE_JOB_PYTHON": sys.executable, "AGENT_PYTHON_BIN": sys.executable, + "PYTHONPATH": str(SKILLS), "DELEGATE_JOB_LOGS_DIR": str(home / "logs")}) + if extra: + env.update(extra) + return env + + +def _wrapper_values(env: dict, keys, cwd: Path) -> subprocess.CompletedProcess: + """The values the real wrapper exported after loading its env file.""" + probe = "; ".join(f'printf "%s=%s\\0" {k} "${{{k}-}}"' for k in keys) + return subprocess.run(["bash", "-c", f'source "{WRAPPER}"; {probe}'], capture_output=True, text=True, + env={**env, "MAM_DELEGATE_JOB_NO_MAIN": "1"}, cwd=str(cwd), timeout=60) + + +def _python_values(env: dict, keys) -> dict: + """The values the real Python loader (mqtt_common) set from the same file.""" + code = ("import json, os; from lib_py.delegation import mqtt_common; mqtt_common.ensure_env_loaded(); " + f"print(json.dumps({{k: os.environ.get(k, '') for k in {list(keys)!r}}}))") + res = subprocess.run([sys.executable, "-c", code], capture_output=True, text=True, env=env, timeout=60) + assert res.returncode == 0, res.stderr + return json.loads(res.stdout.strip().splitlines()[-1]) + + +def _split(out: str) -> dict: + return dict(item.split("=", 1) for item in out.split("\0") if item) + + +# --------------------------------------------------------------------------- +# A1 - the issue's file +# --------------------------------------------------------------------------- + +def test_a1_the_issue_file_does_not_kill_the_wrapper_nor_run_shell(tmp_path): + f = tmp_path / "issue.env" + f.write_text(ISSUE_FILE) + 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 == 0, res.stderr + assert "submit" in res.stdout + assert not (tmp_path / "pwned").exists(), "a value must never be run as shell" + for n, reason in ((3, "invalid key"), (4, "no '='")): + assert f"{f}:{n}: skipped ({reason})" in res.stderr, res.stderr + assert ":1: skipped" not in res.stderr and ":2: skipped" not in res.stderr + assert "s3cr3t-a1" not in res.stdout + res.stderr + + pairs, warnings = envfile.parse(str(f)) + assert dict(pairs) == {"AGENT_SESSIONS_YAML": ".mam/agent-sessions.yaml", "X": "$(touch pwned)", + "MQTT_PASSWORD": "s3cr3t-a1"} + assert len(warnings) == 2 and not any("s3cr3t" in w for w in warnings) + + +def test_a1_a_clean_file_loads_silently(tmp_path): + f = tmp_path / "clean.env" + f.write_text(CLEAN_FILE) + 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 == 0, res.stderr + assert "skipped" not in res.stderr and "WARNING" not in res.stderr, res.stderr + got = _wrapper_values(_env(tmp_path, f), ["AGENT_SESSIONS_YAML", "MQTT_PASSWORD"], tmp_path) + assert _split(got.stdout) == {"AGENT_SESSIONS_YAML": ".mam/agent-sessions.yaml", "MQTT_PASSWORD": "s3cr3t-a1"} + + +def test_a1_an_unreadable_parser_fails_loudly(tmp_path): + """The wrapper stops when the parser cannot run instead of going on unconfigured.""" + f = tmp_path / "clean.env" + f.write_text(CLEAN_FILE) + res = subprocess.run(["bash", str(WRAPPER), "--help"], capture_output=True, text=True, + 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 + + +# --------------------------------------------------------------------------- +# A2 - one table, two real loaders, same values +# --------------------------------------------------------------------------- + +def test_a2_the_wrapper_and_the_python_loader_read_the_same_values(tmp_path): + f = tmp_path / "table.env" + f.write_text(TABLE_FILE) + env = _env(tmp_path, f) + keys = list(TABLE) + ["EXPORTED"] + res = _wrapper_values(env, keys, tmp_path) + assert res.returncode == 0, res.stderr + wrapper = _split(res.stdout) + python = _python_values(env, keys) + assert wrapper == python == {**TABLE, "EXPORTED": ""} + for n in (11, 12): + assert f"{f}:{n}: skipped (invalid key)" in res.stderr, res.stderr + pairs, _ = envfile.parse(str(f)) + assert not TABLE_SKIPPED & {k for k, _ in pairs} + + +# --------------------------------------------------------------------------- +# A3 - the environment beats the file, in both loaders +# --------------------------------------------------------------------------- + +def _dry_budget(env: dict, cwd: Path) -> int: + reg = cwd / "reg" + res = subprocess.run(["bash", str(WRAPPER), "submit", "--agent", "human", "--dry-run", + "--registry-dir", str(reg), "--prompt", "p"], + capture_output=True, text=True, env=env, cwd=str(cwd), timeout=120) + assert res.returncode == 0, res.stdout + res.stderr + jid = re.search(r"registered job:\s*(\S+)", res.stdout).group(1) + return json.loads((reg / f"{jid}.json").read_text())["timeout_sec"] + + +def test_a3_an_environment_value_beats_the_file(tmp_path): + f = tmp_path / "budget.env" + f.write_text("MAM_JOB_TIMEOUT_S=50\n") + (tmp_path / "file").mkdir() + (tmp_path / "both").mkdir() + assert _dry_budget(_env(tmp_path, f), tmp_path / "file") == 50 + both = _env(tmp_path, f, {"MAM_JOB_TIMEOUT_S": "70"}) + assert _dry_budget(both, tmp_path / "both") == 70 + assert _python_values(both, ["MAM_JOB_TIMEOUT_S"]) == {"MAM_JOB_TIMEOUT_S": "70"} + assert _python_values(_env(tmp_path, f), ["MAM_JOB_TIMEOUT_S"]) == {"MAM_JOB_TIMEOUT_S": "50"} + + +# --------------------------------------------------------------------------- +# A4 - the parser sees the path only +# --------------------------------------------------------------------------- + +def test_a4_the_parser_command_line_carries_the_path_only(): + calls = [l for l in WRAPPER.read_text().splitlines() if "envfile.py" in l and not l.lstrip().startswith("#")] + assert len(calls) == 1, calls + assert re.search(r'envfile\.py" --shell "\$TARGET_ENV"\)"', calls[0]), calls[0] + src = WRAPPER.read_text() + assert not re.search(r'^\s*(source|\.)\s+"?\$TARGET_ENV', src, re.M), "the env file must never be sourced" + assert "set -a" not in src + + +# --------------------------------------------------------------------------- +# A5 - the files the repo ships or generates follow the format +# --------------------------------------------------------------------------- + +def _install_seed(tmp_path: Path) -> Path: + """The .mam.env install.sh writes, from its own heredoc run by bash.""" + src = (REPO_ROOT / "deploy" / "install.sh").read_text() + m = re.search(r'cat < "\$MAM_ENV"\n(.*?\nEOF)\n', src, re.S) + assert m, "install.sh no longer seeds .mam.env with a heredoc" + script = f'MAM_ENV="{tmp_path / "seed.env"}"; MAM_PORT=1883; MAM_CLIENT_PREFIX=hermes\ncat < "$MAM_ENV"\n{m.group(1)}\n' + subprocess.run(["bash", "-c", script], check=True, env={**os.environ, "HOME": str(tmp_path)}, timeout=30) + return tmp_path / "seed.env" + + +def test_a5_shipped_and_generated_env_files_parse_without_warnings(tmp_path): + ws = tmp_path / "ws" + (ws / "deploy").mkdir(parents=True) + (ws / ".mam.env.example").write_text((REPO_ROOT / ".mam.env.example").read_text()) + (ws / "deploy" / "generate-env.sh").write_text((REPO_ROOT / "deploy" / "generate-env.sh").read_text()) + subprocess.run(["bash", str(ws / "deploy" / "generate-env.sh")], check=True, capture_output=True, timeout=30) + for f in (REPO_ROOT / ".mam.env.example", ws / ".mam.env", _install_seed(tmp_path)): + pairs, warnings = envfile.parse(str(f)) + assert warnings == [], (f, warnings) + assert not any("$" in v or "`" in v for _, v in pairs), (f, pairs) + + # The template's commented assignments are what a user uncomments: they must + # parse too, and hold no shell syntax the parser would keep literally. + commented = tmp_path / "uncommented.env" + lines = [l[2:] for l in (REPO_ROOT / ".mam.env.example").read_text().splitlines() + if re.match(r"# [A-Za-z_][A-Za-z0-9_]*=", l)] + assert len(lines) > 20 + commented.write_text("\n".join(lines) + "\n") + pairs, warnings = envfile.parse(str(commented)) + assert warnings == [] and len(pairs) == len(lines) + assert not [(k, v) for k, v in pairs if "$" in v or "`" in v] + + +@pytest.mark.parametrize("missing", [True, False]) +def test_a5_shell_mode_exit_code_is_zero_for_absent_and_present_files(tmp_path, missing): + f = tmp_path / "x.env" + if not missing: + f.write_text("A=1\n") + res = subprocess.run([sys.executable, "-I", str(ENVFILE), "--shell", str(f)], capture_output=True, text=True, + env={k: v for k, v in os.environ.items() if k != "A"}, timeout=30) + assert res.returncode == 0 + assert res.stdout == ("" if missing else "export A=1\n")