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
97 lines
10 KiB
Markdown
97 lines
10 KiB
Markdown
# Review: PR #43 (fix/issue-26) @ b58edac
|
|
|
|
- Role: Reviewer (grok)
|
|
- Identity: base `e1eb22bd59ce825e3a22e86433c30f7ffd833810`, head `b58edace115205500c1197d9ce59cb5c6d724893`, tree `14b670fbfc82dc9179cab43cf53eef920a3d6ffa`
|
|
- Verified at `/var/folders/q_/68sxgw3j073dgjg8wl9s08vw0000gn/T/mam-review/loop-20261001T131705Z-a60092/1` (the assigned directory was gone at the start of this pass; the same commit was checked out there again, and `HEAD` / `HEAD^{tree}` match the brief)
|
|
- `git diff --check e1eb22b...HEAD` is clean. Main stayed at `e1eb22b`. No product file was edited.
|
|
|
|
The two loaders share `envfile.py`, the integer rule matches on the knobs that reach bash arithmetic, and the in-process broker covers the client paths this repo uses. One failure mode is open: a `.mam.env` that exists and cannot be read is a warning, and the wrapper keeps going.
|
|
|
|
## 1. Parser
|
|
|
|
`mqtt_common._parse_env_file` (`mqtt_common.py:98-101`) and the wrapper both go through `envfile.parse` / `envfile.apply`. The same input therefore yields the same pairs.
|
|
|
|
Checked against the rules in `envfile.py:3-9` and a direct `parse` of temporary files (no workspace `.mam.env` was read):
|
|
|
|
| Input | Result |
|
|
|---|---|
|
|
| CRLF | `A=1`, `B=two` after quote removal; no warning |
|
|
| One matching quote pair | `" spaced "` becomes ` spaced ` (internal spaces kept) |
|
|
| Mismatched quotes | `"a'` stays `"a'` |
|
|
| Extra `=` | `E=a=b=c` keeps value `a=b=c`; `EMPTY=` is `""` |
|
|
| `$(...)`, `${...}`, backticks | kept as literal text |
|
|
| `export MQTT_PORT=1883` | skipped, warning `invalid key` (the key is `export MQTT_PORT`) |
|
|
| BOM on the first line | that line is skipped (`invalid key`); the next line loads |
|
|
| Line with no `=` / a key that fails `^[A-Za-z_][A-Za-z0-9_]*$` | skipped with the line number; the value is absent from the warning (`envfile.py:60-65`) |
|
|
| Duplicate `A=1` then `A=2` | both pairs are returned; `apply` (`envfile.py:80-83`) sets the first and leaves the second, because the key is then already in the environment |
|
|
|
|
`export KEY=` is the shell prefix the old `source` accepted. Both loaders now skip it. That matches the shipped files: `.mam.env.example` assignments are `KEY=value`, and `deploy/install.sh:513` is an unquoted heredoc, so `$MAM_PORT`, `$MAM_CLIENT_PREFIX`, and `$HOME` (`install.sh:520-526`) are expanded by bash before the parser sees the file. Uncommented example lines contain no `$` or backticks. A leading BOM is not in the example; both loaders skip that first key together.
|
|
|
|
`tests/test_issue26_envfile.py` (the two real loaders, reserved-variable set, and `shlex` quoting) passed inside the run below.
|
|
|
|
## 2. Wrapper eval
|
|
|
|
The wrapper runs `python -I envfile.py --shell "$TARGET_ENV"` and evals stdout (`multi-agent-mux-delegate-job:41-45`). `-I` is safe: the module is stdlib only (`os`, `re`, `shlex`, `sys`, `typing`), and `python -I` on the PR 4.3 venv imports that stdlib. Every printed line is `export NAME=<shlex.quote(value)>` for a key that already passed `_KEY.fullmatch` and `is_reserved` (`envfile.py:62-67`, `97-98`). A probe value `BASH_VERSINFO[$(touch …)0]` came back single-quoted; the marker file was not created, and the warning did not contain `touch`.
|
|
|
|
Missing interpreter: rc 1, stderr `ERROR: cannot read <path>`, usage text absent. That matches `test_a1_an_unreadable_parser_fails_loudly` (`tests/test_issue26_envfile.py:139-146`).
|
|
|
|
Unreadable file, this pass: mode `000` regular file, `MAM_ENV_FILE` pointed at it, `bash wrapper --help`.
|
|
|
|
- `envfile.parse` catches `OSError` and `UnicodeDecodeError` and returns a warning (`envfile.py:52-53`).
|
|
- `main` prints that warning and returns 0 (`envfile.py:94-99`).
|
|
- The wrapper's `|| exit 1` (`multi-agent-mux-delegate-job:44`) runs only when that process is non-zero.
|
|
- Observed: rc 0, stderr `WARNING: <file>: cannot read (PermissionError)`, stdout still contains `submit`.
|
|
|
|
The wrapper is `set -euo pipefail` (`multi-agent-mux-delegate-job:15`). The previous body was `set -a; source "$TARGET_ENV"`, so a permission error ended the process. The new path loads nothing from that file and continues. With no broker variables already in the environment, later clients use the product default broker.
|
|
|
|
`test_a1_an_unreadable_parser_fails_loudly` covers a missing `DELEGATE_JOB_PYTHON` only. It does not cover a file `open` that raises.
|
|
|
|
Alternative: when `parse` hits `OSError` or `UnicodeDecodeError`, `main` returns 1 (the warning text can stay). The wrapper already treats a non-zero parser as `ERROR: cannot read` and exits 1. Per-line skips stay warnings and exit 0. `mqtt_common._parse_env_file` calls `apply` and ignores the CLI status, so the Python loader's warning log stays as it is.
|
|
|
|
## 3. Precedence
|
|
|
|
A variable already in the environment is left alone (`envfile.py:9`, `envfile.py:80-83`, wrapper comment at `multi-agent-mux-delegate-job:37-40`). `MESSAGING.md:366-368` already stated that for the Python loader. The wrapper used to overwrite it on every `source`. Callers that need the file's value must leave the variable unset.
|
|
|
|
- Loop: `LoopRunner.delegate_job` passes `dict(os.environ)` (`runner.py:548-557`). Keys the loop process already loaded stay for the whole process. A process that has not set them still receives the file.
|
|
- Worktree create adds `MAM_ENV_FILE` pointing at the main checkout's file and does not itself export `MQTT_*` (`worktree_prepare.sh`).
|
|
- The remote runner loads through `mqtt_common`, which already used this precedence (`mqtt_common.py:44-46`).
|
|
- The test suite depends on the new order: the session broker is injected with `MQTT_BROKER` / `MQTT_PORT` / `MQTT_TLS`, and a `.mam.env` on disk must not replace them (`tests/conftest.py:1568-1575`).
|
|
|
|
`.mam.env.example` does not say that an already-exported variable wins. The loader docstring and the wrapper comment do.
|
|
|
|
## 4. Integers
|
|
|
|
`mam_int` / `mam_int_knob` (`lib.sh:2345-2365`) and `_positive_int` (`runner.py:71-81`) share one rule: surrounding blanks, digits only, decimal (`10#` in bash, leading zeros stripped before `int` in Python), at most 9 significant digits, then a per-knob minimum. Invalid environment values warn and use the default. Invalid wrapper flags exit 1 (`multi-agent-mux-delegate-job:194-199`).
|
|
|
|
On this host's bash 3.2, `$((0700))` is 448 and `$((0900))` errors, while `[ 700 -eq 0700 ]` is true. The `NOT_ARITHMETIC` rows that say `[ -ge ] (decimal)` match that `[` builtin. The knobs that reach `$(( ))` or `(( ))` in the wrapper and `lib.sh` go through `mam_int`: job budget, heartbeat, misses, ack timeout, inject grace, and the `SKS_*` deadlines including `SKS_QUIESCENT_TRIES` at the `send_keys_safe` call. Minimum 0 is the "check once / do not wait" settings (`MAM_INJECT_ACK_GRACE_S`, `delegate_wait_started`'s ack, `SKS_DIALOG_TIMEOUT`, `SKS_BUSY_TIMEOUT`, `SKS_DELIVERY_GRACE_S`, `SKS_QUIESCENT_TRIES`). Minimum 1 is the budget and the liveness counts, where 0 would close a job immediately.
|
|
|
|
`MAM_WAIT_GRACE_S` (`runner.py:627-628`), `MAM_CHILD_WAIT_S` (`runner.py:748-749`), and `MAM_JOB_TREE_MAX` (`runner.py:752`) still use `isdigit`/`int`/`float`. They stay in Python, so they never hit bash octal. This diff does not claim to move them onto `_positive_int`. `MAM_CHILD_WAIT_S=abc` still raises `ValueError` inside `wait_for_descendants`. That is unchanged behavior, and it is outside the bash-arithmetic fix.
|
|
|
|
## 5. Local broker
|
|
|
|
`tests/_local_mqtt.py` listens on `127.0.0.1:0` with daemon threads and closes the listen socket and connections in `stop`. The session fixture (`tests/conftest.py:1568-1584`) points `MQTT_BROKER` / `MQTT_PORT` / `MQTT_TLS` at it and drops the auth and certificate variables for the session, restoring them afterwards.
|
|
|
|
What the repo's paho 3.1.1 clients need is implemented and checked by `test_c0_make_client_speaks_3_1_1_wildcards_qos1_and_retained`: CONNECT levels 3 and 4 with CONNACK, `+` and `#` subscriptions, QoS 1 PUBACK only after `_route` (`tests/_local_mqtt.py:196-199`), retained replay to a late subscriber, delete on an empty retained payload, and UNSUBSCRIBE. `test_c0_holds_when_the_broker_is_slow` delays CONNECT, UNSUBSCRIBE, and route so a test that reads broker state before the ack fails. QoS 2 is refused. There is no TLS, no auth check, no persistent session, and no QoS 1 redelivery; the module docstring says so, and the product paths under test do not use those.
|
|
|
|
The moved tests keep their product assertions. acceptance1 and acceptance5 also require the subscriber log, and acceptance5 requires the runner log plus `dispatch_verified` / `dispatch_fingerprint` on the audit row. The "public broker did not deliver" skips on those paths are gone (`test_issue18_liveness_contract.py`, `test_issue19_subscriber_daemon_lifecycle.py:417-424`). `test_c2_no_test_names_the_public_broker_or_strips_the_local_one` scans `tests/*.py`. Caller-supplied `MQTT_PORT` still overrides the fixture, so `test_t1_4_broker_unavailable_fail_closed` can still name an unreachable port.
|
|
|
|
Ran without exporting `.mam.env`: the 10 broker tests from issues #6 and #19 that are outside the brief's file list, 10 passed in 100.83s.
|
|
|
|
## 6. The "12 baseline purge failures" claim
|
|
|
|
`tests/test_issue10_13_purge.py` is not in `e1eb22b...HEAD`. Run alone on this host, `HOME` set to a temporary directory, session-id variables unset: 23 passed in 12.69s. The claim that 12 of 13 failures are this file's baseline on this host does not reproduce. The full suite (the brief's ~75 minutes) was left to the orchestrator, so this pass does not confirm or deny a separate total of 13.
|
|
|
|
## 7. Design boundary
|
|
|
|
`mam_int` reads a decimal for bash arithmetic. It does not choose `deadline_timeout` versus `wall_clock_timeout`, extend a budget, or decide a stall. Stall detection stays in `stall_watch.py` and publishes `progress` only. Closing stays on the existing clocks. The new bash is the reader plus `eval` of quoted `export` lines.
|
|
|
|
## Tests
|
|
|
|
From the review tree, with `AGENT_PYTHON_BIN` set to the PR `.venv` and `MQTT_*` / `MAM_ENV_FILE` unset in the parent (the session fixture starts the local broker):
|
|
|
|
`tests/test_issue26_envfile.py tests/test_issue26_ints.py tests/test_local_mqtt.py tests/test_issue18_liveness_contract.py tests/test_issue33_budget.py tests/test_issue30_stall_watch.py tests/test_tier1_unit.py`
|
|
|
|
228 passed, 1 skipped, 529.36s. Plus the 10 broker tests in section 5.
|
|
|
|
[VERDICT: NOT PASS]
|