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

.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

.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).
+warning naming the line (never the value — the file holds secrets). A line
-Variables already in the environment win over the file.
+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 <path>``) 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
+    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:

.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)
 

.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;;
+      --timeout) TIMEOUT="$2"; _INT_FLAGS_GIVEN+=" TIMEOUT"; shift 2;;
-      --idle-timeout) IDLE_TIMEOUT="$2"; shift 2;;
+      --idle-timeout) IDLE_TIMEOUT="$2"; _INT_FLAGS_GIVEN+=" IDLE_TIMEOUT"; shift 2;;
-      --heartbeat) HEARTBEAT_SEC="$2"; shift 2;;
+      --heartbeat) HEARTBEAT_SEC="$2"; _INT_FLAGS_GIVEN+=" HEARTBEAT_SEC"; shift 2;;
-      --heartbeat-misses) HEARTBEAT_MISSES="$2"; shift 2;;
+      --heartbeat-misses) HEARTBEAT_MISSES="$2"; _INT_FLAGS_GIVEN+=" HEARTBEAT_MISSES"; shift 2;;
-      --ack-timeout) ACK_TIMEOUT="$2"; 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

.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:

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": "<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"}
 
 
 # ---------------------------------------------------------------------------

tests/test_issue26_ints.py

@@ -86,8 +86,8 @@ NOT_ARITHMETIC = {
 
 
 def _expected(value: str, minimum: int):
-    s = value.strip()
+    s = value.strip(" \t\n\r\v\f")  # ASCII blanks only, in every locale (review R2)
-    if not s.isdigit():
+    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:]