# Peer Review Report: Branch `fix/issue-10-11-13-15` & v4.4.3 Lockstep (Round 2 Cross-Review)

- **Reviewer**: `agy` (`herdr:reviewer-agy-01`)
- **Job ID**: `0f41ea47`
- **Target Branch**: `fix/issue-10-11-13-15` @ `fae70c1` (base `main` @ `bd3df89` v4.4.2)
- **Commits Reviewed**:
  - `c82ea75` — `fix(issue-10,11,13,15): complete agy purge, orchestrator purge, workspace-anchored registry`
  - `0282312` — `release(v4.4.3): bump framework and eight skills to 4.4.3 in lockstep`
  - `7610258` — `fix(issue-13): refuse to purge the conversation the caller is running in`
  - `f5a6575` — `test(issue-13): give each purge test fresh conversation ids`
  - `fae70c1` — `fix(issue-10,13): check every agy artifact for holders and purge orphaned leftovers`
- **Review Date**: 2026-09-13 (KST)

---

## 1. Executive Summary

Branch `fix/issue-10-11-13-15` resolves issues #10, #11, #13, and #15 while integrating peer-review feedback across two review loops. 

In loop 1 (round 1), codex identified two edge cases:
- **R1**: `agy` in-use check passed only the main DB file to `lsof`, missing processes holding open the presence lock (or sidecars) without the UUID in command line arguments or environment variables.
- **R2**: Orphaned leftovers (e.g. WAL, annotations, or presence lock surviving after an interrupted or pre-#10 purge where the main DB was already gone) failed to resolve as a purge target because resolution relied on DB existence.

Commit `fae70c1` directly resolves both codex findings through:
1. `AgyAgentAdapter.conversation_in_use`: Passes all conversation paths (`_artifact_paths`: DB, sidecars, brain, annotation, presence lock) to `lsof -t`.
2. `AgyAgentAdapter.has_artifacts`: Recognizes any existing artifact piece even if the primary DB has already been deleted.
3. `BaseAgentAdapter.has_artifacts`: Replaces the resolver's ad-hoc path fallback with a clean polymorphic method defaulting to `uuid in path and os.path.exists(path)`, correctly preventing `opencode` (shared DB) from falsely claiming non-opencode UUIDs.

Together with caller session recognition (`7610258`) and dynamic test fixtures (`f5a6575`), the branch is complete, robust, and verified.

All 250 targeted tests across 9 test suites passed with zero failures and zero leaks into the live `.mam/jobs` registry.

---

## 2. In-Depth Technical Verification

### (1) Codex R1 & R2 Solutions (`fae70c1`)
- **Codex R1 (`AgyAgentAdapter.conversation_in_use`)**:
  ```python
  def _artifact_paths(self, uuid: str, ctx: DiscoveryContext) -> list:
      db = self.artifact_path(uuid, ctx)
      root = self._cli_root(ctx)
      return ([db] + [f"{db}{suffix}" for suffix in ("-wal", "-shm", "-journal")]
              + [f"{root}/brain/{uuid}", f"{root}/annotations/{uuid}.pbtxt",
                 f"{root}/presence/{uuid}.lock"])

  def conversation_in_use(self, uuid: str, ctx: DiscoveryContext) -> bool:
      return conversation_in_use(uuid, self._artifact_paths(uuid, ctx))
  ```
  `conversation_in_use` passes every associated file path to `lsof -t`. Even if a process holds open only `presence/<uuid>.lock` (or a `-wal` sidecar) via `flock` and carries neither the UUID in `ps` command-line arguments nor any session environment variable, `lsof` catches the active file descriptor and refuses the purge. Verified by `tests/test_issue10_13_purge.py::test_purge_cli_refuses_when_only_the_presence_lock_is_held_open`.

- **Codex R2 (`AgyAgentAdapter.has_artifacts` & Orphan Resolution)**:
  ```python
  def has_artifacts(self, uuid: str, ctx: DiscoveryContext) -> bool:
      return any(os.path.exists(p) for p in self._artifact_paths(uuid, ctx))
  ```
  If the main DB was previously unlinked or deleted, any remaining orphaned WAL files, annotations, or presence locks are recognized by `has_artifacts`. `_resolve_purge_target` correctly identifies `agy` as the owning agent and purges the stranded artifacts completely. Verified by `tests/test_issue10_13_purge.py::test_purge_cli_resolves_agy_from_orphaned_leftovers`.

- **`BaseAgentAdapter.has_artifacts` Default Contract & Resolver Integration**:
  In `lib_py/agents/base.py`:
  ```python
  def has_artifacts(self, uuid: str, ctx: DiscoveryContext) -> bool:
      path = self.artifact_path(uuid, ctx)
      return uuid in path and os.path.exists(path)
  ```
  In `lib_py/agents/__main__.py` `_resolve_purge_target`:
  ```python
  for name, adapter, ctx in probes:
      try:
          if adapter.has_artifacts(uuid, ctx):
              return name, adapter, ctx
      except Exception:
          continue
  ```
  - **Shared Store Isolation**: `opencode` uses a shared database path (`opencode.db`) which does not embed the UUID (`uuid in path` is `False`). Thus `opencode` never falsely claims other agents' UUIDs in the fallback pass.
  - **Other Adapters (`claude`, `hermes`, `grok`, `codex`)**: All embed the UUID in their artifact paths (`f".../{uuid}.jsonl"`, etc.). Their resolution behavior remains identical to the historical contract.

### (2) Caller Session Protection (`7610258`) & Dynamic Fixtures (`f5a6575`)
- **`AGENT_SESSION_ENV` Verification**:
  Inspects `CLAUDE_CODE_SESSION_ID`, `ANTIGRAVITY_CONVERSATION_ID`, `HERMES_SESSION_ID`, `GROK_SESSION_ID`, and `OPENCODE_SESSION_ID`. If the caller's environment carries the target UUID, `conversation_in_use` immediately returns `True`.
- **Helper-Chain Pruning (`_is_agent_cli`)**:
  Excludes the purge command's own non-agent helper processes (python interpreter, shell runners) but halts exclusion at the nearest agent CLI ancestor (`claude`, `agy`, `hermes`, `grok`, `opencode`, `codex`). If that agent CLI ancestor carries the UUID on its command line, it is treated as an active holder.
- **Dynamic Test Fixtures (`f5a6575`)**:
  All purge tests in `test_issue10_13_purge.py` use dynamic `uuid4` fixtures (`conv` and `other`), preventing false "in use" detections caused by concurrent delegation wrappers whose command-line prompts carry test file diffs.

### (3) `orc_onboard.sh` Flow & Documentation Alignment
- Purge executes **before** registry mutation. A refused purge (exit code `5` due to in-use detection) leaves `.mam/agent-sessions.yaml` untouched.
- Confirmation gate (`--yes`) prevents accidental deletion (exit code `4`).
- Missing on-disk artifacts emit a warning and cleanly remove the UUID registration (exit code `0`).
- Past-tense grammatical correction maps `add -> added`, `remove -> removed`, eliminating the `removeed` typo.
- Full alignment with `multi-agent-mux-orc-onboard/SKILL.md` and `VERSIONS.md` v4.4.3.

### (4) Registry & Audit Log Resolution (#15)
- `default_registry_dir()` and `get_logs_dir()` prioritize `MAM_JOBS_DIR`/`DELEGATE_JOB_LOGS_DIR` &rarr; `$WORKSPACE_ROOT/.mam/...` &rarr; `<cwd>/.mam/...`.
- `job_subscriber.py` argument default updated to `default_registry_dir()`.
- Verified zero test jobs leaked into `.mam/jobs` during test suite runs.

### (5) Version Lockstep Completeness (`v4.4.3`)
- `MAM_VERSION="4.4.3"` in `.agents/skills/lib.sh`.
- `FRAMEWORK_VERSION = "4.4.3"` in `.agents/skills/lib_py/loop/eager.py`.
- 8 `SKILL.md` frontmatter versions: `4.4.3`.
- `VERSIONS.md`: Current release `v4.4.3`, 8-row skills matrix `4.4.3`, and changelog section updated to document codex R1/R2 and claude N1 refinements.
- Documentation badges in `README.md`, `README.ko.md`, `BOOTSTRAP.md`, `BOOTSTRAP.ko.md`, and `MESSAGING.md` header all synchronized.
- Verified no non-historical `4.4.2` references remain.

---

## 3. Test Suite Verification Results

All tests executed with active MQTT heartbeat tracking:

| Test Suite | Result | Details |
| :--- | :---: | :--- |
| `tests/test_issue10_13_purge.py` | 23/23 PASS | Includes presence lock holder test (R1) & orphaned leftover purge test (R2) |
| `tests/test_issue15_registry_isolation.py` | 7/7 PASS | Resolution hierarchy & onboarding leak prevention |
| `tests/test_orc_onboard.py` | 37/37 PASS | Onboarding CLI, remove, past tense message, ownership verification |
| `tests/test_a4_adapter_contract.py` | 33/33 PASS | Agent adapters contract compliance, 2-item purge base invariant |
| `tests/test_version_consistency.py` | 3/3 PASS | 3-way version lockstep, runtime mismatch rejection, env immutability |
| `tests/test_issue5_phase1_delegation.py` | 66/66 PASS | Delegation CLI, snapshot contracts, event publishing |
| `tests/test_tier1_unit.py` | 53/53 PASS | Unit tests, path resolution, B-9 logs directory |
| `tests/test_loop_cli.py` | 9/9 PASS | Loop CLI parsing, creator requirement, planner auto-resolution |
| `tests/test_issue5_phase4_runtime.py` | 5/5 PASS | Python core loop runtime execution & lifecycle |
| **Total Targeted Tests** | **250/250 PASS** | **All 250 tests passed cleanly (0 failures, 0 regressions)** |

### Live Registry Immutability Audit
- Pre-test and post-test inspections of `.mam/jobs` confirmed that zero test job records were written to the real repository registry.

---

## 4. Conclusion

The refinements in `fae70c1` thoroughly and elegantly address codex review findings R1 and R2 without introducing any regressions or architectural friction. All six review criteria are completely satisfied.

[VERDICT: PASS]