multi-agent-mux/.agents/reports/reviewer-grok-01/report-fe93e3f2.md

Review: PR #43 (fix/issue-26) round 2 @ 0eec795

• Role: Reviewer (grok). Full range e1eb22b..0eec795 (this reviewer had no recorded round-1 verdict on the previous job).
• Identity: base e1eb22bd59ce825e3a22e86433c30f7ffd833810, head 0eec795cd1b46f7a28b9a1cc87ef8577619a731f, tree 584eac551f880232896b898cc43156f28ae1bacb, 7 commits, clean worktree.
• Verified at /var/folders/q_/68sxgw3j073dgjg8wl9s08vw0000gn/T/mam-review/loop-20261001T140030Z-34381f/1. Main stayed at e1eb22b. No product file was edited.
• git diff --check b58edac..HEAD is clean. That commit is 7 files, +128/−19: lib.sh, envfile.py, mqtt_common.py, the delegate-job wrapper, .mam.env.example, and the two issue-26 test files.

0eec795 closes the round-1 fail-open: a .mam.env that exists but cannot be read or decoded is EnvFileError, the --shell command exits 1, and the wrapper exits 1. NUL lines are skipped by the shared parser. mam_int uses ASCII whitespace and digits. An explicitly empty integer flag is refused.

1. Parser agreement

mqtt_common._parse_env_file (mqtt_common.py:98-104) and the wrapper both call envfile.apply / envfile.parse. One table, both loaders.

Rules in envfile.py:3-12 and .mam.env.example:14-25 match the code:

Input	Result
Blank line, # comment	skipped (envfile.py:64-65)
Surrounding ASCII space, one matching quote pair	stripped; internal spaces kept (envfile.py:80-83)
Mismatched quotes	kept
Extra =	value keeps every = after the first (envfile.py:69)
Empty value	kept
$(...), ${...}, backticks	literal
export KEY=...	skipped, invalid key (the key is export KEY)
Reserved bash variables	skipped with the name, never the value (envfile.py:77-79)
No =, or a key outside ^[A-Za-z_][A-Za-z0-9_]*$	skipped with the line number (envfile.py:71-76)
CRLF	splitlines() drops the CR
A line containing a NUL byte	skipped, {path}:{n}: skipped (NUL byte) (envfile.py:66-68). The warning has no value.
Whole-file OSError or UnicodeDecodeError	EnvFileError: {path}: cannot read ({type name}) (envfile.py:57-61), from None so the message carries no bytes from the file
Duplicate keys	both pairs returned; apply (envfile.py:91-94) sets the first, because the key is then already in the environment

export KEY= is what the old source accepted. Both loaders skip it, and the example says so (.mam.env.example:21-23). deploy/install.sh:513-530 is an unquoted heredoc, so $MAM_PORT, $MAM_CLIENT_PREFIX, and $HOME expand before the parser sees the file. Uncommented example assignments contain no $ or backticks (test_a5).

A leading BOM is outside the example. open(..., encoding="utf-8") does not strip it, so line 1 fails the key check and the next line loads. Both loaders do that together.

tests/test_issue26_envfile.py locks the two real loaders, including mode 000 and broken UTF-8 (:154-178, skipped only when euid is 0) and the NUL line (:183-193).

2. Wrapper eval

The wrapper runs python -I envfile.py --shell "$TARGET_ENV" and evals stdout (multi-agent-mux-delegate-job:37-45). The command substitution captures stdout only, so the parser's stderr stays visible. Every printed line is export NAME=<shlex.quote(value)> for a key that passed _KEY.fullmatch and is_reserved (envfile.py:74-83, 112-113). A value such as BASH_VERSINFO[$(touch marker)0] stays quoted text; the marker is not created (test_a6).

Failure:

• Missing interpreter: the substitution fails, the wrapper prints ERROR: cannot load <path> and exits 1 (test_a1_an_unreadable_parser_fails_loudly, :138-146).
• Unreadable or undecodable file: envfile.py prints ERROR: {path}: cannot read (PermissionError|UnicodeDecodeError) and returns 1 (envfile.py:107-109); the wrapper then prints ERROR: cannot load <path> (see the line above) and exits 1 (:43-44). The test requires both lines and requires the value to be absent from stderr.
• A missing file is still success with no exports (envfile.py:103-104, test_a5 :294-302). The wrapper only enters this path when [ -f "$TARGET_ENV" ].

