fix(issue-26): review - an unreadable env file is an error, NUL lines are skipped, integers are ASCII in every locale, empty flags are refused
Merge-workspace review of PR #43 (codex R1-R4, claude R1/R2).
- R1: envfile.parse raises EnvFileError when the file exists but cannot be
read or decoded as a whole; `--shell` prints the cause and exits 1, so the
wrapper stops (rc 1, as before #26) instead of running on defaults, and
mqtt_common's entry points stop on the same error. A bad single line is
still a warning. The wrapper's message no longer claims a read failure
when the cause is elsewhere ("cannot load ... see the line above").
- R2: lib.sh mam_int spells out the ASCII blanks and digits instead of
[[:space:]]/[0-9], which follow the locale: U+2003 30 U+2003 was 30 in the
wrapper and invalid in the runner under en_US.UTF-8. The test oracle
_expected uses the same ASCII rule.
- R3: an integer flag given explicitly with an empty value is refused like
any other invalid value; only an absent flag keeps its default.
- R4: a line holding a NUL byte is skipped with a warning by both loaders;
before, the wrapper read "leftright" and the Python loader died.
- .mam.env.example states the format: no expansion, `export` and bash's own
variables skipped, a trailing `# ...` is part of the value, the environment
wins, an unreadable file is an error.
Tests: unreadable (0000) and invalid-UTF-8 files stop the wrapper and the
Python loader; a NUL line is skipped by both; Unicode blanks/digits are
invalid for mam_int, _positive_int, the wrapper and the runner under C and
en_US.UTF-8; explicit empty values for the five integer flags are refused.
All of them fail on b58edac.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01VCgu9Fa9iT7TbJUSzdq4Ff
This commit is contained in:
1 parent
b58edace11
commit
0eec795cd1
7 files changed
+128
-19
No files matched your search
@@ -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": "<unset>", "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": "<unset>", "AFTER": "yes"}
|
||||
|
||||
|
||||
# ---------------------------------------------------------------------------
|
||||
|
||||
Reference in new issue
Block a user