GodopuandClaude Opus 5.5
7cf728e9f8
Review loop (--review-only, --all-reviewer, focused on this Mac: macOS, /usr/bin/python3 3.9.6, bash 3.2): round 1 (@63d8164) agy and grok PASS, claude and codex NOT PASS (F1 paho via the /usr/bin/python3 fallback, F2 logs default path, R1 disk-fallback redispatch interleaving, R2 symlinked registry), sent as a PR comment and fixed by the author in four commits; round 2 (@a7cb90e) 4/4 PASS. Full suite on macOS / Python 3.9.6 @a7cb90e: 1867 passed, 56 skipped, 1 error (the known #55 false positive: WAL/SHM of an agy conversation running in another workspace). Real herdr panes and ~/.claude/projects unchanged. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01VCgu9Fa9iT7TbJUSzdq4Ff
69 lines
7.6 KiB
Markdown
69 lines
7.6 KiB
Markdown
# Review Report — PR #58 Round 2 (Issue #57: Audit logs follow a moved registry)
|
|
|
|
- **Reviewer**: `agy` (`herdr:reviewer-agy-01`)
|
|
- **Job ID**: `e6f0a5ad`
|
|
- **Branch / Range**: `fix/issue-57` (`e32c9574644a9185813a4dc881812694d62b6204..a7cb90e63abf454ccf22f6ff887cc25393755402`, 15 commits, 23 files)
|
|
- **Round 2 Delta**: `63d816468e6d48d01bd96f31d8a25eeadfbaab60..a7cb90e63abf454ccf22f6ff887cc25393755402` (4 commits: `75c0042`, `151af99`, `1c598bb`, `a7cb90e`)
|
|
- **Verification Worktree**: `/var/folders/q_/68sxgw3j073dgjg8wl9s08vw0000gn/T/mam-review/loop-20261015T123502Z-1b9df6/1`
|
|
- **Platform**: macOS Darwin (`arm64-apple-darwin25`, `/usr/bin/python3` 3.9.6 without `paho-mqtt`, `.venv/bin/python` 3.9.6, `/bin/bash` 3.2.57)
|
|
|
|
---
|
|
|
|
## 1. Executive Summary
|
|
|
|
All 4 findings raised in Round 1 (`F1`, `F2`, `R1`, `R2`) are cleanly and completely resolved in `63d8164..a7cb90e`:
|
|
|
|
1. **F1 (`75c0042`) — Stdlib-only path resolution & `paho`-free package import**:
|
|
- Path resolution (`DEFAULT_REGISTRY_DIR`, `_workspace_mam_dir`, `default_registry_dir`, `sibling_logs_dir`, `get_logs_dir`) moved to stdlib-only `.agents/skills/lib_py/delegation/logroot.py`, with `mqtt_common.py` re-exporting all 5 symbols (`getattr(mqtt_common, k) is getattr(logroot, k)`).
|
|
- Both `mqtt_common.py` and `publisher.py` guard `import paho.mqtt.client as mqtt` with `try ... except ImportError: mqtt = None` (raising a descriptive `ImportError` inside `make_client()` only when a broker connection is actually requested). This allows `lib_py.delegation.__init__` (`DelegationService` -> `mqtt_common`, `publisher`) as well as `lib_py.control_plane`, `lib.sh`, and `status.sh` to import and execute cleanly under `/usr/bin/python3` when `paho-mqtt` is absent.
|
|
2. **F2 (`151af99`) — Wrapper `logs` forwards `--registry-dir` only when explicitly passed**:
|
|
- `cmd_logs` in `multi-agent-mux-delegate-job` uses `local -a reg=()` and expands `${reg[@]+"${reg[@]}"}` (Bash 3.2 + `set -u` safe idiom) so `registry.py` receives `--registry-dir` only when the caller passed it. Unadorned `logs --list` and `logs <jid>` from an arbitrary `cwd` continue to honor `MAM_JOBS_DIR` and `WORKSPACE_ROOT`.
|
|
3. **R1 (`1c598bb`) — Re-reading the registry record before adopting an audit-only terminal status**:
|
|
- In `job_subscriber._check_disk_fallback`, after reading `logged = mqtt_common.read_logged_status(...)` and verifying `not stale`, the subscriber re-reads `now = load_job(jid, registry_dir)` and verifies `(now.get("dispatch_token"), now.get("attempt_id", now.get("iteration")), now.get("transition_seq")) == (rec_token, rec_attempt, rec_seq)`.
|
|
- If an `A error -> redispatch (attempt 2) -> B completed` sequence interleaved between the initial `load_job` and `read_logged_status`, `same` is `False` and the subscriber defers to the next poll tick instead of attributing attempt B's completion to caller A's token/attempt.
|
|
4. **R2 (`a7cb90e`) — `sibling_logs_dir` resolves `os.path.realpath(registry_dir)`**:
|
|
- `logroot.sibling_logs_dir(registry_dir)` now computes `os.path.join(os.path.dirname(os.path.realpath(registry_dir)), "delegate_job_logs")`, matching the wrapper's `cd -P ... && pwd -P` and `control_plane._abs()`. When `--registry-dir` points to a symlinked `jobs` directory (`<tmp>/alias/jobs -> <tmp>/actual/jobs`), both `submit` and `logs` resolve to `<tmp>/actual/delegate_job_logs`.
|
|
|
|
---
|
|
|
|
## 2. Verification of Perspective (1): Manual Reproductions & Pre-Fix Failure Check
|
|
|
|
- **Manual Reproductions on macOS (`/usr/bin/python3` 3.9.6 without `paho`, `/bin/bash` 3.2.57)**:
|
|
- **F1**: Confirmed `/usr/bin/python3 -c "import paho"` fails (`ModuleNotFoundError`). With `AGENT_PYTHON_BIN=/usr/bin/python3` and `MAM_JOBS_DIR=$TMP/moved/jobs`, `mam_control_plane_json`, `python3 -m lib_py.control_plane prefix`, `delegate_publish_cmd`, `delegate_logs_dir`, and `status.sh`'s `sibling_logs_dir` candidate resolution all succeeded with zero errors.
|
|
- **F2**: With `WORKSPACE_ROOT=$TMP/ws` and `cwd=$TMP/elsewhere`, registered a job into `$TMP/ws/.mam/jobs` and ran `multi-agent-mux-delegate-job logs --list` and `logs <jid>` without `--registry-dir`; both found the job in `$TMP/ws/.mam/delegate_job_logs` and left no `.mam` in `$TMP/elsewhere`.
|
|
- **R1 & R2**: Verified `<tmp>/alias/jobs -> <tmp>/actual/jobs` symlink agreement between `submit --dry-run` and `logs`, and verified the `A error -> redispatch -> B completed` interleaving in `_check_disk_fallback`.
|
|
- **Pre-Fix Regression Failure Check (`63d8164`)**:
|
|
- Ran the 6 new regression test cases from `tests/test_issue57_logs_follow_registry.py` against an exported copy of `63d8164` in `/tmp`: all 6 failed (`test_control_plane_binding_works_without_paho`, `test_status_works_without_paho[json]`, `test_status_works_without_paho[text]`, `test_logs_without_registry_dir_follows_the_workspace_from_any_cwd`, `test_disk_fallback_rejects_a_terminal_that_belongs_to_a_later_dispatch`, `test_a_symlinked_registry_dir_resolves_to_one_audit_log`), and all 6 pass on `a7cb90e`.
|
|
|
|
---
|
|
|
|
## 3. Verification of Perspective (2) & (3): Side-Effect Analysis & macOS Consistency
|
|
|
|
- **R1 Re-Read Concurrency & Performance**:
|
|
- Every engine state transition increments `transition_seq` monotonically (and redispatch also rotates `dispatch_token` and increments `attempt_id`), so the tuple `(dispatch_token, attempt_id, transition_seq)` is strictly ABA-safe.
|
|
- When no registry record exists on disk (audit-only fallback after registry cleanup), both reads catch `Exception` and produce `(None, None, None) == (None, None, None)` (`same is True`), preserving audit-only terminal resolution.
|
|
- If a normal single-attempt job transitions from `running` to `completed` during the microsecond window between the first `load_job` and `read_logged_status`, `same` is `False` for that tick and the next poll tick reads `status == "completed"` directly from the registry record on the first `load_job`.
|
|
- **R2 `realpath` Impact on Default Paths & `control_plane.forwarded_keys`**:
|
|
- On macOS where `/var` -> `/private/var` and `/tmp` -> `/private/tmp`, `control_plane._abs()` already used `os.path.realpath`. Updating `sibling_logs_dir()` to use `os.path.realpath` makes `get_logs_dir()`, `sibling_logs_dir()`, `control_plane.from_env()`, and `control_plane.forwarded_keys()` (which now checks `if logs and _abs(logs) == _abs(sibling_logs_dir(desc["mam_jobs_dir"])):`) completely consistent.
|
|
- **F1 Module Move, Re-Exports & Eager Import (`lib_py/loop/eager.py`)**:
|
|
- `mqtt_common` re-exports `DEFAULT_REGISTRY_DIR`, `_workspace_mam_dir`, `default_registry_dir`, `get_logs_dir`, and `sibling_logs_dir` from `lib_py.delegation.logroot` at module top-level.
|
|
- Because `lib_py.delegation.mqtt_common` is in `eager.CORE_MODULES`, `eager.eager_load()` imports `lib_py.delegation.logroot` into `sys.modules` at loop startup (`test_issue5_phase4_runtime.py` passes).
|
|
|
|
---
|
|
|
|
## 4. Test Suite Execution Summary
|
|
|
|
Executed in `/var/folders/q_/68sxgw3j073dgjg8wl9s08vw0000gn/T/mam-review/loop-20261015T123502Z-1b9df6/1` with `HERDR_*`, `WORKSPACE_ROOT`, `MAM_ENV_FILE`, `MAM_JOBS_DIR`, and `DELEGATE_JOB_LOGS_DIR` scrubbed, `PYTHONDONTWRITEBYTECODE=1`, `-p no:cacheprovider`, and `--basetemp=/tmp/mam_verify_pr58_r2_bt`:
|
|
|
|
- `tests/test_issue57_logs_follow_registry.py`: **18 passed**
|
|
- `tests/test_issue15_registry_isolation.py`: **20 passed**
|
|
- `tests/test_tier1_unit.py`: **58 passed**
|
|
- `tests/test_version_consistency.py`: **3 passed**
|
|
- `tests/test_issue5_phase4_runtime.py`: **5 passed**
|
|
- `tests/test_issue21_worktree_prereqs.py`: **36 passed**
|
|
- `tests/test_issue21_acceptance.py`: **9 passed**
|
|
|
|
Total: **149 passed, 0 failed**. Verification worktree `git status -s` is empty and no `.mam` directory was created.
|
|
|
|
[VERDICT: PASS]
|