186 lines
12 KiB
Markdown
186 lines
12 KiB
Markdown
# Cross-Code Review: A-4 BaseAgentAdapter Introduction & Agent Knowledge Abstraction
|
|
|
|
**Job ID**: 98e3986f
|
|
**Reviewer**: cline
|
|
**Date**: 2026-08-14
|
|
**Scope**: A-4 BaseAgentAdapter 도입 및 에이전트 지식 추상화 — lint, operability, and loss cross-review
|
|
|
|
---
|
|
|
|
## 1. Change Summary
|
|
|
|
### Modified Files (5)
|
|
| File | Change |
|
|
|------|--------|
|
|
| `lib.sh:1120-1125` | F-1 fix: `VERIFY_SESSION_PYTHON` facade now fails explicitly (`echo ERROR + return/exit 1`) instead of silent empty string |
|
|
| `lib_py/verify_session.py:82-86` | `home = home_dir or os.environ.get("HOME_DIR", "")` → `home = resolve_home(home_dir)` |
|
|
| `lib_py/workspace_uuid.py:18-22` | `home = os.environ['HOME_DIR']` → `home = resolve_home()` |
|
|
| `reconcile.sh:323,344-358,863-868` | Replaces `exec(os.environ['MAM_VERIFY_PY'])` with direct `from lib_py.verify_session import ...`; replaces subprocess `load_state_json` with `from lib_py.state import load_state_json` (with subprocess fallback); removes `MAM_VERIFY_PY="$VERIFY_SESSION_PYTHON"` from both dry-run and write paths |
|
|
| `deploy/gitea-ci.yml:76-78` | `py_compile` glob changed from flat `lib_py/*.py` to recursive `lib_py/**/*.py` to catch new subdirectories |
|
|
|
|
### New Files (13)
|
|
| File | Purpose |
|
|
|------|---------|
|
|
| `lib_py/paths.py` | `resolve_home()` — unified HOME_DIR resolution contract (N0): explicit arg → HOME_DIR env → HOME env → expanduser('~'); raises ValueError on empty/root |
|
|
| `lib_py/state.py` | `load_state_json()` — Python direct state loader for agent-sessions.yaml/.db (N7); faithful extraction of shell `load_state_json` |
|
|
| `lib_py/agents/__init__.py` | Package init |
|
|
| `lib_py/agents/base.py` | `BaseAgentAdapter` abstract base, `SpawnSpec`, `DiscoveryContext` (N4) |
|
|
| `lib_py/agents/registry.py` | Static adapter registry: `get_adapter()`, `own_key()`, `agent_of_row()` (N4 & N5) |
|
|
| `lib_py/agents/__main__.py` | CLI bridge: `python -m lib_py.agents <facts|resolve>` |
|
|
| `lib_py/agents/adapters/__init__.py` | Adapters package init |
|
|
| `lib_py/agents/adapters/claude.py` | `ClaudeAgentAdapter` — `own_key = claude_session_id_own` |
|
|
| `lib_py/agents/adapters/agy.py` | `AgyAgentAdapter` — `own_key = agy_conversation_id_own` |
|
|
| `lib_py/agents/adapters/hermes.py` | `HermesAgentAdapter` — `own_key = hermes_conversation_id_own` |
|
|
| `lib_py/agents/adapters/cline.py` | `ClineAgentAdapter` — `own_key = cline_conversation_id_own` |
|
|
| `tests/test_a4_adapter_contract.py` | 3 contract tests: `resolve_home` contract, adapter registry, `agent_of_row` priority |
|
|
|
|
---
|
|
|
|
## 2. Syntax & Compilation Checks
|
|
|
|
| Check | Result |
|
|
|-------|--------|
|
|
| `bash -n lib.sh` | ✅ PASS |
|
|
| `bash -n reconcile.sh` | ✅ PASS |
|
|
| `py_compile` all `lib_py/**/*.py` (recursive, 11 files) | ✅ PASS |
|
|
| `py_compile tests/test_a4_adapter_contract.py` | ✅ PASS |
|
|
---
|
|
|
|
## 3. Import & Module Verification
|
|
|
|
| Check | Result |
|
|
|-------|--------|
|
|
| `from lib_py.paths import resolve_home` | ✅ OK |
|
|
| `from lib_py.state import load_state_json` | ✅ OK |
|
|
| `from lib_py.agents.registry import get_adapter, own_key, agent_of_row` | ✅ OK |
|
|
| `from lib_py.agents.base import BaseAgentAdapter, SpawnSpec, DiscoveryContext` | ✅ OK |
|
|
| All 4 adapter imports (claude, agy, hermes, cline) | ✅ OK |
|
|
| Circular import check | ✅ None — `base.py` → `paths`, `verify_session`; `registry.py` → `base` + adapters; adapters → `base` only |
|
|
|
|
### CLI Bridge Verification
|
|
```
|
|
$ python -m lib_py.agents facts claude
|
|
AGENT_NAME=claude
|
|
OWN_KEY=claude_session_id_own
|
|
|
|
$ python -m lib_py.agents resolve my-workspace-creator-hermes
|
|
hermes
|
|
```
|
|
|
|
### Registry Edge Cases
|
|
- `own_key('unknown')` → `None` ✅
|
|
- `own_key('')` → `None` ✅
|
|
- `get_adapter('CLAUDE')` → case-insensitive → `claude` ✅
|
|
|
|
---
|
|
|
|
## 4. Test Results
|
|
|
|
| Test Suite | Tests | Result |
|
|
|------------|-------|--------|
|
|
| `test_a4_adapter_contract.py` (new) | 3 | ✅ All PASS |
|
|
| `test_sanity.py` | 2 | ✅ All PASS |
|
|
| `test_tier1_unit.py` | 29 | ✅ All PASS |
|
|
| `test_orc_onboard.py` | 40 | ✅ All PASS |
|
|
| **Total** | **74** | **✅ All PASS** |
|
|
|
|
New contract tests verify:
|
|
1. `resolve_home` contract: explicit arg, HOME_DIR env, HOME/expanduser fallback
|
|
2. Adapter registry: all 4 agents have correct `name` and `own_key` (claude → `session`, others → `conversation`)
|
|
3. `agent_of_row` priority: explicit `agent` field > session name suffix > pane.cmd match
|
|
|
|
---
|
|
|
|
## 5. Design Analysis
|
|
|
|
### 5.1 `resolve_home()` Contract (N0)
|
|
Well-designed unified home resolution with fail-close semantics:
|
|
- **Priority chain**: explicit arg → `HOME_DIR` env → `HOME` env → `expanduser('~')`
|
|
- **Fail-close**: raises `ValueError` if result is empty or `/` — prevents root-relative silent fail-close
|
|
- **Behavioral change in `verify_session.py`**: Old code `home_dir or os.environ.get("HOME_DIR", "")` silently returned `""` → now raises `ValueError`. This is the intended fix — callers that previously got `""` would build paths like `/.claude/projects` (root-relative), which is a silent failure mode.
|
|
- **Behavioral improvement in `workspace_uuid.py`**: Old code `os.environ['HOME_DIR']` raised `KeyError` on missing env → now falls back to `HOME`/`expanduser`. More robust.
|
|
|
|
### 5.2 BaseAgentAdapter & Registry (N4 & N5)
|
|
Clean OOP design:
|
|
- `BaseAgentAdapter` defines `name`, `own_key` (abstract properties), `derive_session_name()`, `matches_session_name()`, `verify_session()`
|
|
- `matches_session_name()` checks `-{role}-{name}` suffix (creator/planner/reviewer) or bare `-{name}` — matches the kind detection logic in `lib.sh`
|
|
- `derive_session_name()` returns `f"{slug}-{role}-{name}"` — matches the shell `derive_session_name` function
|
|
- `agent_of_row()` implements strict priority: explicit `agent` field → session name suffix → `pane.cmd` exact/binary-path match → None
|
|
- Static registry with 4 singleton adapter instances — no dynamic registration needed
|
|
|
|
### 5.3 `state.py` Extraction (N7)
|
|
Faithful extraction of shell `load_state_json` (lib.sh:828-869):
|
|
- Identical DB-first, YAML-fallback logic
|
|
- Identical `clean_surrogates` recursive cleaner
|
|
- Identical `sqlite3.connect(timeout=60.0)` + `PRAGMA busy_timeout = 60000`
|
|
- **Enhancement**: accepts `yaml_path` parameter with env var fallback (`AGENT_SESSIONS_YAML` or `YAML_PATH`) — more flexible than shell version which always gets `YAML_PATH` from `env_python`
|
|
- **Difference**: returns dict directly instead of printing JSON to stdout — correct for in-process use
|
|
|
|
### 5.4 `reconcile.sh` Decoupling
|
|
- `exec(os.environ['MAM_VERIFY_PY'])` → `from lib_py.verify_session import verify_session_uuid, workspace_key` — eliminates the env-var code injection pattern
|
|
- `MAM_VERIFY_PY="$VERIFY_SESSION_PYTHON"` removed from both dry-run and write paths — no more env-var coupling
|
|
- `load_state_json` now tries direct Python import first, falls back to subprocess — performance improvement with safe fallback
|
|
|
|
### 5.5 CI glob fix (gitea-ci.yml)
|
|
Old: `py_compile .agents/skills/lib_py/*.py` (flat — misses `lib_py/agents/**/*.py`)
|
|
New: `glob.glob('.agents/skills/lib_py/**/*.py', recursive=True)` — correctly catches all nested Python files. **Necessary change.**
|
|
|
|
---
|
|
|
|
## 6. Findings
|
|
|
|
### F-1 (Low): Unused imports in `base.py` and `__main__.py`
|
|
- `base.py:3`: `import os, json, sqlite3` — none used in the file body
|
|
- `base.py:4`: `from typing import Optional, Dict, Any, List` — `List` unused
|
|
- `__main__.py:3`: `import sys, json` — `json` unused
|
|
- **Impact**: Dead imports; would be flagged by flake8 F401. No runtime impact.
|
|
- **Recommendation**: Remove unused imports for cleanliness.
|
|
|
|
### F-2 (Low): `VERIFY_SESSION_PYTHON` is now a dead variable
|
|
- `lib.sh:1121` defines `VERIFY_SESSION_PYTHON="$(cat ...)"` but no consumer remains — `reconcile.sh` was the only consumer (via `MAM_VERIFY_PY`), now removed.
|
|
- The `if-else` block still serves as a **file existence guard** (else branch fails explicitly), but the variable assignment itself is dead code.
|
|
- **Impact**: None at runtime — the guard is useful, the variable is harmless dead code.
|
|
- **Recommendation**: Could simplify to a pure existence check, but keeping it documents the historical facade pattern. Acceptable as-is.
|
|
|
|
### F-3 (Low): `reconcile.sh:326` still uses `os.environ['HOME_DIR']` directly
|
|
- Line 326: `home = os.environ['HOME_DIR']` — not changed to `resolve_home()`
|
|
- `reconcile.sh` always sets `HOME_DIR="$HOME_DIR"` via shell wrapper (line 71), so `KeyError` is impossible in practice
|
|
- **Impact**: Inconsistency with the `resolve_home()` pattern adopted in `verify_session.py` and `workspace_uuid.py`. No runtime risk.
|
|
- **Recommendation**: Could adopt `resolve_home()` for consistency, but not required since the env var is always set by the wrapper.
|
|
|
|
### F-4 (Low): `test_resolve_home_contract` test 3 fragility
|
|
- Test 3 creates `env_copy` with `HOME_DIR` popped, computes expected `home_val` from the copy, but calls `resolve_home()` with the **actual** `os.environ` (which may still have `HOME_DIR` set from prior tests or environment).
|
|
- If `HOME_DIR` was set in the environment when the test runs, `resolve_home()` would return the `HOME_DIR` value, but `home_val` would be `HOME`/`expanduser` — mismatch → test failure.
|
|
- **Impact**: Test passes in current environment (HOME_DIR not set during direct pytest run), but is fragile in environments where HOME_DIR is pre-set.
|
|
- **Recommendation**: Use `monkeypatch.delenv('HOME_DIR', raising=False)` to properly isolate the test.
|
|
|
|
---
|
|
|
|
## 7. Loss / Regression Analysis
|
|
|
|
| Concern | Status |
|
|
|---------|--------|
|
|
| `MAM_VERIFY_PY` references in codebase | ✅ Fully removed from all `.sh`/`.py` files (only in `.mam/jobs/` probe scripts — historical) |
|
|
| `load_state_json` shell function still used by other scripts | ✅ Yes — `stop_session.sh`, `orc_onboard.sh`, `run_loop.sh`, `lib.sh` itself. The new `state.py` module coexists; only `reconcile.sh` uses the Python import path. No loss. |
|
|
| `conftest.py` copies `lib_py/` to test sandboxes | ✅ `shutil.copytree(src_skills, ...)` copies entire `.agents/skills/` tree including new `lib_py/agents/` subdirectory |
|
|
| `deploy/remove.sh` includes new files | ✅ `.agents/skills/lib_py` directory entry covers `agents/` subdirectory automatically |
|
|
| `derive_session_name` logic matches shell version | ✅ `f"{slug}-{role.lower()}-{name}"` — identical |
|
|
| `matches_session_name` logic matches shell kind detection | ✅ Checks `-{role}-{name}` and `-{name}` suffixes — consistent |
|
|
| `own_key` naming convention | ✅ claude → `session_id_own`, others → `conversation_id_own` — matches existing schema |
|
|
| F-1 fix from prior review (job 11a99829) correctly applied | ✅ `return 1 2>/dev/null || exit 1` handles both sourced and executed contexts |
|
|
|
|
---
|
|
|
|
## 8. Verdict
|
|
|
|
The A-4 BaseAgentAdapter introduction and agent knowledge abstraction changes are **well-designed, correctly implemented, and fully tested**:
|
|
|
|
- **No regressions**: All 74 tests pass (including 3 new contract tests)
|
|
- **No code loss**: All existing consumers of `load_state_json` and `verify_session_uuid` continue to work; new Python module coexists with shell versions
|
|
- **Clean design**: `BaseAgentAdapter` + static registry + per-agent adapters follows OOP best practices; `resolve_home()` contract prevents root-relative silent fail-close
|
|
- **Proper decoupling**: `reconcile.sh` eliminates env-var code injection (`exec(os.environ['MAM_VERIFY_PY'])`) in favor of direct imports
|
|
- **CI coverage**: Recursive glob fix ensures new nested Python files are compiled in CI
|
|
|
|
The 4 findings (F-1 through F-4) are all **Low severity** — unused imports, dead variable, consistency gap, and test fragility. None block the PASS verdict.
|
|
|
|
[VERDICT: PASS] |