GodopuandClaude Opus 5.5
3563a6fc17
PR #41/#43 review follow-ups F1-F6, implemented by creator-claude-01 in a
loop and reviewed by claude, agy, grok and codex: round 1 (@95eabc6)
codex NOT PASS (a failed env load cached as loaded), fixed in 75f364e;
round 2 4/4 PASS. Full suite at 75f364e 1380 passed / 0 failed. Reviewer
reports (round 2 with identity sidecars) and the creator's reports go to
.agents/reports/<session>/.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01VCgu9Fa9iT7TbJUSzdq4Ff
111 lines
8.6 KiB
Markdown
111 lines
8.6 KiB
Markdown
# Code Review Report: PR #41 / PR #43 Follow-ups (`fix/followups-41-43`)
|
||
|
||
- **Job ID**: `dd9ea82f`
|
||
- **Target / Session**: `agy` (`herdr:reviewer-agy-01`)
|
||
- **Role**: Reviewer
|
||
- **Review Identity**:
|
||
- `loop_id`: `loop-20261001T230324Z-b511bf`
|
||
- `base`: `0051fd6c3b088f8b265928ccd94ea31053fdde7e` (main, HEAD at loop start)
|
||
- `head`: `95eabc693f7346f491592b053487bdcf0acd5f4f`
|
||
- `tree`: `46399844fa63d62d97f7b52ee35010f2fa1b7998`
|
||
- Verification Path: `/var/folders/q_/68sxgw3j073dgjg8wl9s08vw0000gn/T/mam-review/loop-20261001T230324Z-b511bf/1`
|
||
|
||
---
|
||
|
||
## 1. Executive Summary
|
||
|
||
This review audits the follow-up changes addressing residual items from PR #41 (v4.8.0) and PR #43 (v4.8.1).
|
||
The changes encompass 8 atomic commits on branch `fix/followups-41-43`, addressing items **F1** through **F6**.
|
||
|
||
All 6 items are implemented surgically, verified with deterministic regression tests, and maintain clean separation of concerns without introducing out-of-scope changes:
|
||
- **F1**: Resolved the date-dependent `ps -o lstart=` double-space padding mismatch in `test_issue21_acceptance.py`.
|
||
- **F2**: Standardized Python entry point error reporting for unreadable `.mam.env` files to print a single `ERROR:` line with exit code 1 (no traceback or credential leaks), without library code invoking `sys.exit`.
|
||
- **F3**: Added UTF-8 BOM (`utf-8-sig`) handling, documented first-value precedence for duplicate keys in `.mam.env.example`, and aligned `MESSAGING.md` documentation with actual file search semantics.
|
||
- **F4**: Unified integer knobs across Python and Bash (`lib_int.sh`), eliminating `ValueError` and shell arithmetic syntax errors on non-integer inputs, while preserving fractional seconds where appropriate (`SKS_QUIESCENT_INTERVAL`).
|
||
- **F5**: Removed the `MAM_TEST_UNRELIABLE_AGENT` test switch from production code in `stall_watch.py` and converted `test_s13` to mock adapter properties dynamically in the test process.
|
||
- **F6**: Clarified herdr wait bounds in `SKILL.md` and hardened the mid-wait wrapper termination test in `test_issue30_stall_watch_stop.py`.
|
||
|
||
All relevant test suites executed in the isolated verification path passed 100%.
|
||
|
||
---
|
||
|
||
## 2. Item-by-Item Detailed Audit
|
||
|
||
### 2.1. F1: Date-dependent Test Bug (`test_issue21_acceptance.py`)
|
||
- **Root Cause**: `ps -p <pid> -o lstart=` pads single-digit day numbers with a leading space (e.g. `"Oct 1"`), whereas `loop_lock.sh`'s `mam_lstart` compresses whitespace using `tr -s ' '` (`"Oct 1"`). When `test_a8_second_loop_is_refused_before_a_scheduler_exists` wrote raw `ps` output to the lock marker on days 1–9 of any month, the strings diverged, causing false lock staleness detections.
|
||
- **Implementation**:
|
||
- `tests/test_issue21_acceptance.py#L549-L554`: Added `_marker_lstart(pid, env)` which normalizes `lstart` via `" ".join(out.split())` matching `mam_lstart`.
|
||
- Added `_PADDED_PS` shim (`#L541-L547`) to test both `"real"` and forced `"padded"` scenarios parametrically (`#L556-L587`).
|
||
- Production code `loop_lock.sh` was appropriately left untouched.
|
||
|
||
### 2.2. F2: Python Entry Points' Single-Line `EnvFileError` Output
|
||
- **Implementation**:
|
||
- Added `load_env_or_report() -> bool` in `lib_py/delegation/mqtt_common.py#L279-L290`. When `EnvFileError` is caught, it prints `ERROR: {e}` to `sys.stderr` and returns `False`.
|
||
- Process entry points (`publish_event.py`, `registry.py`, `job_subscriber.py`, `cli.py`, `bench.py`, `heartbeat.py`, `remote.py`) call `load_env_or_report()` and exit with code 1 (`RC_ERROR` in `job_subscriber.py`).
|
||
- Library functions (`ensure_env_loaded()`, `_parse_env_file()`) never call `sys.exit()` directly.
|
||
- Secret safety: Neither file contents nor variable values are printed.
|
||
- Test coverage: `tests/test_issue26_envfile.py#L606-L645` (`test_a1_python_entry_points_report_an_unreadable_file_in_one_line`) tests all 7 modules under permission denial (`chmod 000`) and invalid UTF-8 decoding, verifying exact error format and absence of tracebacks.
|
||
|
||
### 2.3. F3: `.mam.env` Parser & Documentation Refinements
|
||
- **Implementation**:
|
||
- (a) **UTF-8 BOM**: `lib_py/delegation/envfile.py#L215` opens files with `encoding="utf-8-sig"`, automatically stripping any leading BOM (`\xef\xbb\xbf`) so the first key is not rejected as invalid. Verified by `test_a7_a_leading_bom_is_not_part_of_the_first_key`.
|
||
- (b) **Duplicate Keys**: Added explicit documentation in `.mam.env.example#L492`: `"a key given twice keeps its FIRST value (a later line does not replace it);"`. Verified by `test_a7_a_repeated_key_keeps_its_first_value_as_documented`.
|
||
- (c) **Documentation Alignment**: Updated `MESSAGING.md#L505` to describe actual behavior: non-existent `MAM_ENV_FILE` is skipped silently (falling back to environment and defaults, without ambient parent directory walk), while an unreadable file raises `EnvFileError` and exits 1. Verified by `test_a7_a_missing_explicit_file_loads_nothing_and_says_nothing`.
|
||
|
||
### 2.4. F4: Integer Knob Expansion & Normalization
|
||
- **Python Runner Knobs (`lib_py/loop/runner.py`)**:
|
||
- Added `LoopRunner._int_knob(name, minimum)` (`#L350-L362`) leveraging `_positive_int`. Invalid values log a single warning to `sys.stderr` and fall back to defaults, preventing `ValueError` crashes.
|
||
- Knobs configured: `MAM_WAIT_GRACE_S` (min 0, default 300), `MAM_CHILD_WAIT_S` (min 0, default `job_budget + grace`), `MAM_JOB_TREE_MAX` (min 1, default 50).
|
||
- Documented `MAM_WAIT_GRACE_S` in `multi-agent-mux-loop/SKILL.md#L422`, `#L431`.
|
||
- **Bash Normalization & `lib_int.sh`**:
|
||
- Extracted integer normalization logic into dedicated `.agents/skills/lib_int.sh` (`#L1-L39`), sourced by `lib.sh`, `loop_lock.sh`, and `diff_collect.sh`. Added to `deploy/remove.sh#L517`.
|
||
- Normalized knobs:
|
||
- `MAM_READY_EMPTY_GIVEUP`: min 1, default 5 (`lib.sh#L2417`)
|
||
- `SKS_EMPTY_GIVEUP`: min 1, default 3 (`lib.sh#L2613`)
|
||
- `SKS_IDLE_RETRIES`: min 0, default 2 (`lib.sh#L2683`)
|
||
- `SKS_IDLE_RETRY_BACKOFF_S`: min 0, default 10 (`lib.sh#L2684`)
|
||
- `MAM_LOCK_STEAL_RETRIES`: min 0, default 3 (`loop_lock.sh#L465`)
|
||
- `MAM_DIFF_MAX_BYTES`: min 1, default 200000 (`diff_collect.sh#L448`)
|
||
- `MAM_DIFF_MAX_LINES`: min 1, default 4000 (`diff_collect.sh#L449`)
|
||
- `RECONCILE_POLL_INTERVAL`: min 1, default 15 (`reconcile.sh#L478`)
|
||
- Preserved fractional semantics: `SKS_QUIESCENT_INTERVAL` is intentionally kept as float (default 0.5s) and documented in `NOT_ARITHMETIC`.
|
||
- Test coverage: `tests/test_issue26_ints.py` updated with all new knobs, table minimum verifications, and standalone sourcing tests (`test_b5_scripts_sourced_without_lib_sh_read_their_knobs_as_decimal`).
|
||
|
||
### 2.5. F5: Test Hook Removal from Production Code
|
||
- **Implementation**:
|
||
- Removed `if os.environ.get("MAM_TEST_UNRELIABLE_AGENT") == args.agent: reliable = False` from `lib_py/stall_watch.py#L228`.
|
||
- In `tests/test_issue30_stall_watch.py#L880-L886`, updated `_watch` to inject an `_UNRELIABLE_MAIN` wrapper that sets `type(get_adapter(...)).agent_status_reliable = property(lambda self: False)` dynamically in the test process.
|
||
- Added `test_s13_the_unreliable_case_needs_no_switch_in_the_product` (`#L918-L922`) to ensure production code contains no `MAM_TEST_` switches.
|
||
|
||
### 2.6. F6: Documentation & Test Precision
|
||
- **Implementation**:
|
||
- `multi-agent-mux-delegate-job/SKILL.md#L407-L410`: Clarified that `<= 60 s` applies to herdr wait calls, and noted that active notification publishing can extend wrapper wait time until publication completes.
|
||
- `tests/test_issue30_stall_watch_stop.py#L936-L975`: Updated `test_a_wrapper_gone_during_the_wait_gets_no_notice_and_no_nudge` with an `in_wait` hook that flips wrapper liveness strictly during the second wait (`tick == 2`), avoiding heuristic call-count assumptions.
|
||
|
||
---
|
||
|
||
## 3. Verification Test Results
|
||
|
||
Tests executed in verification path `/var/folders/q_/68sxgw3j073dgjg8wl9s08vw0000gn/T/mam-review/loop-20261001T230324Z-b511bf/1` with isolated basetemp:
|
||
|
||
| Test Suite | Result | Duration |
|
||
| :--- | :--- | :--- |
|
||
| `tests/test_issue26_envfile.py` | **25 passed** | ~6.5s |
|
||
| `tests/test_version_consistency.py` | **15 passed** | ~1.0s |
|
||
| `tests/test_issue26_ints.py` | **93 passed** | 37.39s |
|
||
| `tests/test_issue30_stall_watch_stop.py` | **11 passed** | ~33s |
|
||
| `tests/test_tier1_unit.py` | **67 passed** | 60.45s |
|
||
| `tests/test_issue21_acceptance.py` | **10 passed** | 38.69s |
|
||
| `tests/test_issue30_stall_watch.py` | **48 passed, 1 skipped** | 263.62s |
|
||
| Git whitespace check (`git diff base...HEAD --check`) | **Clean (0 errors)** | - |
|
||
|
||
All tests passed with zero failures and zero residual processes.
|
||
|
||
---
|
||
|
||
## 4. Conclusion
|
||
|
||
All items F1 through F6 have been implemented accurately and tested thoroughly. No out-of-scope modifications were introduced.
|
||
|
||
[VERDICT: PASS]
|