GodopuandClaude Opus 5.5
2e90c809d3
Issue #26: one .mam.env parser for the wrapper and the Python tools, decimal integer knobs, tests on a local MQTT broker. Merge-workspace review: round 1 (@b58edac) claude agy PASS, codex NOT PASS (R1-R4), grok no verdict (its late report, archived as report-c9ed5b14.md, found R1 too); round 2 (@0eec795) 4/4 PASS. Reports and round-2 identity sidecars go to .agents/reports/<session>/. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01VCgu9Fa9iT7TbJUSzdq4Ff
122 lines
13 KiB
Markdown
122 lines
13 KiB
Markdown
# 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]
|