Python entry points take the same read failure. _python_load asserts a non-zero exit, no LOADED, and EnvFileError in stderr.

3. Environment wins over the file

apply sets a key only when it is absent from the mapping (envfile.py:92-94). The shell side passes the process environment in, so a variable the wrapper already set, or one the caller exported, stays. Documented in envfile.py:10, the wrapper comment at :37-40, .mam.env.example:24, and MESSAGING.md:366-369.

Effect of leaving source (which overwrote the environment):

• The loop runner starts the wrapper with dict(os.environ) and sets DELEGATE_JOB_PYTHON to sys.executable when unset (runner.py:548-550), so the file cannot replace the runner's broker or interpreter.
• The remote runner already used the Python loader, which had the same precedence.
• Tests set MQTT_BROKER / MQTT_PORT / MQTT_TLS in the environment (conftest.py:1568-1575). The file can no longer point those back at another broker.
• A stale shell export of MQTT_* is no longer overwritten on each wrapper start. That is the intended precedence. install.sh still expands its seed before the file exists.

test_a3 (:230-238) checks file 50 against environment 70 on a real dry-run record and on the Python loader.

4. Integer rule

mam_int (lib.sh:2345-2358) builds its class from the six ASCII blanks $ ' \t\n\r\v\f ' and the digits 0123456789, strips leading zeros, forces base 10, and accepts min <= n <= 999999999. runner._positive_int (runner.py:71-81) strips the same six characters and requires isascii() and isdigit(). The expectation helper uses the same ASCII strip (test_issue26_ints.py:88-96).

Minimums in INT_KNOBS (:35-54):

• Minimum 1 (0 would close or idle out immediately): MAM_JOB_TIMEOUT_S 3600, MAM_HEARTBEAT_SEC 30, MAM_HEARTBEAT_MISSES 4, MAM_ACK_TIMEOUT_S 180, and the five flags --timeout, --idle-timeout, --heartbeat, --heartbeat-misses, --ack-timeout.
• Minimum 0 (0 means check once or do not wait): MAM_INJECT_ACK_GRACE_S, delegate_wait_started's ack_s (lib.sh:2384), SKS_DIALOG_TIMEOUT 30, SKS_BUSY_TIMEOUT 120, SKS_DELIVERY_GRACE_S 15, SKS_QUIESCENT_TRIES 20.

An invalid environment value warns and uses the default (mam_int_knob, lib.sh:2364-2369). An invalid flag exits 1. parse_opts records that the flag was present (:167-179) and refuses an empty value (:199-203); the default applies only when the flag is absent. The space-padded token keeps HEARTBEAT_SEC distinct from HEARTBEAT_MISSES. test_b2 includes the explicit empty string among the refused values (:228-232).

Locale: test_b1_unicode_blanks_and_digits_are_invalid_in_every_locale (:188-204) runs U+2003, U+00A0, U+3000, and Arabic and fullwidth digits under C and en_US.UTF-8, through mam_int, _positive_int, the wrapper record, and the runner. All four reject them and keep the 3600 default.

Three runner settings stay on their own Python conversions and do not reach bash arithmetic: MAM_WAIT_GRACE_S (runner.py:627-628, isdigit then int), MAM_CHILD_WAIT_S (:748-749, float), MAM_JOB_TREE_MAX (:752, int). They predate this integer rule. NOT_ARITHMETIC classifies the shell defaults that use [ -ge ] / [ -le ] / sleep / an argv, and on this host's bash 3.2 those [ comparisons are decimal.

5. Local MQTT broker

tests/_local_mqtt.py is an in-process MQTT 3.1.1 broker on 127.0.0.1:0, daemon accept and connection threads (:103-104, :139), stopped by closing the listen socket and the live connections (:107-120). Scope is what the repo's clients use: protocol level 4, QoS 0/1, retained messages. QoS 2 closes the connection (:190-191).

A PUBLISH is routed and, when retained, stored before PUBACK (:196-200). The session fixture (conftest.py:1568-1584) sets MQTT_BROKER / MQTT_PORT / MQTT_TLS and drops MQTT_HOST and the credential and certificate variables for the session, then restores them. test_c0 waits for CONNACK, SUBACK, UNSUBACK, and PUBACK before it reads broker state, and it checks topic, payload, QoS, and the retain flag (test_local_mqtt.py:100-130). test_c0_holds_when_the_broker_is_slow (:140-160) delays CONNECT, UNSUBSCRIBE, and routing so a test that reads state before the ack fails. test_c2 (:297-310) rejects the public-broker literal outside an allowlist and rejects a helper that drops MQTT_BROKER / MQTT_HOST without naming a replacement. test_c0_publish_event_reaches_job_subscriber_over_the_local_broker uses a uuid4 topic prefix.

