GodopuandClaude Opus 5.5
6fd417a7b7
Review loop (--review-only, --all-reviewer, focused on this Mac: macOS, /usr/bin/python3 3.9.6, bash 3.2): round 1 (@6f4f40a) agy and grok PASS, claude and codex NOT PASS (a non-uuid id `memory` purged the auto-memory dir, a linked dir below a root was followed, a NaN startedAt counted as dead), sent as a PR comment and fixed by the author in eleven commits; round 2 (@2a8d738) confirmed all three fixed but 4/4 NOT PASS on one test holder (`sh -c "sleep 120" <uuid>` loses the uuid on macOS /bin/sh), fixed in de6f3fc; round 3 (@de6f3fc) 4/4 PASS. Full suite on macOS / Python 3.9.6 @de6f3fc: 1934 passed, 56 skipped, 1 error (the known #55 false positive: agy conversations of another workspace). The real ~/.claude, /private/tmp/claude-<uid> and herdr panes unchanged. Also names lib_py.delegation.logroot in eager.CORE_MODULES (the v4.9.4 review's non-blocking note). Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01VCgu9Fa9iT7TbJUSzdq4Ff
82 lines
7.4 KiB
Markdown
82 lines
7.4 KiB
Markdown
# Pre-Merge Review Report (Round 2): PR #59 (`fix/issue-31`)
|
|
|
|
- **Job ID**: `b4aa3aff`
|
|
- **Reviewer**: `herdr:reviewer-agy-01` (`agy`)
|
|
- **Commit Range**: `26e2c43753169671b600d03274901dbe2039887` .. `2a8d73825b3832376aeaf96b09548359dc3cb887` (Round 2 delta: `6f4f40a4adb2115d5997dffd31fb6473f05cab68..2a8d73825b3832376aeaf96b09548359dc3cb887`, 11 commits)
|
|
- **Verification Worktree**: `/var/folders/q_/68sxgw3j073dgjg8wl9s08vw0000gn/T/mam-review/loop-20261017T135731Z-e93f0a/1`
|
|
- **Host / Runtime**: macOS `arm64-apple-darwin25` (Darwin 25.3.0), `/usr/bin/python3` 3.9.6 & `.venv/bin/python` 3.9.6, `/bin/bash` 3.2.57
|
|
|
|
---
|
|
|
|
## 1. Round 1 Findings Verification
|
|
|
|
All three required production fixes and all four non-blocking improvements from Round 1 are cleanly resolved in `6f4f40a..2a8d738`:
|
|
|
|
1. **[Blocker — Resolved] Canonical UUID enforcement across Claude adapter (`bd09c6a`, `c0248b8`, `81b6c2e`, `2a8d738`)**:
|
|
- `_canonical(uuid)` in `.agents/skills/lib_py/agents/adapters/claude.py:10-18` validates `isinstance(uuid, str) and _UUID.fullmatch(uuid)` (`[0-9a-f]{8}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{12}`, case-insensitive) and normalizes to lowercase.
|
|
- `_candidates`, `has_artifacts`, `conversation_in_use`, `purge_artifacts`, and `residual_artifacts` all gate on `_canonical(uuid)`, ensuring non-UUID names (such as `memory`, `notes`, or `uuid-c`) never touch `projects/<key>/memory/` or any other directory/file.
|
|
- `conversation_in_use` checks `any(conversation_in_use(u, [self.artifact_path(canon, ctx)]) for u in {canon, uuid})` plus `self._session_files(canon, ctx)`, properly handling uppercase/mixed-case UUIDs on case-insensitive APFS.
|
|
- `tests/test_a4_adapter_contract.py:225-232` was updated with user approval to use a canonical UUID (`0c1a5f3e-6b2d-4c8e-9a7f-1d2e3f4a5b6c`).
|
|
|
|
2. **[P1 — Resolved] Symlink boundary check below Claude roots (`6e0361c`, `dd292c1`)**:
|
|
- `_inside(root, path)` in `.agents/skills/lib_py/agents/adapters/claude.py:108-122` walks every path component of `os.path.relpath(os.path.dirname(path), root)` and verifies `stat.S_ISDIR(os.lstat(cur).st_mode)` (rejecting any intermediate symlink) and `os.path.realpath(os.path.dirname(path)).startswith(real_root.rstrip(os.sep) + os.sep)`.
|
|
- Roots that are themselves symlinks (such as `/tmp -> /private/tmp` on macOS or a symlinked `~/.claude`) and leaf symlinks (`_remove` unlinks the leaf symlink without traversing) work as intended.
|
|
- `has_artifacts` checks `bool(self._candidates(uuid, ctx))` so a candidate behind a symlink still selects `claude` in `purge auto` / `purge claude` and is reported in `residual_artifacts` without being deleted.
|
|
|
|
3. **[P2 — Resolved] Fail-safe `startedAt` validation in `_session_file_live` (`f943b0d`, `60779ba`)**:
|
|
- `.agents/skills/lib_py/agents/adapters/claude.py:87-95` rejects `bool`, non-`(int, float)`, `OverflowError` on `float(started) / 1000.0` (e.g. `10**400`), `NaN`, `±inf`, and `<= 0`, returning `True` (fail-safe live) whenever the PID is alive.
|
|
|
|
4. **[Non-Blocking Notes — Resolved]**:
|
|
- **UTC `lstart` parsing (`af8ed39`)**: `ps -p <pid> -o lstart=` runs with `LC_ALL="C", TZ="UTC0"` and parses via `calendar.timegm`. Verified directly on macOS Darwin with both `.venv/bin/python` and `/usr/bin/python3` (`abs(time.time() - lstart) < 0.7s`).
|
|
- **Actionable `in_use_hints` & exit-4 wording (`e156179`)**: `BaseAgentAdapter.in_use_hints` and `ClaudeAgentAdapter.in_use_hints` print the active `<cfg>/sessions/` path and `ps -axww | grep <uuid>` hint on stderr when `in-use` or `stop_session.sh --purge-conversation` exits 4; `SKILL.md` and `stop_session.sh` accurately state that registry records and conversation files are preserved even though the herdr session itself may already be down.
|
|
- **Host `/tmp/claude-<uid>` test isolation (`26fb1c4`)**: `_SYSTEM_TMP = "/tmp"` in `claude.py` is redirected by the `own_tmp_roots` autouse fixture (and `sitecustomize.py` for child Python processes) in `tests/test_issue31_purge_residual.py`.
|
|
|
|
5. **Pre-fix failure verification**:
|
|
- Running the new Round 2 regression tests from `tests/test_issue31_purge_residual.py` against pre-fix code (`26fb1c4`, before `bd09c6a..2a8d738`) in an isolated `/tmp` copy produced **25 test failures** across all targeted bug classes (`test_claude_rejects_malformed_uuid`, `test_any_spelling_of_the_uuid_is_the_lowercase_session`, `test_claude_purge_never_follows_a_linked_dir_below_a_root`, `test_purge_cli_reports_a_linked_orphan_it_will_not_remove`, `test_a_non_uuid_id_reaches_nothing`, `test_liveness_reads_lstart_as_utc`, `test_an_unusable_started_at_means_live`, `test_a_live_holder_with_an_unusable_started_at_blocks_the_purge`).
|
|
|
|
---
|
|
|
|
## 2. Remaining Issue Blocking Merge
|
|
|
|
### [P1] `test_any_spelling_of_the_uuid_is_the_lowercase_session` fails on macOS due to `/bin/sh -c` single-command `exec` optimization
|
|
|
|
- **Location**: `tests/test_issue31_purge_residual.py:315` (introduced in commit `c0248b8`)
|
|
- **Symptom**:
|
|
Running pytest on macOS fails both parametrizations (`[upper]` and `[mixed]`):
|
|
```text
|
|
FAILED tests/test_issue31_purge_residual.py::test_any_spelling_of_the_uuid_is_the_lowercase_session[upper]
|
|
FAILED tests/test_issue31_purge_residual.py::test_any_spelling_of_the_uuid_is_the_lowercase_session[mixed]
|
|
AssertionError: a command line naming the lowercase id
|
|
assert False is True
|
|
```
|
|
- **Root Cause**:
|
|
Line 315 spawns the dummy holder process as:
|
|
```python
|
|
named = subprocess.Popen(["sh", "-c", "sleep 120", conv])
|
|
```
|
|
On macOS (`/bin/sh` is GNU bash 3.2.57 in POSIX mode), `sh -c "sleep 120" <conv>` sees a single simple command with no subsequent commands or traps and optimizes by calling `execve("/bin/sleep", ["sleep", "120"])` directly in-place (`CMD_NO_FORK`). Within ~10ms (before `time.sleep(0.05)` finishes), the process image and argv in `ps -axo pid=,command=` become `sleep 120`, and `$0` (`conv`) vanishes from the process list.
|
|
Note that `tests/test_issue10_13_purge.py:111-118` (`_holder`) already documents this exact pitfall:
|
|
> `(Not sh -c 'sleep …': a shell execs its last simple command and the uuid would vanish from the process list.)`
|
|
- **Required Fix**:
|
|
In `tests/test_issue31_purge_residual.py:315`, use `sys.executable` instead of `sh -c`:
|
|
```python
|
|
named = subprocess.Popen([sys.executable, "-c", "import time; time.sleep(120)", conv])
|
|
```
|
|
*(Verified in an isolated `/tmp` copy on macOS: replacing line 315 with `sys.executable` makes both `[upper]` and `[mixed]` pass in 2.24s, with no orphaned child `sleep` process after `named.kill()`).*
|
|
|
|
---
|
|
|
|
## 3. Verification Summary
|
|
|
|
- Targeted pytest suite run against `/var/folders/q_/68sxgw3j073dgjg8wl9s08vw0000gn/T/mam-review/loop-20261017T135731Z-e93f0a/1` with scrubbed herdr/MAM/Claude environment variables:
|
|
- `tests/test_issue55_agent_homes.py`: **PASSED**
|
|
- `tests/test_issue42_skill_blocks.py`: **PASSED**
|
|
- `tests/test_a4_adapter_contract.py`: **PASSED**
|
|
- `tests/test_orc_onboard.py`: **PASSED**
|
|
- `tests/test_tier1_unit.py`: **PASSED**
|
|
- `tests/test_issue31_purge_residual.py`: **65 passed, 2 failed** (`test_any_spelling_of_the_uuid_is_the_lowercase_session[upper]` and `[mixed]`)
|
|
- Total: **250 passed, 2 failed, 1 skipped** (all 252 pass once `tests/test_issue31_purge_residual.py:315` uses `sys.executable`).
|
|
- Verification worktree `/var/folders/q_/68sxgw3j073dgjg8wl9s08vw0000gn/T/mam-review/loop-20261017T135731Z-e93f0a/1` verified clean (`git status -s` empty, no `.mam` directory created).
|
|
|
|
[VERDICT: NOT PASS]
|