GodopuandClaude Fable 5.1
1cc84282f3
Final-round reports of the fix/issue-10-11-13-15 review loop: planner-reviewer-claude-01
(aa16fa5c), reviewer-agy-01 (0f41ea47), reviewer-grok-01 (3ea06fb6) and
planner-reviewer-codex-01 (79204d61), all [VERDICT: PASS] on fae70c1.
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01VCgu9Fa9iT7TbJUSzdq4Ff
135 lines
9.0 KiB
Markdown
135 lines
9.0 KiB
Markdown
# 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` → `$WORKSPACE_ROOT/.mam/...` → `<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]
|