docs(reports): archive phase 2b unanimous pass review reports (a665d729, 7e7eea07, a7f1ee0e)
This commit is contained in:
Godopu committed
1 parent
8d7e010f9f
commit
e1203166d9
3 files changed
+143
No files matched your search
@@ -0,0 +1,69 @@
|
|||||||
|
# Issue #5 Phase 2b — Round 4 교차 리뷰 (Codex Round 3 R4 후속 결함 수정)
|
||||||
|
|
||||||
|
- **Job**: a665d729 / Reviewer: planner-reviewer-claude-01
|
||||||
|
- **리뷰 대상**: 커밋 `8d7e010` "fix(issue-5): record an accepted intent, not a refused one" (기준 `9a651cc`)
|
||||||
|
- **범위 고정**: brief에 내장된 diff와 `git diff 9a651cc..HEAD -- . ':(exclude).agents/reports'`를 바이트 비교 → **동일**(15,916 B). 변경 파일 4개: `lib_py/delegation/job_subscriber.py`, `docs/contracts/issue-5/phase2b-autocomplete.md`, `docs/contracts/issue-5/task.md`, `tests/test_issue5_phase2b_autocomplete.py`.
|
||||||
|
- **동결 스냅샷**: `mam-loop-freeze.03Uqb9`의 `job_subscriber.py` md5 = `9a651cc` 시점과 동일(수정 **전**). engine/notifier/autocomplete/lib.sh는 HEAD와 동일. 즉 이 루프 자신의 구독자 데몬(pid 10863)은 수정 전 코드를 실행 중이며, 수정은 다음 루프 기동(새 동결)부터 효력을 갖는다. B-13 설계상 당연한 결과이고 diff의 결함이 아니다.
|
||||||
|
|
||||||
|
## 1. 지적 사항 대조 (Codex Round 3)
|
||||||
|
|
||||||
|
지적: `_Watcher.on_message`에서 `seen_intents[jid].add(identity)`가 단조 seq 가드보다 **앞서** 실행되어, 늦게 도착한 과거 패킷이 자기 intent를 "본 것"으로 태우고 뒤따르는 정당한 재시도가 드롭된다.
|
||||||
|
|
||||||
|
수정 후 순서(`job_subscriber.py:144-179`, 직접 확인):
|
||||||
|
1. `data`가 dict가 아니면 intent 없음으로 취급(이전 코드의 `(data or {}).get` 경로 제거)
|
||||||
|
2. intent가 있으나 dict가 아니면 **드롭+경고**
|
||||||
|
3. identity 튜플 생성 → `in seen_intents` 대조를 `try/except TypeError`로 감싸 해시 불가면 **드롭+경고**
|
||||||
|
4. 이미 본 identity면 드롭(seq 바닥을 올리지 않음)
|
||||||
|
5. seq 단조 가드
|
||||||
|
6. **가드 통과 후에만** `seen_intents[jid].add(identity)`
|
||||||
|
7. 감사 로그 `received` 기록 → 큐 투입
|
||||||
|
|
||||||
|
brief의 요구 2)·3) 모두 충족. 요구 3)의 시나리오(`last_seq=10`, seq=9 거부 → 동일 intent seq=11 수신 → `qsize()==1`)가 `test_r4b_a_stale_packet_does_not_burn_the_intent_for_its_own_retry`에 그대로 있다.
|
||||||
|
|
||||||
|
## 2. 실측 재현
|
||||||
|
|
||||||
|
### 2-1. 신규 테스트 6종 — HEAD vs 수정 전 트리
|
||||||
|
`9a651cc` 워크트리에 HEAD의 테스트 파일을 복사해 실행:
|
||||||
|
|
||||||
|
| 결과 | 테스트 |
|
||||||
|
|---|---|
|
||||||
|
| 수정 전 **FAIL** | `stale_packet_does_not_burn_the_intent` (`seen_intents == {(7,'completed','T')}` 잔존), `unhashable_intent_is_refused` (`TypeError: unhashable type: 'list'` 콜백 탈출), `non_object_intent_is_refused` |
|
||||||
|
| 수정 전 PASS(보존 가드) | `duplicate_intent_does_not_advance_the_sequence_floor`, `absent_or_null_intent`, `real_retry_is_deduplicated_by_the_real_consumer` |
|
||||||
|
|
||||||
|
task.md P2B-07의 "3종 역검증 실패 / 3종 보존" 주장과 **정확히 일치**. HEAD에서는 파일 전체 65 passed.
|
||||||
|
|
||||||
|
### 2-2. 테스트 밖 엣지 — 실제 콜백에 직접 투입 (HEAD / 수정 전)
|
||||||
|
|
||||||
|
| 입력 | 수정 전 | HEAD |
|
||||||
|
|---|---|---|
|
||||||
|
| `data`가 비어있지 않은 list / str | **`AttributeError` 콜백 탈출** | 정상 수신(intent 없음 취급) |
|
||||||
|
| intent의 `event` 필드가 dict(중첩 해시 불가) | **`TypeError` 콜백 탈출** | 드롭+경고, `last_seq` 불변 |
|
||||||
|
| 같은 wire seq QoS1 재전달(+intent) | 1건 | 1건 |
|
||||||
|
| 거부된 replay(seq 9) 뒤 바닥(1)보다 크고 9보다 작은 정상 이벤트(seq 4) | 수신 | 수신 |
|
||||||
|
| 필드가 전부 없는 intent `{}` 두 번 | 1건 수신, `(None,None,None)` 기록 | 동일 (O-1 참조) |
|
||||||
|
|
||||||
|
`data`가 비객체인 경우는 task.md가 주장하지 않은 **추가 탈출 경로**인데 이번 수정으로 함께 닫혔다(paho 네트워크 스레드에서 콜백 예외는 해당 구독자의 모든 job 수신을 끊으므로 실제 가치가 있다).
|
||||||
|
|
||||||
|
### 2-3. 종단 테스트의 전제 확인
|
||||||
|
`test_r4b_a_real_retry_...`는 도달 불가 브로커(127.0.0.1:65432)로 `notifier.flush` 2회 후 감사 로그의 `published` 항목 2건을 실제 콜백에 투입한다. `publish_event.record_outcome`이 발행 성공 여부와 무관하게 `published` 감사 항목을 기록(G-1/G-2/G-4)함을 코드로 확인했으므로 전제가 성립하며, 두 항목의 wire seq가 다르고 `notification_intent`가 같음 → 1건만 큐에 남는다.
|
||||||
|
|
||||||
|
### 2-4. 린트·정합성
|
||||||
|
- `py_compile` OK. pyflakes/ruff는 venv에 없고 저장소에 line-length 설정도 없음(pyproject/setup.cfg/ruff.toml 부재). 신규 줄 중 `job_subscriber.py:161`(102자)이 100자를 넘지만 기존 줄(98·170·227·228)도 이미 넘으므로 관례 위반 아님.
|
||||||
|
- 테스트 실행 후 `git status` clean(샌드박스에 격리됨). HEAD 수집 총계 732 = 726 + 6.
|
||||||
|
|
||||||
|
## 3. 판단
|
||||||
|
|
||||||
|
**결함 없음(blocking).** 지적된 순서 역전은 "대조는 가드 앞, 기록은 가드 뒤"로 정확히 고쳐졌고, 그 논리(중복은 시퀀스를 소비할 자격이 없고, 전달 안 된 이벤트는 본 것이 아니다)가 코드 주석·계약 문서·테스트에 일관되게 적혀 있다. 방어적 파싱 강화는 콜백 예외 탈출 경로 3가지(비객체 intent, 해시 불가 필드, 비객체 `data`)를 닫았다.
|
||||||
|
|
||||||
|
## 4. 비차단 관찰 (다음 라운드/Phase 3 참고)
|
||||||
|
|
||||||
|
- **O-1. 식별 필드가 전부 없는 intent는 "안정된 신원"이 아니다.** `{}`나 `{"foo":1}`은 identity `(None,None,None)`을 얻어 **첫 번째만 수신**되고 이후 같은 꼴은 모두 replay로 드롭된다. 정상 발행자(`notifier.intent_for`)는 세 필드를 항상 채우므로 실제 경로에서는 발생하지 않고, 발생하더라도 방향이 fail-closed(드롭)라 이중 적용은 없다. 다만 phase2b-autocomplete.md의 새 문장 "안정된 신원을 만들 수 없는 intent는 거부한다"와 코드가 정확히 맞으려면 `identity` 생성 직후 `if any(f is None for f in identity): 드롭+경고` 한 줄이면 된다. **검증**: HEAD 워크트리에 이 4줄을 적용하고 phase2b·phase1 테스트 131 passed, `{}`/`{"foo":1}`는 드롭되고 정상 intent는 1회 수신됨을 확인.
|
||||||
|
- **O-2. task.md P2B-07 문구의 개수.** "신규 회귀 6종(3종 실패 역검증, 3종 보존) + 종단 테스트"로 읽히지만 종단 테스트는 6종에 **포함**된다(`test_r4b_` 함수는 정확히 6개, 보존 3종 중 하나가 종단 테스트). "6종(그중 1종은 종단)"으로 고치면 된다.
|
||||||
|
- **O-3. 운영 노트.** 동결 스냅샷 원리상 현재 실행 중인 루프의 구독자는 수정 전 코드다. 이 라운드의 알림 재시도가 stale-first 상황을 만나면 옛 결함이 재현될 수 있으나, 재시작 후 디스크 폴백 + 멱등 ACK가 종단을 보증한다(기존 계약). 조치 불필요, 인지만.
|
||||||
|
- 이전 라운드 인계 사항(O-7 direct 잡의 `claim` 토큰 발급, `_delegate_py_bin` 앵커링, redispatch 시 shadow dispatch 갱신, `preexec_fn`)은 이번 diff 범위 밖이며 그대로 유효하다.
|
||||||
|
|
||||||
|
## 5. 전체 스위트
|
||||||
|
|
||||||
|
`.venv/bin/python -m pytest tests -q -p no:cacheprovider` (백그라운드 1회, 정상 종료): **732 passed in 802.02s (0:13:22)**, FAILED/ERROR 0건. 이전 라운드 726 → 732 (+6, 신규 `test_r4b_*` 6종). 이번에는 이전 라운드(19ac4c53)에서 있었던 비정상 종료가 재현되지 않았다.
|
||||||
|
|
||||||
|
[VERDICT: PASS]
|
||||||
@@ -0,0 +1,24 @@
|
|||||||
|
# Issue #5 Phase 2b final correction review
|
||||||
|
|
||||||
|
Job: a7f1ee0e
|
||||||
|
Reviewer: planner-reviewer-codex-01 (planner,reviewer)
|
||||||
|
Reviewed HEAD: 8d7e010; correction relative to 9a651cc.
|
||||||
|
|
||||||
|
The remaining R4 ordering defect is resolved. No blocking findings in the requested correction; no supplementary edits or escalation required.
|
||||||
|
|
||||||
|
## Assessment
|
||||||
|
|
||||||
|
- `_Watcher.on_message` now adds an intent to seen_intents only after the wire sequence passes the monotonic check. A rejected stale packet cannot suppress a subsequent valid retry.
|
||||||
|
- Existing accepted-intent duplicate rejection remains intact.
|
||||||
|
- Non-object intent values and unhashable identity components are rejected without escaping the callback. Access to data is guarded by an object-type check.
|
||||||
|
- The change is confined to subscriber validation, tests and documentation. Previously reviewed dispatch-token/CAS, manifest verification and notification settlement fixes remain unchanged.
|
||||||
|
|
||||||
|
## Verification
|
||||||
|
|
||||||
|
- `./.venv/bin/python -m pytest tests/test_issue5_phase2b_autocomplete.py -q`: **65 passed in 1.28s**.
|
||||||
|
- Independent real-subscriber probe: initialize last_seq=10; send the same intent with wire sequences 9, 11, 12. Result: **one queued event, last_seq=11**. Thus stale-first rejection, fresh-retry acceptance and subsequent duplicate rejection all hold together.
|
||||||
|
- `git diff --check 9a651cc..HEAD`: clean.
|
||||||
|
- Working tree clean before writing the assigned report; no production or fixture files modified by this reviewer.
|
||||||
|
- Full preexisting suite was not rerun in this review. This verdict addresses the requested correction and carries forward the prior resolved findings; it does not claim independent full-suite or other-reviewer approval.
|
||||||
|
|
||||||
|
[VERDICT: PASS]
|
||||||
@@ -0,0 +1,50 @@
|
|||||||
|
# Issue #5 Phase 2b Round 3 review (`8d7e010`)
|
||||||
|
|
||||||
|
- **Reviewer**: `reviewer-creator-grok-01`
|
||||||
|
- **Job**: `7e7eea07`
|
||||||
|
- **HEAD**: `8d7e010` (`fix(issue-5): record an accepted intent, not a refused one`)
|
||||||
|
- **Prior Grok residual**: `0d502d33` item 1 — `seen_intents` recorded before the seq check
|
||||||
|
- **Tests this turn**: `pytest tests/test_issue5_phase2b_autocomplete.py tests/test_issue5_phase2a_loop.py` → **156 passed**. Phase 2b file: **65 collected**. Collect-only **732**. `git diff --check 9a651cc..HEAD` clean.
|
||||||
|
|
||||||
|
The live callback was re-read and the stale-first probe from `0d502d33` was re-run against this tree.
|
||||||
|
|
||||||
|
---
|
||||||
|
|
||||||
|
## Adjudication
|
||||||
|
|
||||||
|
[ADJUDICATION: SUSTAINED]
|
||||||
|
|
||||||
|
`job_subscriber.py:143-179` now splits the two operations:
|
||||||
|
|
||||||
|
1. **Lookup** of `identity in seen_intents` still runs *before* the seq guard, so a replay with a fresh wire seq is dropped.
|
||||||
|
2. **`seen_intents.add`** runs *after* `seq > last_seq` and after `last_seq` is advanced. A refused packet does not mark its intent as accepted.
|
||||||
|
|
||||||
|
Malformed `notification_intent` (non-object, or unhashable field) is dropped inside defensive parsing with a warning; `TypeError` does not escape the MQTT callback.
|
||||||
|
|
||||||
|
Required regression `test_r4b_a_stale_packet_does_not_burn_the_intent_for_its_own_retry`: `last_seq=10`, seq=9 refused, same intent at seq=11 accepted, `qsize()==1`, a further replay stays at 1.
|
||||||
|
|
||||||
|
**Reproduced this turn** (same probe that failed on `9a651cc`):
|
||||||
|
|
||||||
|
```
|
||||||
|
stale seq=9 last=10 -> qsize=0 seen=set() last=10
|
||||||
|
retry seq=11 same intent -> qsize=1
|
||||||
|
replay seq=12 -> qsize=1
|
||||||
|
unhashable: raised=False qsize=0 last=0
|
||||||
|
```
|
||||||
|
|
||||||
|
Also covered: refused replay does not raise `last_seq` (`test_r4b_a_duplicate_intent_does_not_advance_the_sequence_floor`); end-to-end two `notifier.flush` attempts with distinct wire seqs land once on the real callback (`test_r4b_a_real_retry_is_deduplicated_by_the_real_consumer`).
|
||||||
|
|
||||||
|
---
|
||||||
|
|
||||||
|
## Residuals (unchanged, not this patch)
|
||||||
|
|
||||||
|
1. No production caller of `evaluate` / `recover` (Phase 3; §2-1).
|
||||||
|
2. `started` does not mint `dispatch_token`.
|
||||||
|
3. Bookkeeping may write any `pending_notification` value, not only `None`.
|
||||||
|
4. `pick_pending` split-lock.
|
||||||
|
|
||||||
|
No `[ESCALATE: PLANNER]`. This is the local order fix the residual asked for.
|
||||||
|
|
||||||
|
---
|
||||||
|
|
||||||
|
[VERDICT: PASS]
|
||||||
Reference in new issue
Block a user