From 71ea8095c515b01c872277b664f45b2abf0b2e20 Mon Sep 17 00:00:00 2001 From: Godopu Date: Fri, 2 Oct 2026 08:10:42 +0900 Subject: [PATCH] fix(followups): F3 - .mam.env BOM, repeated keys and a missing MAM_ENV_FILE PR #43 review (grok): (a) A UTF-8 byte-order mark at the start of the file made the first line an "invalid key", so its setting was dropped. The one parser (envfile.py, used by the wrapper and mqtt_common) now reads with utf-8-sig. (b) A key given twice keeps its first value (a later line does not replace it). The behaviour is unchanged; .mam.env.example's FORMAT section and envfile.py's docstring now say so. (c) MESSAGING.md said a MAM_ENV_FILE naming no file logs an error and makes connections fail closed. Both loaders skip it silently and refuse the ambient search; a file that exists but cannot be read is the error (EnvFileError, one ERROR line and rc 1). The text now says that. Tests A7: BOM in both loaders, first-value-wins in both loaders plus the FORMAT text, and a missing explicit file loading neither it nor the ambient .mam.env without an error plus the MESSAGING text (all three fail at 0051fd6). Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_01HCGpJXhSZasJLU38ksDghe --- .agents/skills/lib_py/delegation/envfile.py | 7 +++- .mam.env.example | 2 + MESSAGING.md | 2 +- tests/test_issue26_envfile.py | 44 +++++++++++++++++++++ 4 files changed, 52 insertions(+), 3 deletions(-) diff --git a/.agents/skills/lib_py/delegation/envfile.py b/.agents/skills/lib_py/delegation/envfile.py index 6efbefb..fff4c28 100644 --- a/.agents/skills/lib_py/delegation/envfile.py +++ b/.agents/skills/lib_py/delegation/envfile.py @@ -7,7 +7,9 @@ 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). 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 +A UTF-8 byte-order mark at the start of the file is dropped. Variables +already in the environment win over the file, and a key given twice keeps its +first value. 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. @@ -55,7 +57,8 @@ def parse(path: str) -> Tuple[List[Tuple[str, str]], List[str]]: pairs: List[Tuple[str, str]] = [] warnings: List[str] = [] try: - with open(path, "r", encoding="utf-8") as f: + # utf-8-sig: a byte-order mark at the start is not part of the first key + with open(path, "r", encoding="utf-8-sig") as f: lines = f.read().splitlines() except (OSError, UnicodeDecodeError) as e: raise EnvFileError(f"{path}: cannot read ({type(e).__name__})") from None diff --git a/.mam.env.example b/.mam.env.example index 02d6939..92f428b 100644 --- a/.mam.env.example +++ b/.mam.env.example @@ -21,7 +21,9 @@ # - `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 UTF-8 byte-order mark at the start of the file is ignored; # - a variable already set in the environment wins over this file; +# - a key given twice keeps its FIRST value (a later line does not replace it); # - 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 diff --git a/MESSAGING.md b/MESSAGING.md index f6155ea..ada0492 100644 --- a/MESSAGING.md +++ b/MESSAGING.md @@ -366,7 +366,7 @@ The subscriber acts as the central execution watchdog: * **`.mam.env` Resolution Hierarchy (`_load_dotenv`)**: Configuration files are resolved with strict precedence rules: 1. **OS Environment Precedence**: Any variable already defined in `os.environ` is preserved and never overwritten by file-based configs. - 2. **Explicit Override (`MAM_ENV_FILE`)**: If `MAM_ENV_FILE` is set, only that specific file is parsed. If the specified file does not exist, an error is logged and ambient search is refused (preventing silent fallback to unintended parent configs). Connection attempts fail-closed (`RuntimeError`). + 2. **Explicit Override (`MAM_ENV_FILE`)**: If `MAM_ENV_FILE` is set, only that specific file is parsed. If the specified file does not exist, it is skipped silently and ambient search is refused (no fallback to unintended parent configs): the process runs on its environment and the defaults. If it exists but cannot be read or decoded, that is an error (`EnvFileError`): the wrapper and the Python entry points print one `ERROR: : cannot read ()` line and exit 1. 3. **Workspace Root Auto-Discovery**: If `MAM_ENV_FILE` is not set, the resolver searches candidate paths in order: `MAM_REAL_ROOT`, `WORKSPACE_ROOT`, upward directory walk searching for `.agents` or `.git` boundary markers, and `os.getcwd()`. 4. **Public Broker Security Alert (B-17)**: If the final resolved host falls back to the public sandbox `broker.hivemq.com`, a prominent security warning is emitted. * **Broker Config Resolution (`broker_config_from_job`)**: diff --git a/tests/test_issue26_envfile.py b/tests/test_issue26_envfile.py index f7b6c76..4017832 100644 --- a/tests/test_issue26_envfile.py +++ b/tests/test_issue26_envfile.py @@ -405,3 +405,47 @@ def test_a6_the_file_cannot_replace_the_wrappers_own_variables(tmp_path): assert not (tmp_path / "pwned-lib").exists() got = _wrapper_values(_env(tmp_path, f), ["SCRIPT_DIR", "TARGET_ENV"], tmp_path) assert _split(got.stdout) == {"SCRIPT_DIR": str(WRAPPER.parent), "TARGET_ENV": str(f)} + + +# --------------------------------------------------------------------------- +# A7 - follow-ups F3 (PR #43 review grok) +# --------------------------------------------------------------------------- + +def test_a7_a_leading_bom_is_not_part_of_the_first_key(tmp_path): + """An editor's byte-order mark used to turn the first line into + "invalid key" and drop the first setting, in both loaders.""" + f = tmp_path / "bom.env" + f.write_bytes(b"\xef\xbb\xbfFIRST=one\nSECOND=two\n") + env = _env(tmp_path, f) + res = _wrapper_values(env, ["FIRST", "SECOND"], tmp_path) + assert res.returncode == 0, res.stderr + assert _split(res.stdout) == {"FIRST": "one", "SECOND": "two"} + assert "skipped" not in res.stderr, res.stderr + assert _python_values(env, ["FIRST", "SECOND"]) == {"FIRST": "one", "SECOND": "two"} + assert envfile.parse(str(f)) == ([("FIRST", "one"), ("SECOND", "two")], []) + + +def test_a7_a_repeated_key_keeps_its_first_value_as_documented(tmp_path): + f = tmp_path / "dup.env" + f.write_text("DUP=first\nDUP=second\n") + env = _env(tmp_path, f) + res = _wrapper_values(env, ["DUP"], tmp_path) + assert res.returncode == 0, res.stderr + assert _split(res.stdout) == {"DUP": "first"} == _python_values(env, ["DUP"]) + fmt = (REPO_ROOT / ".mam.env.example").read_text().split("# SECURITY:")[0] + assert "a key given twice keeps its FIRST value" in fmt, "document it in the FORMAT section" + + +def test_a7_a_missing_explicit_file_loads_nothing_and_says_nothing(tmp_path): + """MAM_ENV_FILE naming no file: no error, and no fallback to the file the + ambient search would find - in both loaders, and so in MESSAGING.md.""" + (tmp_path / ".mam.env").write_text("PROBE_AMBIENT=found\n") + absent = tmp_path / "absent.env" + env = _env(tmp_path, absent) + res = _wrapper_values(env, ["PROBE_AMBIENT"], tmp_path) + assert res.returncode == 0, res.stderr + assert _split(res.stdout) == {"PROBE_AMBIENT": ""} + assert "ERROR" not in res.stderr and str(absent) not in res.stderr, res.stderr + assert _python_values(env, ["PROBE_AMBIENT"]) == {"PROBE_AMBIENT": ""} + doc = next(l for l in (REPO_ROOT / "MESSAGING.md").read_text().splitlines() if "Explicit Override (`MAM_ENV_FILE`)" in l) + assert "does not exist, it is skipped silently" in doc and "an error is logged" not in doc, doc