GodopuandClaude Opus 5.5
56e6713292
The update skill joins the version matrix and the lockstep test's skill list. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01VCgu9Fa9iT7TbJUSzdq4Ff
116 lines
11 KiB
Markdown
116 lines
11 KiB
Markdown
# Code Review Report — PR #75 Follow-up Round (`1d63e88..ec7f5c4`, Issue #72)
|
||
|
||
- **Job ID**: `a967e100`
|
||
- **Reviewer**: `agy` (`herdr:reviewer-agy-01`)
|
||
- **Loop ID**: `loop-20261027T201245Z-63c71b` (Round 1)
|
||
- **Review Snapshot Path**: `/var/folders/q_/68sxgw3j073dgjg8wl9s08vw0000gn/T/mam-review/loop-20261027T201245Z-63c71b/1`
|
||
- **Branch**: `fix/issue-72`
|
||
- **Base Commit**: `1d63e882bc1c46e9af8484647d5091b296d8fa3f` (cumulative base: `0e1e2e87dea7fcaab69d6ac388121c35b02b7dab`)
|
||
- **Head Commit**: `ec7f5c4e57286fb3c4238b8220f39094f4640391`
|
||
- **Tree Hash**: `fbadb24de60ccdae684b23e2e0a41cdf23ab81eb`
|
||
|
||
---
|
||
|
||
## 1. 확인한 것 (Files & Line Ranges Inspected in Review Snapshot)
|
||
|
||
- **`1d63e88..ec7f5c4` 변경분 집중 검증**:
|
||
- `.agents/skills/lib.sh:2278-2317` (`_start_watchdog_legacy`):
|
||
- `MAM_DELEGATION_PY=0` 경로에서 `case $- in *m*) had_m=1 ;; esac`, `set -m`으로 `nohup bash "$monitor_script" --subscribe --idle-timeout 0 </dev/null >> "$log_file" 2>&1 &`를 실행한 뒤 `[ "$had_m" = 1 ] || set +m`으로 호출자의 job-control 옵션을 복원(`lib.sh:2303-2309`).
|
||
- macOS(`/bin/bash` 3.2, `setsid(1)` 없음)에서도 `set -m` 아래 백그라운드 작업이 자체 프로세스 그룹(`pgid == pid`)으로 기동되어 호출자의 프로세스 그룹에서 분리됨을 확인.
|
||
- 기존 동작(`_monitor_pids` 중복 방지, `MAM_NO_SUBSCRIBER_DAEMON=1` opt-out, `echo "$pid"` 보고, `</dev/null`, `>> "$log_file" 2>&1`)이 그대로 보존됨을 확인.
|
||
- `tests/test_issue72_update.py:184-195, 221-241, 697-726`:
|
||
- `_descendants(pid)`(`184-186`) 및 읽기 전용 `_processes_naming(path)`(`189-193`) 추가.
|
||
- `box` fixture teardown(`221-241`)에서 패턴 기반 `pkill -KILL -f <ws>`를 완전히 제거하고, 테스트가 직접 띄운 `b.helpers` 프로세스 그룹(`os.killpg`)과 `_exact_monitor_pids(ws / MONITOR_REL)` + 그 자식(`_descendants(pid)`)만 `SIGKILL`로 정리한 뒤 `_until(lambda: not _processes_naming(ws), timeout=10)`으로 잔존 프로세스가 없음을 검증.
|
||
- `test_every_restart_starts_a_real_monitor_in_both_delegation_modes`(`697-726`): 6개 조합(`mode` `0`/`1` × `installed`/`killed`/`rolled-back`) 모두에서 `assert all(os.getpgid(int(p)) != os.getpgrp() for p in box.monitor_pids())`로 모니터의 PGID가 호출자의 PGID와 다름을 검증.
|
||
- **누적 변경분(`0e1e2e8..ec7f5c4`) 정합성 확인**:
|
||
- `.agents/skills/lib.sh:24-35, 2238-2317, 3299-3379`
|
||
- `.agents/skills/lib_py/delegation/daemon.py:40-79`
|
||
- `.agents/skills/lib_py/pane_lock.py:1-130`
|
||
- `.agents/skills/lib_py/update.py:1-428`
|
||
- `.agents/skills/multi-agent-mux-update/SKILL.md:1-30`
|
||
- `.agents/skills/multi-agent-mux-update/scripts/{update.sh,install_phase.sh,post_update.sh}`
|
||
- `.agents/skills/multi-agent-mux-{create,resume,stop,loop,orc-onboard,delegate-job}` 및 `deploy/{install.sh,install_mam.sh,remove.sh}`
|
||
|
||
---
|
||
|
||
## 2. 결정 1–10, U1–U4, Attach 댓글 및 완료 조건 대조표
|
||
|
||
| 항목 | 요구사항 요약 | 구현 위치 | 테스트 커버리지 | 상태 |
|
||
|---|---|---|---|---|
|
||
| 결정 1 | 활성 세션(레지스트리 `running` + 모든 실행 중 herdr 서버의 워크스페이스 내 라이브 에이전트)·루프·열린 잡(`pending`/`dispatched`/`running`, 타 제어 평면은 본 레지스트리 대상 잡만) 있으면 거부 및 해제 명령 안내 | `lib_py/update.py:150-267`, `update.sh:124-162` | `tests/test_issue72_update.py` (`test_every_refusal_condition_blocks_and_leaves_the_workspace_untouched` 외 9개) | 충족 |
|
||
| 결정 2 | 모니터 데몬(`reconcile.sh --subscribe`) 자동 정지(`SIGTERM`→`SIGKILL`)·설치/타임아웃/롤백 후 호출자 프로세스 그룹에서 분리하여 재기동(`MAM_DELEGATION_PY=0/1` 모두 정확 매칭 및 별도 PGID); `resume` 시 모니터 보장 | `update.sh:100-116,165-212`, `post_update.sh:37-46`, `install_phase.sh:57-69`, `resume_session.sh:215-225,250,299`, `daemon.py:40-79`, `lib.sh:2238-2317` | `tests/test_issue72_update.py` (`test_every_restart_starts_a_real_monitor_in_both_delegation_modes` 외), `tests/test_issue72_resume_monitor.py` | 충족 |
|
||
| 결정 3 | 설치 실패 시 `.agents`, `.mam_deploy`, 루트 문서, `.venv`, 설치기 관리 `.mam` 항목, 레지스트리 전체 되돌리기 | `update.sh:240-262`, `install_phase.sh:27-44,104-115` | `tests/test_issue72_update.py` (`test_a_failing_install_rolls_back_the_entire_workspace_and_restarts_the_monitor` 외) | 충족 |
|
||
| 결정 4 | 업데이트 완료 후 새 코드로 레지스트리 변환(`lib_py.registry_format`), 레지스트리 없으면 생략, 실패 시 설치 유지 + 0 아닌 종료 | `post_update.sh:27-35` | `tests/test_issue72_update.py` (`test_a_conversion_failure_keeps_the_new_install_and_exits_nonzero` 외) | 충족 |
|
||
| 결정 5 | 2단계 분리: 워크스페이스 밖 임시 복사본(`install_phase.sh`)을 `exec`(같은 PID 유지)한 뒤 새 트리의 `post_update.sh`를 별도 프로세스로 실행 | `update.sh:214-215,264-267`, `install_phase.sh:118-129` | `tests/test_issue72_update.py` (`test_the_clean_update_runs_both_phases_converts_the_registry_and_restarts_the_monitor`) | 충족 |
|
||
| 결정 6 | `deploy/update.sh` 제거, `multi-agent-mux-update` 스킬로 일원화, `deploy/install.sh` 제자리 재실행 정리(없어진 파일 삭제·수정 파일 보존, `install_state` 제자리 갱신) | `deploy/install.sh:191-196,403-448,632-653`, `deploy/update.sh`(삭제) | `tests/test_issue72_installer.py`, `tests/test_deploy_layout.py`, `tests/test_issue8_update_state.py` | 충족 |
|
||
| 결정 7 | 호출 에이전트 자신(`(caller_server, HERDR_PANE_ID)`)과 그 `running` 행·실행 중인 잡 1개는 차단 대상에서 제외 | `lib_py/update.py:192-219,229-230,244-250`, `lib_py/pane_lock.py:25-39` | `tests/test_issue72_update.py` (`test_the_calling_agent_its_row_and_its_running_job_are_excluded` 외 3개) | 충족 |
|
||
| 결정 8 | 업데이트 입장 잠금(`.mam/update-admission.lock` 공유/배타 + `.mam/update-in-progress` 표식): `create`·`resume`·`run_loop`·`delegate submit`·`stop`·`orc-onboard` 차단, 자식 프로세스 fd 누수 방지 | `lib.sh:3312-3379`, `lib_py/pane_lock.py:42-115`, `lib_py/update.py:277-322` | `tests/test_issue72_admission.py` (21개 케이스) | 충족 |
|
||
| 결정 9 | `update.sh [-y|--yes]`: TTY면 프롬프트, 비-TTY에서 `-y` 없으면 거부(`exit 1`) | `update.sh:30-60` | `tests/test_issue72_update.py` (`test_confirmation_prompt_and_non_tty_refusal`) | 충족 |
|
||
| 결정 10 | 되돌리기 범위에서 작업 기록(`jobs`, `delegate_job_logs`, `state`, `worktrees`, `locks`) 보존 및 레지스트리 백업(`sqlite3.backup` + YAML/bak) 복원 | `update.sh:241-256`, `lib_py/update.py:342-366`, `install_phase.sh:37-44` | `tests/test_issue72_update.py` (`test_rollback_restores_every_item_the_installer_can_touch_and_leaves_work_records`) | 충족 |
|
||
| U1 | attach 안내 헬퍼는 추가하지 않고 기존 행의 `attach_command`(`ATTACH_CMD`)를 그대로 사용 | `create_session.sh:654`, `multi-agent-mux-delegate-job:680-687` | `tests/test_issue72_attach_hints.py` | 충족 |
|
||
| U2 | `.mam` 없는 트리에서 입장 잠금 헬퍼가 `.mam`이나 잠금 파일을 만들지 않고 통과 | `lib.sh:3315` | `tests/test_issue72_admission.py` (`test_a_tree_without_mam_stays_clean`) | 충족 |
|
||
| U3 | `MAM_UPDATE_MONITOR_STOP_S`를 `mam_int_knob`으로 읽고 `test_issue26_ints.py` 표에 등록 | `update.sh:189`, `tests/test_issue26_ints.py:67` | `tests/test_issue26_ints.py` | 충족 |
|
||
| U4 | 이전 업데이트가 남긴 `.mam/update-backup/` 상태별 처리(`backing-up`/`installed`/빈 상태 삭제, `installing`은 `--skip-registry`로 파일만 복원, 알 수 없는 상태 거부) | `update.sh:218-237`, `install_phase.sh:36` | `tests/test_issue72_update.py` (`test_a_leftover_backup_from_a_killed_update_is_handled_by_state`) | 충족 |
|
||
| 댓글 #926, #930 | `multi-agent-mux-delegate-job` 및 `create_session.sh`의 attach 안내가 `--session <server>`를 포함한 `attach_command`를 출력 | `create_session.sh:654`, `multi-agent-mux-delegate-job:680-687` | `tests/test_issue72_attach_hints.py` (8개 케이스) | 충족 |
|
||
| 첫 전환 (완료 조건 5) | 옛 `update.sh`가 있는 설치본에서 첫 전환 시 새 `install.sh`를 거쳐 `multi-agent-mux-update` 스킬과 레지스트리 변환까지 도달 | `deploy/install.sh:191-196,403-405`, `tests/test_issue72_update.py:909-966` | `tests/test_issue72_update.py` (`test_the_first_transition_from_the_old_updater_reaches_the_skill_and_conversion`) | 충족 |
|
||
|
||
빠진 완료 조건: **없음**.
|
||
|
||
---
|
||
|
||
## 3. 실행한 테스트와 결과 (macOS 실측)
|
||
|
||
검증 경로(`/var/folders/q_/68sxgw3j073dgjg8wl9s08vw0000gn/T/mam-review/loop-20261027T201245Z-63c71b/1`)에서 브리프의 격리 실행 명령으로 새·바뀐 테스트 파일과 watchdog 관련 테스트(`test_issue5_phase1_delegation.py`, `test_issue19_subscriber_daemon_lifecycle.py`)를 모두 실행했습니다.
|
||
|
||
### 명령 1 (`tests/test_issue72_update.py`, `tests/test_issue72_resume_monitor.py`, `tests/test_issue42_skill_blocks.py`)
|
||
```bash
|
||
BT="$(mktemp -d "${TMPDIR:-/tmp}/mam-rev-a967e100-1.XXXXXX")"
|
||
trap 'rm -rf "$BT"' EXIT
|
||
cd /var/folders/q_/68sxgw3j073dgjg8wl9s08vw0000gn/T/mam-review/loop-20261027T201245Z-63c71b/1 && \
|
||
env -u HERDR_SOCKET_PATH -u HERDR_PANE_ID -u HERDR_TAB_ID -u HERDR_WORKSPACE_ID -u HERDR_SESSION_NAME -u HERDR_SESSION -u WORKSPACE_ROOT -u MAM_ENV_FILE -u MAM_JOBS_DIR -u DELEGATE_JOB_LOGS_DIR \
|
||
PYTHONDONTWRITEBYTECODE=1 /Users/godopu16/PuKi/laa/canary_projects/multi-agent-mux/.venv/bin/python -m pytest -p no:cacheprovider --basetemp="$BT/pytest" \
|
||
tests/test_issue72_update.py \
|
||
tests/test_issue72_resume_monitor.py \
|
||
tests/test_issue42_skill_blocks.py
|
||
```
|
||
- **결과**: `95 passed, 1 skipped in 379.90s (0:06:19)` (exit code `0`)
|
||
|
||
### 명령 2 (watchdog 관련 스위트 2종 + 나머지 변경·신규 테스트 7종)
|
||
```bash
|
||
BT="$(mktemp -d "${TMPDIR:-/tmp}/mam-rev-a967e100-2.XXXXXX")"
|
||
trap 'rm -rf "$BT"' EXIT
|
||
cd /var/folders/q_/68sxgw3j073dgjg8wl9s08vw0000gn/T/mam-review/loop-20261027T201245Z-63c71b/1 && \
|
||
env -u HERDR_SOCKET_PATH -u HERDR_PANE_ID -u HERDR_TAB_ID -u HERDR_WORKSPACE_ID -u HERDR_SESSION_NAME -u HERDR_SESSION -u WORKSPACE_ROOT -u MAM_ENV_FILE -u MAM_JOBS_DIR -u DELEGATE_JOB_LOGS_DIR \
|
||
PYTHONDONTWRITEBYTECODE=1 /Users/godopu16/PuKi/laa/canary_projects/multi-agent-mux/.venv/bin/python -m pytest -p no:cacheprovider --basetemp="$BT/pytest" \
|
||
tests/test_issue5_phase1_delegation.py \
|
||
tests/test_issue19_subscriber_daemon_lifecycle.py \
|
||
tests/test_deploy_layout.py \
|
||
tests/test_issue26_ints.py \
|
||
tests/test_issue8_update_state.py \
|
||
tests/test_version_consistency.py \
|
||
tests/test_issue72_attach_hints.py \
|
||
tests/test_issue72_installer.py \
|
||
tests/test_issue72_admission.py
|
||
```
|
||
- **결과**: `219 passed in 509.96s (0:08:29)` (exit code `0`)
|
||
|
||
### 명령 3 (`/bin/bash -n` 구문 검사 및 프로세스·워크트리 무결성 확인)
|
||
- 변경된 모든 셸 스크립트(13개) `/bin/bash -n` 통과 (`exit 0`).
|
||
- `git status --short`: 출력 없음 (검증 스냅샷 깨끗함).
|
||
- `ps -ef`: 본 체크아웃의 기존 `reconcile.sh --subscribe` 데몬(PID `3714`)만 정상 유지 중이며, 테스트가 남긴 자식·더미 프로세스 없음.
|
||
|
||
---
|
||
|
||
## 4. 발견 사항 (차단 / 비차단)
|
||
|
||
- **차단 (Blocking)**: 없음.
|
||
- **비차단 (Non-Blocking)**: 없음.
|
||
|
||
---
|
||
|
||
## 5. 사용자 확인 필요
|
||
|
||
- 없음.
|
||
|
||
[VERDICT: PASS]
|