Commit 2826e70 lists the moved cases: issue 18 acceptance 1/2/3/5, issue 6 t1_1/2/7/8/14/15/16/20, issue 19's two MQTT-loop cases, issue 33 b3/r3/b7, issue 30 s9/s10/r2, and calls that set 22. Those harnesses inject local_broker_env() and the "broker unreachable" skips are gone. 0eec795 does not touch the broker or those tests. This round's required run includes tests/test_local_mqtt.py and tests/test_issue18_liveness_contract.py; both are in the 174 passes below. Issue 6, 19, 30, and 33 were outside this brief's command.

6. The baseline-failure claim

tests/test_issue10_13_purge.py is identical to e1eb22b (git diff empty). On this host, with HOME and --basetemp under /tmp and with MQTT variables unset:

23 passed in 11.62s

The claim that a full-suite run shows 13 failures of which 12 are this host's baseline in that file does not reproduce here. The tests use tmp_path sandboxes and uuid4 conversation ids. Nothing under the real home was purged.

7. Design boundary

The shell diff adds numeric parsing for clocks that already existed: mam_int / mam_int_knob on the job timeout, heartbeat, ack, inject grace, and the SKS dialog, busy, delivery, and quiescent counts (lib.sh around the $(( $(date +%s) + $(mam_int_knob SKS_...) )) deadlines). That is the decimal-integer rule, so 0700 is 700 and 0900 does not abort arithmetic.

No new bash judgment of a timeout label, a budget extension, or a stall close landed in this range. MAM_STALL_WATCH stays a string compare (NOT_ARITHMETIC). Closing remains the existing clocks. Nothing here implements #30 or #33, and nothing needs to move back to the planner.

Should Python entry points raise?

Yes. A file that exists and cannot be read must stop the process. Warning and continuing would load an empty configuration and, when MQTT_* are unset, the public default broker. That is the fail-open 0eec795 removes, and it has to be the same in both loaders. publish_event.main calls ensure_env_loaded() after argument parsing and does not catch the exception (publish_event.py:337); the same call is the first environment step in registry.main, job_subscriber.main, cli.main, bench.main, and reconcile's embedded subscriber (mqtt_common.py:113-115). An agent whose .mam.env is unreadable then cannot publish started or completed, and the job closes on the clocks. That is the same outcome as refusing to run unconfigured.

The wrapper prints one ERROR line and exits 1. The Python process exits with a traceback that names EnvFileError and the path and the exception type, and does not include the file body. That difference is acceptable. _env_loaded = True is set before _load_dotenv (mqtt_common.py:123-124), so a caller that caught the exception would not retry in-process. The entry points above do not catch it.

Checks

From the review snapshot, MQTT variables unset, no .mam.env exported, AGENT_PYTHON_BIN and the interpreter the pr-43 venv, --basetemp under /tmp:

174 passed in 208.02s — tests/test_issue26_envfile.py, tests/test_issue26_ints.py, tests/test_local_mqtt.py, tests/test_issue18_liveness_contract.py, tests/test_tier1_unit.py.

The full suite is the orchestrator's.

Notes (non-blocking)

• Duplicate keys in one file are first-wins (envfile.py:91-94). The old source was last-wins. Both loaders agree. The example states environment-over-file and does not mention a repeated key inside the file.
• A # comment that also contains a NUL is skipped as a comment (envfile.py:64-65 runs before the NUL check). A value line with a NUL still warns and is skipped.
• MESSAGING.md:369 still says a missing explicit MAM_ENV_FILE logs an error and connections fail closed with RuntimeError. mqtt_common.py:51-54 returns without loading when that path is not a regular file, and this PR does not change that branch. A file that exists and cannot be read now raises, which is the contract under review.
• MAM_WAIT_GRACE_S, MAM_CHILD_WAIT_S, and MAM_JOB_TREE_MAX remain outside _positive_int (section 4). They do not reach bash arithmetic.

[VERDICT: PASS]