docs(reports): archive v4.4.5 unanimous PASS review reports (Issue #19)
Round 3 at 134fe97: claude, agy, grok and codex all PASS.
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01VCgu9Fa9iT7TbJUSzdq4Ff
This commit is contained in:
1 parent
134fe9704c
commit
1bd643e979
4 files changed
+383
No files matched your search
@@ -0,0 +1,184 @@
|
||||
# 🔍 Review Report — Job ff5f9925 (branch `fix/issue-19`, reviewed HEAD `134fe97`)
|
||||
|
||||
- **Reviewer**: `planner-reviewer-claude-01` (role: planner,reviewer) · 2026-09-13
|
||||
- **대상**: `git log 8e7282c..HEAD` 6커밋, `git diff 8e7282c...HEAD` 22파일 +791/−50.
|
||||
- 커밋: `ce315d8`(fix #19), `38d8ccb`(release v4.4.5), `4da4a50`, `c096cb4`, `8f9745b`, **`134fe97`**.
|
||||
- 이번 라운드에 새로 들어온 것은 `134fe97` 하나다. grok 의 N1·B1 을 반영했고, 4파일 +97/−27 이다.
|
||||
- 브리프에는 HEAD 가 `38d8ccb` 로 적혀 있다. 실제로 검토한 HEAD 는 **`134fe97`** 이며, `completed` 발행 명령에서 HEAD 를 다시 확인한다.
|
||||
- **방법**:
|
||||
- 브랜치 전체 diff 와 `134fe97` 을 정독했다.
|
||||
- macOS `/bin/bash` 3.2.57(PATH 의 bash 와 같다)에서 B1 을 재현하고 수정을 확인했다.
|
||||
- paho 2.1.0 의 연결 중 시그널 지연을 데몬 없이 python 단독으로 실측했다.
|
||||
- 선택 테스트 5파일을 실행하고, 신규 시그널 테스트를 3회 반복했다. 정적 검사도 했다.
|
||||
- 리뷰 중 45초 간격으로 `progress` 를 발행했다. 코드는 수정하지 않았다.
|
||||
- **안전 규칙 준수**:
|
||||
- 실제 monitor 데몬 35505(이 저장소)와 8152(다른 프로젝트), 그리고 `.mam/monitor.lock` 은 읽기 전용 `ps` 로만 확인했다.
|
||||
- `scrub-secrets`·purge 는 실행하지 않았다.
|
||||
- 데몬·트랩 실험은 pytest 샌드박스, `bash -c` 단독 스크립트, python 단독 프로세스에서만 했다.
|
||||
|
||||
## 0. 이전 리뷰와 이번 커밋
|
||||
|
||||
| 리뷰 | 대상 HEAD | 판정 | 요지 |
|
||||
|---|---|---|---|
|
||||
| `planner-reviewer-claude-01`(`9114ac86`) | `8f9745b` | PASS | **B1 을 놓쳤다**(§0-1) |
|
||||
| `reviewer-agy-01`(`0f0327f6`) | `8f9745b` | PASS | — |
|
||||
| `reviewer-grok-01`(`0dc6400b`) | `8f9745b` | **NOT PASS** | **N1(차단)**: 구독 직후의 SIGTERM 이 정리 경로를 타지 않아 `test_mqtt_loop_sigterm_exits_cleanly` 가 실패. **B1(비차단)**: 첫 `&` 전의 TERM 에서 트랩이 rc 1 로 끝남 |
|
||||
| `planner-reviewer-codex-01`(`236cc993`) | `8f9745b` | PASS | — |
|
||||
|
||||
creator(`fdb85679`)는 반론 없이 N1·B1 을 `134fe97` 에 반영했다. N1 은 grok 의 대안 1(핸들러를 연결 전에 설치), B1 은 `set +u` 로 읽는 방식이다.
|
||||
|
||||
### 0-1. 지난 리뷰의 누락 인정
|
||||
|
||||
- 나는 `9114ac86` §1-1 에서 `${_sub_pid:-${!:-}}` 를 승인했다. 근거는 "`$!` 는 비어 있거나 구독자 자신" 이라는 추론뿐이었고, 대상 셸인 bash 3.2 에서 실제로 실행해 보지 않았다.
|
||||
- bash 3.2 는 `set -u` 아래에서 첫 `&` 전에 `${!:-}` 를 `$!: unbound variable` 로 처리해 셸을 죽인다. 이것을 grok 이 찾았다(B1).
|
||||
- 이번에는 트랩과 `set -u` 경로를 `/bin/bash` 3.2.57 에서 직접 실행해 확인했다(§1-1).
|
||||
|
||||
## 1. `134fe97` 검증
|
||||
|
||||
### 1-1. B1: `_sub_stop` 의 `$!` 읽기 (`reconcile.sh:80-98`)
|
||||
|
||||
수정 내용(`:86-91`): 먼저 `local child="${_sub_pid:-}"` 로 읽고, 비어 있을 때만 `set +u; child="$!"; set -u` 로 `$!` 를 읽는다.
|
||||
|
||||
| 실측 (`/bin/bash` 3.2.57, `set -euo pipefail`) | 결과 |
|
||||
|---|---|
|
||||
| `set -u; x="${!:-}"` (수정 전 방식), 첫 `&` 전 | `$!: unbound variable`. B1 재현 |
|
||||
| 수정 전 트랩 본문 + `kill -TERM $$`, 자식 없음 | 같은 오류로 rc 1 |
|
||||
| **수정 후 `_sub_stop` 원문**, 자식 없음 | **rc 143** |
|
||||
| **수정 후 `_sub_stop` 원문**, `sleep 30 &` 자식 있음 | **rc 143**. `sleep` 도 종료됨 |
|
||||
|
||||
- `set +u` 가 풀려 있는 구간은 대입 한 줄뿐이다. 트랩은 곧바로 `exit 143` 으로 끝나므로 nounset 해제가 다른 코드로 새지 않는다.
|
||||
- 테스트 `test_sub_stop_trap_exits_143_before_the_first_background_child`(`tests/test_issue19_subscriber_daemon_lifecycle.py:395-411`):
|
||||
- `_sub_stop` 을 정규식으로 원문 그대로 추출한다. 정규식은 두 칸 들여쓴 첫 `}` 까지 잡는데, 함수 본문의 `fi` 는 네 칸 들여쓰기라 경계가 정확하다.
|
||||
- `set -euo pipefail` 인 `bash -c` 에서 자식 없이 한 번, 자식이 있는 상태로 한 번 실행한다.
|
||||
- 자식이 있는 경우는 0.5 s 기다린 뒤 TERM 을 보낸다. §1-3 의 fork-exec 창과 섞이지 않게 분리한 것이다.
|
||||
- 이 테스트는 PATH 의 `bash` 로 실행된다. 따라서 B1 회귀는 bash 3.2 환경(macOS)에서 잡힌다. 이 머신에는 bash 4 이상이 없어 다른 버전의 동작은 확인하지 못했다.
|
||||
|
||||
### 1-2. N1: python 핸들러를 연결 전에 설치 (`reconcile.sh:300-377`)
|
||||
|
||||
**순서**:
|
||||
1. `stop_event`, `_on_stop_signal`, `signal.signal` ×2 (`:300-317`)
|
||||
2. 클라이언트 생성 (`:319-323`)
|
||||
3. `connect` (`:326-331`)
|
||||
4. `loop_start` + CONNACK 대기 (`:332-341`)
|
||||
5. 메인 루프 (`:343-370`)
|
||||
6. `finally` 에서 `loop_stop`/`disconnect` (`:371-376`)
|
||||
7. `sys.exit(0)` (`:377`)
|
||||
|
||||
| 시그널이 도착한 시점 | 동작 | 결과 |
|
||||
|---|---|---|
|
||||
| 인터프리터 기동·import 중 (핸들러 설치 전) | python 기본 동작으로 즉시 종료 | 연결·구독 전이라 정리할 것이 없다. lock 은 프로세스 종료로 풀리고 bash 는 143 |
|
||||
| `connect` 중 (TCP 연결 대기) | 핸들러는 플래그만 세우고, `connect` 는 PEP 475 재시도로 시간 초과까지 계속된다. 예외가 나면 `stop_signal` 이 있으므로 폴백하지 않는다(`:329`). 메인 루프 첫 검사(`:346`)에서 `signal 15 received` 를 찍고 `finally` 로 간다 | rc 0, 폴백 없음. **최대 약 5 s 지연**(§3-1) |
|
||||
| `connect` 성공 뒤 CONNACK 대기 중 | `stop_event.wait(0.1)` 이 곧바로 깨어나 대기 루프를 빠져나온다(`:335-337`). `stop_signal` 이 있으므로 폴백하지 않는다(`:338`) | 즉시, rc 0 |
|
||||
| 구독 직후 (`subscribed to` 로그 뒤) | 메인 루프의 `stop_event.wait` 이 깨어난다 | 즉시, rc 0. grok N1 의 사례 |
|
||||
| `loop_start` 전 (`connect` 예외 경로) | `finally` 의 `loop_stop()` 은 스레드가 없어 `MQTT_ERR_INVAL` 을 반환할 뿐이다(paho 2.1.0). `disconnect` 는 `try` 안에 있다 | 예외 없음 |
|
||||
|
||||
- **rc 계약**:
|
||||
- 시그널을 받은 구독자는 rc 3 을 내지 않는다(`:329`, `:338`). 그래서 bash 가 폴링 폴백으로 들어가지 않는다.
|
||||
- rc 0(정상), 3(폴백), 1(lock 실패)의 의미는 그대로다.
|
||||
- python PID 에만 직접 TERM 을 보낸 경우 python 은 0, bash 는 `exit 0` 으로 끝난다. 수정 전(python 이 시그널로 죽고 bash 는 0)과 외부 결과가 같다.
|
||||
- **paho 스레드와의 충돌 없음**:
|
||||
- python 시그널 핸들러는 항상 메인 스레드에서 실행되고, 플래그와 `threading.Event` 만 건드린다.
|
||||
- paho 스레드는 시그널을 처리하지 않는다. `loop_stop()` 은 메인 스레드에서 스레드를 join 한다.
|
||||
- **테스트 `test_sigterm_during_broker_handshake_takes_the_clean_path`** (`:361-392`):
|
||||
- 브로커 없이, 임의 포트에서 응답하지 않는 로컬 리스너를 쓴다.
|
||||
- `connecting to` 로그 1 s 뒤에 TERM 을 보내고, rc 143, `signal 15 received` 있음, 폴백 없음을 확인한다.
|
||||
- CONNACK 대기 5 s 중 1 s 시점이므로 약 4 s 여유가 있다. `connect` 전에 TERM 이 닿아도 로컬 연결은 곧바로 끝나 같은 결과가 된다.
|
||||
- 부하 때문에 로그 확인에서 TERM 까지 5 s 를 넘길 때만 실패할 수 있어, 결정적이라고 판단한다.
|
||||
- grok 이 실패를 본 `test_mqtt_loop_sigterm_exits_cleanly` 는 이번 실행에서 모두 통과했다. 이제 `subscribed to` 로그는 "핸들러 설치 이후" 를 뜻하므로, 테스트의 트리거 조건을 바꾸지 않은 판단이 옳다.
|
||||
|
||||
### 1-3. creator 가 새로 보고한 fork-exec 창 (고치지 않음. 후속 과제에 동의)
|
||||
|
||||
- **현상**: TERM 이 `&` 의 fork 와 exec 사이 수 ms 안에 오는 경우다. 자식이 물려받은 트랩이 전달된 TERM 을 삼키고, bash 는 트랩의 `wait` 에 묶인다.
|
||||
- **비차단으로 보는 이유**:
|
||||
- 기동 직후의 매우 좁은 창이다.
|
||||
- 그렇게 살아남은 python 도 #19 의 자기 종료가 그대로 적용된다. 워크스페이스가 삭제되거나 lock 이 교체되면 5 s 안에 종료하므로, 샌드박스에서 무한 누수가 되지는 않는다.
|
||||
- **후속 수정 때 함께 볼 점**:
|
||||
- 부모 bash 를 잃은 python 구독자(argv 는 `python -`)는 conftest 선정자(`bash|sh <root>/…/reconcile.sh --subscribe`)에도 세션 가드에도 보이지 않는다.
|
||||
- creator 가 제안한 방식(자식 생존 확인 → 제한 횟수 TERM 재전송 → 마지막에 KILL)에 동의한다.
|
||||
|
||||
## 2. 브랜치 전체 검증 항목 (1)–(7)
|
||||
|
||||
| # | 항목 | 결과 |
|
||||
|---|---|---|
|
||||
| (1) | `reconcile.sh` | ✅ 아래 1번 목록 |
|
||||
| (2) | `daemon.py`/`lib.sh`/`cli.py` | ✅ `134fe97` 에서 바뀌지 않았다. 아래 2번 목록 |
|
||||
| (3) | `tests/conftest.py` | ✅ `134fe97` 에서 바뀌지 않았다. 아래 3번 목록 |
|
||||
| (4) | 신규 테스트 결정성 | ✅ 아래 4번 목록 |
|
||||
| (5) | 문서 | ✅ 아래 5번 목록 |
|
||||
| (6) | 버전 락스텝 · 릴리스 커밋 | ✅ 아래 6번 목록 |
|
||||
| (7) | 테스트 · 정적 검사 | ✅ 아래 7번 목록 |
|
||||
|
||||
1. **`reconcile.sh`**:
|
||||
- **rc 계약**: `set +e`(`:101`) → `&`(`:105`) → `_sub_pid=$!`/`wait`/`sub_rc`(`:379-381`) → `set -e`(`:383`) 순서다. rc 가 `3` 일 때만 폴백하고(`:385`), 그 밖에는 `exit 0`(`:408`)으로 끝나 종전 계약과 같다.
|
||||
- **트랩**: 첫 자식 전(B1 수정), `&` 직후(`$!`, R-1), 자식이 이미 끝난 뒤(kill/wait 가 아무 일도 하지 않음) 모두 rc 143 이다.
|
||||
- **`_workspace_gone`**(`:132-150`):
|
||||
- `os.getcwd()` 예외를 처리한다(`:138-141`).
|
||||
- `(st_dev, st_ino)` 로 비교한다(`:128-129`, `:145-147`). 원래 lock 을 열어 둔 채이므로 그 inode 는 재사용될 수 없고, 교체 판정이 안전하다.
|
||||
- **paho 스레드와 충돌 없음**: §1-2.
|
||||
- **폴백**: `trap … exit 143`(`:394`) 과 `sleep & wait $! || true`(`:404-405`). 여기의 `$!` 는 바로 앞 `&` 로 이미 값이 있으므로 B1 과 같은 문제가 없다.
|
||||
- **`set -euo pipefail`**: B1 외에 의도치 않게 종료되는 새 경로는 없다.
|
||||
2. **`daemon.py`/`lib.sh`/`cli.py`**:
|
||||
- 옵트아웃은 리터럴 `1` 만 인정한다(`daemon.py:28-31`).
|
||||
- 옵트아웃 중에도 기존 데몬 PID 를 보고한다. stdout 은 비우고 stderr 에 한 줄 남긴다.
|
||||
- legacy 경로(`lib.sh:2024-2027`)가 python 본체와 바이트 단위로 같다.
|
||||
3. **`tests/conftest.py`**:
|
||||
- reaper 는 import 시점에 잡은 `_REAL_POPEN`(`:16-26`)으로 명령을 실행한다.
|
||||
- 선정자는 argv 형태와 스크립트 경로 전체로 대상을 고르고, 공백이 든 후보는 실제 파일일 때만 인정한다(`:32-58`).
|
||||
- 3 s 유예 뒤 SIGKILL 로 넘어간다(`:86-97`).
|
||||
- `mam_sandbox`(`:113`)는 finalizer 를 먼저 등록해(`:123`) 맨 마지막에 실행되게 하고, 옵트아웃을 기본으로 켠다(`:161`).
|
||||
- 세션 가드는 basetemp 기준으로만 판정한다(`:1384-1398`).
|
||||
4. **신규 테스트 결정성**:
|
||||
- 테스트는 16건이다. 15개 함수에 파라미터화 1개(`:130`, 2케이스)가 더해진 수로, 실제 수집 수와 같다.
|
||||
- 신규 2건은 임의 포트 리스너와 스크립트 원문 추출만 쓰므로 외부에 의존하지 않는다.
|
||||
- 시그널 테스트 4건을 3회 반복했고 매번 모두 통과했다. 소요 시간 편차는 ±0.06 s 이하였다(§2 표).
|
||||
- 포트 9 가정, 브로커 skip 조건, `from conftest import` 에 대한 판단은 지난 리뷰와 같다.
|
||||
5. **문서**:
|
||||
- monitor `SKILL.md:86-92` 의 핸들러 순서·폴백 설명이 코드와 일치한다.
|
||||
- `VERSIONS.md:47`(항목 3)과 `:51`(신규 테스트 16개, 수집 수와 같음)이 정확하다.
|
||||
- create `SKILL.md:71` 은 바뀌지 않았고 여전히 정확하다.
|
||||
6. **버전 락스텝 · 릴리스 커밋**:
|
||||
- `lib.sh:32`, `lib_py/loop/eager.py:16` 이 4.4.5 이고, `SKILL.md` 8개가 모두 `version: 4.4.5` 다.
|
||||
- `VERSIONS.md:9`(현재 버전)와 `:23-`(매트릭스), `README.md:5`, `BOOTSTRAP.md:5`, `MESSAGING.md:3` 도 4.4.5 다.
|
||||
- `verify_framework_version` 을 통과했다.
|
||||
- `134fe97` 은 버전 값을 바꾸지 않았다. `VERSIONS.md` 는 본문만 수정했다.
|
||||
- 릴리스 커밋 `38d8ccb` 의 형식은 그대로다.
|
||||
7. **테스트 · 정적 검사**: 아래 표.
|
||||
|
||||
| 테스트 · 정적 검사 | 결과 |
|
||||
|---|---|
|
||||
| **선택 실행 5파일** (issue19 16건, `test_issue5_phase1_delegation`, `test_tier2_component`, `test_version_consistency`, `test_herdr_shim_contract`) | **144 passed in 284.44 s, rc 0, skipped 0**. 지난 라운드 142건에 신규 2건이 더해졌다. 브로커에 의존하는 2건도 skip 없이 실제로 실행되었다 |
|
||||
| **시그널 테스트 반복** (`-k "sigterm or handshake or sub_stop"` ×3) | 매회 4 passed. 핸드셰이크 2.30–2.32 s, 폴백 SIGTERM 1.33 s, MQTT SIGTERM 1.16–1.22 s, `_sub_stop` 0.51–0.52 s |
|
||||
| **데몬 누수** (정밀 필터: argv0 `bash`/`sh` + `…/reconcile.sh` + `--subscribe`) | 실행 전후 모두 실제 데몬 2개(8152, 35505)만 같은 PID 로 살아 있었다. 샌드박스 데몬 **0개**. B1 테스트가 띄운 `sleep 30` 잔여도 0개 |
|
||||
| **#15 레지스트리 가드** | 실행 전후 실제 `.mam/jobs` 레코드 목록이 **같다**(추가 0건) |
|
||||
| **정적 검사** | `bash -n`(`reconcile.sh`, `lib.sh`) 통과. `py_compile`(`tests/conftest.py`, 신규 테스트) 통과. `git diff --check 8e7282c...HEAD` 클린. 작업 트리 클린, HEAD `134fe97` |
|
||||
|
||||
## 3. 비차단 권고
|
||||
|
||||
1. **연결 중 SIGTERM 이 최대 약 5 s 늦게 처리된다 (N1 의 부작용)**:
|
||||
- 실측(데몬 없이 python 단독): paho 2.1.0 `connect` 를 응답 없는 주소(`192.0.2.1`, `10.255.255.1`)로 걸고 1 s 뒤 TERM 을 보냈다.
|
||||
- 핸들러는 1.00 s 에 실행되었다.
|
||||
- 그러나 `connect` 는 5.00 s 시간 초과까지 계속되었고, 프로세스는 TERM 뒤 **4.0 s** 에 끝났다.
|
||||
- 수정 전에는 이 구간의 TERM 이 곧바로 프로세스를 죽였다(정리할 것은 없었다).
|
||||
- 영향: 브로커 주소가 블랙홀일 때만 생긴다. conftest reaper 는 3 s 유예(`conftest.py:92`) 뒤 SIGKILL 로 넘어가므로 회수는 된다. 다만 드물게 monitor `SKILL.md:92` 의 "`SIGKILL` is never required" 문구와 어긋난다.
|
||||
- 개선안(선택): 연결 단계에서만 핸들러가 예외를 던지게 하거나, 문서에 "연결 중에는 최대 약 5 s" 를 적는다.
|
||||
2. **fork-exec 창**: §1-3 의 후속 과제.
|
||||
3. **폴백의 남는 `sleep`**: 지난 권고를 유지한다. 문서화되어 있고, 간격(`RECONCILE_POLL_INTERVAL`) 안에 스스로 끝난다.
|
||||
4. **`_run_real` 시간 초과**: 지난 권고를 유지한다. 시간 초과 시 자식 `ps`/`pgrep` 을 끝내지 않는다(`conftest.py:19-26`).
|
||||
|
||||
## 4. GM 참고
|
||||
|
||||
- **HEAD 가 브리프의 `38d8ccb` 에서 `134fe97` 로 옮겨졌다.**
|
||||
- 2회차에서 agy·codex 의 PASS 와 grok 의 NOT PASS 는 모두 `8f9745b` 까지만 본 것이다.
|
||||
- `134fe97` 을 본 것은 creator 자체 검증과 이 리포트뿐이다. 만장일치 판정 전에 각 리뷰어가 `134fe97` 을 확인했는지 볼 것.
|
||||
- 이 저장소의 실제 데몬 PID 35505 는 수정 전 스크립트로 계속 돌고 있다. 재기동 여부는 오케스트레이터가 판단한다.
|
||||
- 내 지난 리뷰(`9114ac86`)에는 B1 누락이 있었다(§0-1).
|
||||
|
||||
## 5. 종합
|
||||
|
||||
- grok 의 N1(차단)과 B1 이 정확히 반영되었다.
|
||||
- python 핸들러는 연결 전에 설치된다. 시그널을 받은 구독자는 폴백하지 않고 정리 경로(rc 0, bash 143)로 끝난다.
|
||||
- `_sub_stop` 은 bash 3.2 에서 첫 자식 전에도 rc 143 으로 끝난다.
|
||||
- 두 수정 모두 회귀 테스트로 고정되었다.
|
||||
- 브리프의 7개 검증 항목을 브랜치 전체 기준으로 충족한다. 남은 사항(연결 중 약 5 s 지연, fork-exec 창)은 좁은 창이며 비차단이다. 설계 재검토는 필요 없다.
|
||||
|
||||
[VERDICT: PASS]
|
||||
@@ -0,0 +1,39 @@
|
||||
# Issue #19 review — Codex
|
||||
|
||||
PASS for clean HEAD `134fe9704c63bd9b1f46b64b01868f904f5a33cc`, base `8e7282c`. No new blocking findings. No planner escalation is needed.
|
||||
|
||||
Reviewed the branch log from the base and cumulative changes, carrying forward the full-file review through `8f9745b` from job `236cc993` and inspecting the entire new `8f9745b..HEAD` diff. Also checked `git diff 8e7282c...HEAD` for the cumulative subscriber implementation and cumulative diff hygiene. The six commits are `ce315d8`, `38d8ccb`, `4da4a50`, `c096cb4`, `8f9745b`, and `134fe97`. Only the subscriber script, its lifecycle tests, monitor documentation, and VERSIONS changed since the last review. HEAD and the tracked worktree remained stable and clean during verification.
|
||||
|
||||
## Latest corrections
|
||||
|
||||
- **Bash nounset:** `_sub_stop` now reads the fallback `$!` with nounset temporarily disabled, avoiding the Bash 3.2 unbound-variable failure before the first background child. It restores nounset, skips an empty PID, tolerates an exited child, and exits 143. The regression passed with no child and with an already-executing child on this host's actual **GNU Bash 3.2.57**. This test does not prove every fork-to-exec scheduling race; its comment accurately limits the exercised window.
|
||||
- **Connection-phase signals:** Python installs both handlers before configuring/connecting the MQTT client. The CONNACK wait uses the stop event, and connection-error/acceptance-failure branches check the recorded stop signal before choosing rc 3. The new local silent-listener test verifies signal logging, wrapper rc 143, prompt exit, and absence of polling fallback while awaiting CONNACK. Both existing connected-MQTT tests also passed, exercising the subscription-ready path.
|
||||
- **Scope of prompt shutdown:** The local test completes the TCP handshake and then waits for CONNACK. It does not exercise stalled DNS resolution or a blocked synchronous TCP/TLS connection operation; setting an event does not itself cancel those operations. Treat the documentation as the intended cleanup path, not a measured latency guarantee for every network stall.
|
||||
|
||||
## Cumulative review
|
||||
|
||||
- Python remains a background child of the waiting Bash wrapper. The trap forwards TERM and waits; strict-shell nonzero paths are intentionally tolerated or captured. Workspace checks use `(st_dev, st_ino)`, catch missing paths, and handle `getcwd` failure in the new check.
|
||||
- Internal Python statuses remain 0 normally, 3 for broker fallback, and 1 for lock setup errors. The shell still masks non-3 child failures with its final `exit 0`, as the base already did. This preserves existing behavior; it does not provide external lock-error rc 1.
|
||||
- Polling intentionally checks directory/lock presence without retaining the Python lock. An active foreground `--once` pass can defer its trap, and an interrupted sleep can survive until its finite interval expires. These limitations are now documented.
|
||||
- Literal `MAM_NO_SUBSCRIBER_DAEMON=1` remains the sole opt-out value. Python and legacy paths preserve existing-PID reporting, missing-monitor validation, and empty stdout/single stderr diagnostic when disabled without an existing daemon. Create documentation correctly qualifies when the PID line is empty.
|
||||
- The prior whitespace-path cleanup blocker remains fixed. The full-path matcher preserves root boundaries and rejects ordinary prompt-bearing wrappers; whitespace candidates require an existing file. The live-process regression verifies discovery at both fixture and basetemp roots and actual termination. Cleanup still uses a direct-child snapshot rather than recursive ownership tracking. Import-time Popen binding protects teardown from subprocess monkeypatches, and zombie handling avoids treating terminated processes as live.
|
||||
- Fixture cleanup is registered first within that fixture, and the session leak guard uses its own basetemp. Existing subprocess patches need not have been undone when cleanup runs, which is why import-time Popen binding matters.
|
||||
- There are now 16 lifecycle cases. Port 9 remains an environmental closed-port assumption. The new local listener uses a kernel-selected port. Two MQTT tests can skip when no broker is reachable; neither skipped here. Timing assertions and conftest imports passed on this host.
|
||||
- Version 4.4.5 remains synchronized across lib.sh, eager.py, all eight skill frontmatters, VERSIONS current/matrix, README/BOOTSTRAP variants, and MESSAGING. The release commit remains `release(v4.4.5)`. Updated documentation reflects early handler installation and correctly counts 16 lifecycle tests. The changelog's full-suite figure is recorded prior evidence, not a full-suite run conducted for this job.
|
||||
|
||||
## Validation
|
||||
|
||||
Fresh run on `134fe97`:
|
||||
|
||||
```text
|
||||
PYTHONDONTWRITEBYTECODE=1 .venv/bin/python -m pytest -p no:cacheprovider tests/test_issue19_subscriber_daemon_lifecycle.py tests/test_version_consistency.py -q
|
||||
19 passed in 39.39s
|
||||
```
|
||||
|
||||
`bash -n` passed for reconcile.sh and lib.sh. `git diff --check 8e7282c...HEAD` passed. Post-run raw `pgrep -f 'pytest-of-.*reconcile.sh --subscribe'` produced three matches; filtering for actual Bash/sh subscriber command shape found **zero daemons**. The session leak guard passed.
|
||||
|
||||
Earlier independent evidence: all five requested test files passed on `c096cb4` (**140 passed in 314.62s**); the lifecycle/version subset passed on `8f9745b` (**17 passed in 39.74s**). These are explicitly prior-commit results. This review reran the lifecycle and version checks affected by the latest correction, not the entire five-file suite.
|
||||
|
||||
No source edits, commits, production monitor/lock manipulation, real conversation purge, registry scrubbing, push, merge, or issue updates were performed. Workspace writes were limited to this report and authorized lifecycle events.
|
||||
|
||||
[VERDICT: PASS]
|
||||
@@ -0,0 +1,109 @@
|
||||
# Peer Review Report: Branch `fix/issue-19` (Comprehensive Review — Job `e2420d1b`)
|
||||
|
||||
- **Reviewer**: agy (session: `herdr:reviewer-agy-01`)
|
||||
- **Job ID**: `e2420d1b`
|
||||
- **Role**: Reviewer
|
||||
- **Target Branch**: `fix/issue-19`
|
||||
- **Base Commit**: `8e7282c` (v4.4.4)
|
||||
- **Evaluated Commits**:
|
||||
- `ce315d8` — `fix(issue-19): bound the subscriber daemon to its workspace, forward SIGTERM, keep sandboxes from leaking it`
|
||||
- `38d8ccb` — `release(v4.4.5): bump framework and eight skills to 4.4.5 in lockstep`
|
||||
- `4da4a50` — `test(issue-19): run the sandbox reaper through an import-time Popen`
|
||||
- `c096cb4` — `fix(issue-19): close the trap race, select daemons by argv shape, align docs with the polling fallback`
|
||||
- `8f9745b` — `fix(issue-19): match the daemon's whole script path so spaced sandbox paths are reaped`
|
||||
|
||||
---
|
||||
|
||||
## 1. Executive Summary
|
||||
|
||||
This peer review evaluated branch `fix/issue-19` through its latest commit `8f9745b`. The implementation addresses Issue #19, where subscriber daemons (`reconcile.sh --subscribe --idle-timeout 0`) spawned per workspace were leaking across test runs (202 orphan processes observed holding broker connections for deleted temp directories).
|
||||
|
||||
The complete branch includes the initial fix (`ce315d8`), version lockstep v4.4.5 (`38d8ccb`), and three peer-review hardening commits:
|
||||
1. `4da4a50`: Binds `_REAL_POPEN = _subprocess.Popen` at import time in `conftest.py` so test mocks cannot sabotage the sandbox reaper.
|
||||
2. `c096cb4`: Closes the race window between background spawn and PID assignment via `${!:-}` with `nounset` protection in bash 3.2, installs Python signal handlers before network operations, matches daemon processes strictly by argv shape to avoid false positives on prompt-bearing wrappers, and documents the polling fallback lock-presence contract.
|
||||
3. `8f9745b`: Extracts the full script path up to `/reconcile.sh --subscribe` so sandboxes or `--basetemp` paths containing whitespace are correctly identified and reaped.
|
||||
|
||||
All review criteria are satisfied with complete fidelity. A total of 144 test cases (19 targeted + 125 regression) executed with a 100% pass rate, zero orphan processes, and zero job registry leaks.
|
||||
|
||||
---
|
||||
|
||||
## 2. Detailed Technical Audit
|
||||
|
||||
### 2.1 Reconcile Shell Script (`reconcile.sh`)
|
||||
- **Background Execution & Return Code Contract**:
|
||||
- Python subscriber runs in background: `"$PYBIN" - <<'PYEOF' &; _sub_pid=$!; wait "$_sub_pid"; sub_rc=$?; _sub_pid=""; set -e`.
|
||||
- Exit code 0 indicates normal exit; exit code 3 indicates broker unavailability and triggers polling fallback; lock contention returns 1 and exits cleanly.
|
||||
- **Trap Safety & Bash 3.2 Compatibility**:
|
||||
- `_sub_stop()` handles `TERM` and `INT`.
|
||||
- In bash 3.2 (default `/bin/bash` on macOS), expanding `$!` before the first background child aborts with an unbound variable error under `set -u`. Commit `c096cb4` temporarily unsets `nounset` (`set +u; child="$!"; set -u`), ensuring the trap reliably exits 143 even if a signal arrives before child launch.
|
||||
- Signal forwarding safely suppresses errors (`kill -TERM "$child" 2>/dev/null || true; wait "$child" 2>/dev/null || true`).
|
||||
- **Identity Comparison `(st_dev, st_ino)`**:
|
||||
- `lock_ident = (_lock_st.st_dev, _lock_st.st_ino)` checks both device and inode.
|
||||
- `_workspace_gone()` catches `OSError` on `os.getcwd()` and detects replaced or deleted `.mam/monitor.lock` files, terminating the daemon cleanly when its workspace is removed.
|
||||
- **Early Signal Handlers**:
|
||||
- Signal handlers are installed *before* `client.connect(...)` and `client.loop_start()`.
|
||||
- If `SIGTERM` arrives while awaiting `CONNACK`, `stop_event.wait(0.1)` catches it immediately, cleanly exiting 0 without triggering the broker-down fallback.
|
||||
- **Polling Fallback**:
|
||||
- `sleep "$POLL_INTERVAL" & wait $! || true` allows immediate signal handling (exit 143).
|
||||
- Verifies presence of `$WORKSPACE_ROOT` and `.mam/monitor.lock` on each cycle.
|
||||
|
||||
### 2.2 Opt-Out Mechanism (`daemon.py`, `lib.sh`, `cli.py`)
|
||||
- **Literal Value Check**:
|
||||
- `daemon_disabled()` requires literal `"1"`. Loose values ("yes", "true", "0") do not disable the daemon.
|
||||
- **Deduplication & Output Parity**:
|
||||
- Active daemons continue to be reported during opt-out.
|
||||
- When opting out without an existing daemon:
|
||||
- Python CLI prints nothing to stdout and emits a single note to stderr.
|
||||
- Legacy shell (`lib.sh`) only echoes when `$pid` is non-empty (`if [ -n "$pid" ]; then echo "$pid"; fi`), preventing extraneous blank lines on stdout and matching Python behavior byte-for-byte.
|
||||
|
||||
### 2.3 Conftest Sandboxing & Reaper Hardening
|
||||
- **Import-Time Popen (`_REAL_POPEN`)**:
|
||||
- Capturing `_subprocess.Popen` at import time insulates fixture teardown from test monkeypatching or spying of `subprocess.run` / `subprocess.Popen`.
|
||||
- **Argv-Shape Matching (`_is_subscriber_daemon_argv`)**:
|
||||
- Confirms shell executable (`bash` or `sh`), verifies path prefix under `root`, and regex matches `(.*reconcile\.sh) --subscribe`.
|
||||
- Excludes processes that merely quote the command line in prompt arguments (e.g. `submit --prompt "..."`) or sibling roots (`/sbx1` vs `/sbx10`).
|
||||
- Supports whitespace-containing paths by verifying `os.path.isfile(script)`.
|
||||
- **Session Leak Guard**:
|
||||
- `subscriber_daemon_leak_guard` scopes strictly to pytest `basetemp`, guaranteeing no daemon outlives the test suite.
|
||||
|
||||
---
|
||||
|
||||
## 3. Version Lockstep v4.4.5 Audit
|
||||
|
||||
All components remain strictly locked at `4.4.5`:
|
||||
- `.agents/skills/lib.sh`: `MAM_VERSION="4.4.5"`
|
||||
- `.agents/skills/lib_py/loop/eager.py`: `FRAMEWORK_VERSION = "4.4.5"`
|
||||
- Frontmatter `version: 4.4.5` across all 8 skills (`create`, `stop`, `resume`, `status`, `monitor`, `delegate-job`, `loop`, `orc-onboard`).
|
||||
- `VERSIONS.md`: Current release overview, 8-row skills matrix, and detailed `v4.4.5` changelog accounting for all 16 lifecycle tests and architectural refinements.
|
||||
- Badges and headers in `README.md`, `README.ko.md`, `BOOTSTRAP.md`, `BOOTSTRAP.ko.md`, and `MESSAGING.md`.
|
||||
- Release commit title: `release(v4.4.5): bump framework and eight skills to 4.4.5 in lockstep`.
|
||||
- Remaining occurrences of `4.4.4` in markdown files are purely historical.
|
||||
|
||||
---
|
||||
|
||||
## 4. Test Suite Execution & Audits
|
||||
|
||||
### 4.1 Targeted Suites
|
||||
```bash
|
||||
.venv/bin/python -m pytest tests/test_issue19_subscriber_daemon_lifecycle.py tests/test_version_consistency.py
|
||||
```
|
||||
- **Result**: 19 passed in 37.99s (16 lifecycle tests + 3 version consistency tests).
|
||||
|
||||
### 4.2 Regression Suites
|
||||
```bash
|
||||
.venv/bin/python -m pytest tests/test_issue5_phase1_delegation.py tests/test_tier2_component.py tests/test_herdr_shim_contract.py
|
||||
```
|
||||
- **Result**: 125 passed in 240.03s.
|
||||
|
||||
### 4.3 Process & Registry Integrity
|
||||
- Orphan daemons under `/private/var/folders`: **0**
|
||||
- Real workspace monitor daemon: **Preserved intact** (PID 35505)
|
||||
- Registry file count in `.mam/jobs`: **52** before and after (0 leaks)
|
||||
|
||||
---
|
||||
|
||||
## 5. Conclusion
|
||||
|
||||
Branch `fix/issue-19` across commits `ce315d8` through `8f9745b` delivers a complete, robust, and well-verified solution to Issue #19. All criteria are fully satisfied.
|
||||
|
||||
[VERDICT: PASS]
|
||||
@@ -0,0 +1,51 @@
|
||||
# 🔍 Review Report — Job b6915b56 (`fix/issue-19` @ `134fe97`)
|
||||
|
||||
- **Reviewer**: `reviewer-grok-01` (role: reviewer) · 2026-09-13
|
||||
- **범위**: `git log`/`git diff` **`8e7282c...HEAD`**
|
||||
- **HEAD**: `134fe97` `fix(issue-19): arm the subscriber's stop handlers before it connects; read $! with nounset off`
|
||||
- **선행 NOT PASS**: `0dc6400b` N1 (핸들러가 subscribe 이후), B1 (`set -u` 에서 빈 `$!`)
|
||||
- **안전**: 실제 `.mam/monitor.lock`·워크스페이스 데몬 미접촉. 소스 무변경.
|
||||
|
||||
커밋: `ce315d8` → `38d8ccb` → `4da4a50` → `c096cb4` → `8f9745b` → **`134fe97`**. `git diff --check` 클린.
|
||||
|
||||
## N1 닫힘 — SIGTERM 핸들러를 connect 앞에
|
||||
|
||||
`reconcile.sh:300-341`: `signal.signal(SIGTERM/INT)` 을 `client.connect` / `loop_start` **앞**에 둔다. CONNACK 대기는 `stop_event.wait(0.1)`. 시그널이 있으면 `sys.exit(3)` 을 타지 않아 폴링 폴백으로 안 떨어진다. 메인 루프가 `"signal N received, exiting"` 을 찍고 `finally: loop_stop/disconnect`, 파이썬 rc 0, 래퍼 143.
|
||||
|
||||
`test_sigterm_during_broker_handshake_takes_the_clean_path`: 응답 없는 로컬 listen 포트, `connecting to` 후 1s, SIGTERM → 143, 로그에 signal 15, `falling back to polling` 없음. 직전 실패 테스트 `test_mqtt_loop_sigterm_exits_cleanly` 도 통과(핸들러가 subscribe 이전에 이미 있음).
|
||||
|
||||
paho `loop_start` 스레드는 핸들러가 만지지 않고, 메인만 `stop_event` 를 깨운 뒤 `loop_stop` — 충돌 없음.
|
||||
|
||||
## B1 닫힘 — 첫 `&` 전 `set -u`
|
||||
|
||||
`_sub_stop` (`reconcile.sh:80-97`): `_sub_pid` 가 비면 `set +u; child="$!"; set -u`. bash 3.2 에서 `${!:-}` 가 unbound 로 죽던 경로가 143 으로 끝남. `test_sub_stop_trap_exits_143_before_the_first_background_child` 가 자식 없음/있음 둘 다 143.
|
||||
|
||||
`&` 와 `_sub_pid=$!` 사이는 `$!` 로 커버. 이미 끝난 자식은 kill/wait no-op.
|
||||
|
||||
## 유지되는 계약
|
||||
|
||||
- 래퍼: 자식 0 → `exit 0`; 자식 3 → 폴링 후 0; 자식 1(lock 예외) → 폴링 없이 0; TERM → 143.
|
||||
- `_workspace_gone`: `(st_dev, st_ino)`, `getcwd` OSError, lock 교체/삭제.
|
||||
- 폴링: lock **존재**만. sleep `& wait`. 트랩은 sleep 자식을 안 죽임(문서화, ≤ POLL_INTERVAL).
|
||||
- 옵트아웃 리터럴 `1`, 기존 PID 보고, stdout 비움. argv 형태 회수, 접두/래퍼/공백 경로. 세션 가드 basetemp.
|
||||
|
||||
## 문서·락스텝
|
||||
|
||||
SKILL.md: 핸들러는 connect 전, 폴링은 presence, sleep 잔여. VERSIONS.md 항목 3·테스트 16개가 `134fe97` 과 맞음. **4.4.5** lockstep 유지 (`38d8ccb` 이후는 패치 커밋).
|
||||
|
||||
## 테스트
|
||||
|
||||
```
|
||||
env -u MAM_VERSION .venv/bin/python -m pytest -q \
|
||||
tests/test_issue19_subscriber_daemon_lifecycle.py \
|
||||
tests/test_issue5_phase1_delegation.py tests/test_tier2_component.py \
|
||||
tests/test_version_consistency.py tests/test_herdr_shim_contract.py
|
||||
```
|
||||
|
||||
**144 passed in 284.23s** (직전 141+1 fail → N1/B1 테스트 2 + 실패분 회복). issue19 **16 collected**. `bash -n` 클린. 실제 `pytest-of-*/…/bash …/reconcile.sh --subscribe` 0건(argv 에 브리프를 실은 래퍼만 `pgrep -f` 에 걸림). 워크스페이스 모니터 미접촉.
|
||||
|
||||
## 종합
|
||||
|
||||
N1·B1 이 코드·테스트로 닫혔고, 1회차 #19 수명 계약은 회귀 없음. 설계 재작업 불필요.
|
||||
|
||||
[VERDICT: PASS]
|
||||
Reference in new issue
Block a user