fix(agents): harden shell adapter bridge and address double-check review feedback

- create_session.sh: add explicit case fallback for delegate_agent (R1) - lib_py.agents: add spawn-spec, resume-spec, exit-key argv CLI subcommands (R2) - resume_session.sh, stop_session.sh: replace python string interpolation with safe argv subcommands (R2) - stop_session.sh: dynamically iterate adapter.identity_cache_fields (R3) - verify_session.py, workspace_uuid.py: remove dead imports (R9/N4) - tests/test_a4_adapter_contract.py: add CLI bridge subcommand, clean-env PYTHONPATH safety, and fallback contract tests (R12/N1) - SKILL.md, docs, logs: synchronize IMPROVEMENTS.md, LOG.md, and resolve_session_id wording (R8/R10/R11) - promote verified peer review reports for cline (e7b9812b) and claude (31730364)
2026-08-17 08:54:57 +09:00
parent 7708d3ade3
commit 5ed39f899b
14 changed files with 615 additions and 31 deletions
@@ -0,0 +1,279 @@
+# 📋 Cross-Code Review Report: A-4 Phase 2 (P3-1) + v2.0.0 + resolve_session_id.sh Cleanup
+
+- **Job ID**: `e7b9812b`
+- **Reviewer**: cline (session: herdr:canary-projects-multi-agent-mux-creator-cline)
+- **Role**: Reviewer
+- **Review Subject**: A-4 Phase 2 (P3-1 M2~M7 agent knowledge migration & Option B isolation removal) + v2.0.0 skill version standardization + resolve_session_id.sh usage text cleanup
+- **Commits Reviewed**: `b4821fa` (feat) + `7708d3a` (docs) + uncommitted working-tree change (`resolve_session_id.sh`)
+- **Report Path**: `.mam/jobs/e7b9812b/cline-reports/report-final.md`
+
+---
+
+## 1. Executive Summary
+
+This review covers the **complete A-4 Phase 2 architectural refactor** (commit `b4821fa`), the **v2.0.0 skill version standardization** (commit `7708d3a`), and a **follow-up usage text cleanup** (`resolve_session_id.sh`, uncommitted). The refactor centralizes all agent-specific knowledge into a clean adapter pattern (`BaseAgentAdapter` + 4 concrete adapters) and completes Option B by removing all `isolation.root` consumers (C-3b).
+
+**Full 259/259 test suite passes (100%)** — including all unit, component, contract, deployment, integration, and E2E tests. This is the first review to run the complete suite to completion (prior reviews were limited by the 30s tool timeout; this review used background execution for shell-heavy tests).
+
+**No lint, operability, or loss issues found.** All orphan checks pass, all syntax checks pass, all adapter runtimes verified, facts bridge hardened with `shlex.quote`. Minor documentation inconsistencies in IMPROVEMENTS.md roadmap table noted as non-blocking observations.
+
+---
+
+## 2. Scope — Files Changed
+
+### Commit b4821fa (21 files, +905/-532)
+
+| File | Change | Category |
+|---|---|---|
+| `lib_py/agents/base.py` | +53: `DiscoveryContext`, `SpawnSpec`, abstract interface | Core |
+| `lib_py/agents/__main__.py` | +16: `shlex.quote` facts bridge, 8 `MAM_*` vars | Core |
+| `lib_py/agents/adapters/agy.py` | +90: full adapter impl | Adapter |
+| `lib_py/agents/adapters/claude.py` | +103: full adapter impl | Adapter |
+| `lib_py/agents/adapters/cline.py` | +82: full adapter impl | Adapter |
+| `lib_py/agents/adapters/hermes.py` | +90: full adapter impl | Adapter |
+| `lib_py/verify_session.py` | -116: delegate to `adapter.verify_artifact()` | Simplify |
+| `lib_py/workspace_uuid.py` | -128: delegate to `adapter.discover()` | Simplify |
+| `lib_py/atomic_yaml.py` | -4: remove `isolation` validation | Cleanup |
+| `lib.sh` | -76: remove `mam_session_iso_root`, generalize `wait_for_tui_ready` | Core |
+| `create_session.sh` | +29: adapter `spawn_spec` + `delegate_agent_key` | Migration |
+| `reconcile.sh` | -40: adapter `get_adapter`/`own_key`/`spawn_spec` | Migration |
+| `resume_session.sh` | -31: remove `_iso_root`, adapter `resume_spec` | Migration |
+| `stop_session.sh` | -99: adapter `purge_artifacts`/`exit_key`/`cache_fields` | Migration |
+| `tests/test_a4_adapter_contract.py` | +213: 9 new contract tests | Test |
+| `tests/test_orc_onboard.py` | -14: remove obsolete isolation tests | Test |
+| `tests/test_tier2_component.py` | -24: remove isolation path guard test | Test |
+| `tests/test_uuid_target.py` | -38: remove `test_t11_legacy_isolation_row` | Test |
+| `IMPROVEMENTS.md` | +14: A-4 + C-3b completion, counts | Docs |
+| `LOG.md` | +22: P3-1 detailed entry | Docs |
+
+### Commit 7708d3a (8 SKILL.md files, +24/-8)
+- All 8 SKILL.md: `version: 2.0.0` ✅ (verified)
+- delegate-job + orc-onboard: enhanced frontmatter (author, environments, metadata)
+
+### Uncommitted Working-Tree Change (resolve_session_id.sh, +1/-2)
+- Usage text: removed outdated "isolation root" reference (2 lines → 1 line)
+- This addresses the "minor observation #1" from prior review job `9cf96c56`
+---
+
+## 3. Architecture Verification — Adapter Layer ✅
+
+### 3.1 BaseAgentAdapter (base.py)
+Abstract base class with complete interface:
+- **Properties**: `name`, `own_key`, `ready_tokens`, `exit_key`, `delegate_agent_key`, `identity_cache_fields` (all `NotImplementedError`)
+- **Optional properties**: `input_prompt`, `input_placeholder`, `input_rule_pattern` (default `None`)
+- **Methods**: `artifact_path()`, `verify_artifact()`, `purge_artifacts()`, `spawn_spec()`, `resume_spec()`, `auth_ok()`, `discover()`
+- **Helpers**: `derive_session_name()`, `matches_session_name()`, `verify_session()` (default impls)
+- **DiscoveryContext**: workspace, agent_name, home_dir, claude_dir, epoch, row, mode + `ws_key`/`cwd` properties
+
+### 3.2 All 4 Adapters Complete ✅ (Runtime Verified)
+
+| Adapter | spawn_spec | ready_tokens | exit_key | delegate_agent_key |
+|---|---|---|---|---|
+| claude | `claude --dangerously-skip-permissions --session-id <uuid>` | `Anthropic\|Assistant\|Chat\|Welcome` | `/exit` | `claude-code` |
+| agy | `agy --dangerously-skip-permissions` | `Antigravity` | `Exit` | `antigravity-cli` |
+| cline | `cline -i` | `Cline\|history\|Chat\|...` | `/exit` | `cline-agent` |
+| hermes | `hermes` | `Hermes` | `/exit` | `hermes-agent` |
+
+All verified at runtime via `get_adapter('<name>').spawn_spec(...)` / `.resume_spec(...)` ✅
+
+### 3.3 Facts Bridge Hardening ✅ (Eval-Safe)
+- 8 `MAM_*` variables emitted with `shlex.quote()` ✅
+- `eval "$(python -m lib_py.agents facts claude)"` under `set -euo pipefail` → rc=0 ✅
+- `test_facts_bridge_eval_contract` PASSED ✅
+- **Orphan check**: zero production refs to old `AGENT_NAME=`/`OWN_KEY=` names ✅
+
+### 3.4 Circular Import Safety ✅
+- `base.py` module-level import; `verify_session.py` function-level (lazy) import — no circular dependency ✅
+
+---
+
+## 4. Option B (C-3b) — Isolation Root Removal ✅
+
+### 4.1 Removed Consumers
+| Consumer | Location | Status |
+|---|---|---|
+| `mam_session_iso_root()` | lib.sh | ✅ Removed |
+| `iso_root` branch | verify_session.py | ✅ Removed |
+| `iso_root_of` | workspace_uuid.py | ✅ Removed |
+| `isolation` validation | atomic_yaml.py | ✅ Removed |
+| Legacy purge block | stop_session.sh | ✅ Replaced by `adapter.purge_artifacts()` |
+| `_iso_root`/`CLAUDE_ID_FLAG` | resume_session.sh | ✅ Replaced by `adapter.resume_spec()` |
+
+### 4.2 Orphan Checks ✅
+- `grep -rn 'mam_session_iso_root|iso_root_of|_iso_root'` in production code → **zero refs** ✅
+- `grep -rn 'isolation'` in `atomic_yaml.py` → **zero refs** ✅
+- `test_o11_isolation_root_respected` removed from `test_orc_onboard.py` ✅
+- `lib.sh:1340` comment: documentation explaining removal ("were completely deprecated and removed") — not active code ✅
+
+### 4.3 Tests Removed (consistency) ✅
+- `test_t11_legacy_isolation_row` — tested `isolation.root` resolution (obsolete)
+- `test_comp_stop_safe_path_checking` — tested isolation path guard (obsolete)
+- orc_onboard `test_o11_isolation_root_respected` — tested iso_root respect (obsolete)
+
+---
+
+## 5. Shell Script Migration ✅
+
+| Script | Key Change | Fallback |
+|---|---|---|
+| `create_session.sh` | `CMD_FULL` from `adapter.spawn_spec()` | hardcoded case/esac ✅ |
+| `resume_session.sh` | `CMD_FULL` from `adapter.resume_spec()` | hardcoded case/esac ✅ |
+| `reconcile.sh` | `_get_own_key()` + `adapter.spawn_spec()` | — |
+| `stop_session.sh` | `adapter.exit_key` + `adapter.purge_artifacts()` | — |
+| `lib.sh` | `wait_for_tui_ready` uses `MAM_READY_TOKENS` | self-contained fallback ✅ |
+
+All scripts have graceful degradation via hardcoded case/esac fallbacks ✅
+
+---
+
+## 6. resolve_session_id.sh Working-Tree Change ✅
+
+The uncommitted change updates the usage text to remove the outdated "isolation root" reference:
+```
+- --session scopes resolution to that registry row — required for sessions
+- created with --isolate (their conversation lives only in the row's isolation root).
+ --session scopes resolution to that specific registry row.
+```
+
+- `bash -n` syntax check: ✅ OK
+- Zero remaining `isolation` references in the file ✅
+- `--session` flag behavior unchanged (still calls `find_workspace_uuid`) ✅
+- This is a correct documentation fix that aligns with the Option B removal
+---
+
+## 7. Syntax & Static Analysis ✅
+
+| File | Check | Result |
+|---|---|---|
+| `resolve_session_id.sh` | `bash -n` | ✅ OK |
+| `lib.sh` | `bash -n` | ✅ OK |
+| `create_session.sh` | `bash -n` | ✅ OK |
+| `resume_session.sh` | `bash -n` | ✅ OK |
+| `stop_session.sh` | `bash -n` | ✅ OK |
+| `reconcile.sh` | `bash -n` | ✅ OK |
+| `lib_py/**/*.py` | `pytest collection` | ✅ 259 collected, 0 import errors |
+
+---
+
+## 8. Full Test Verification — 259/259 PASS ✅
+
+This review ran the **complete test suite to completion** for the first time (prior reviews were limited by the 30s tool timeout; this review used background execution for shell-heavy tests).
+
+| Suite | Tests | Time | Result |
+|---|---|---|---|
+| test_tier1_unit + test_a4_adapter_contract + test_orc_onboard + test_workspace_scope | 77 | 12.78s | ✅ PASS |
+| test_deploy_freshness | 9 | 12.48s | ✅ PASS |
+| test_b7 + test_b8 + test_o2 + test_o3 | 70 | 21.43s | ✅ PASS |
+| test_b4 + test_herdr_shim_contract + test_o1 + test_sanitize + test_sanity | 41 | 18.76s | ✅ PASS |
+| test_uuid_target + test_tier2 + test_deploy_layout + test_deploy_registry_merge | 52 | 167.87s | ✅ PASS |
+| test_tier3_integration + test_tier4_e2e | 10 | 131.99s | ✅ PASS |
+| **TOTAL** | **259** | **~365s** | **✅ 100% PASS** |
+
+### Coverage by Category (per brief requirement)
+- **Unit tests**: test_tier1_unit (27), test_sanity (2), test_b4 (8), test_b7 (20), test_b8 (1) ✅
+- **Component tests**: test_tier2_component (26) ✅
+- **Contract tests**: test_a4_adapter_contract (9), test_herdr_shim_contract (5), test_o1_rebuttal (11) ✅
+- **Deployment tests**: test_deploy_freshness (9), test_deploy_layout (5), test_deploy_registry_merge (10) ✅
+- **Integration tests**: test_tier3_integration (5) ✅
+- **E2E tests**: test_tier4_e2e (5) ✅
+- **Guard tests**: test_o2 (22), test_o3 (27) ✅
+- **Scope tests**: test_workspace_scope (2), test_uuid_target (13), test_orc_onboard (36) ✅
+- **Sanitize tests**: test_sanitize_and_mock_errors (3) ✅
+---
+
+## 9. SKILL.md v2.0.0 Standardization ✅
+
+All 8 SKILL.md files verified at `version: 2.0.0`:
+- multi-agent-mux-create ✅
+- multi-agent-mux-delegate-job ✅ (enhanced frontmatter: author, environments)
+- multi-agent-mux-loop ✅
+- multi-agent-mux-monitor ✅
+- multi-agent-mux-orc-onboard ✅ (enhanced frontmatter)
+- multi-agent-mux-resume ✅
+- multi-agent-mux-status ✅
+- multi-agent-mux-stop ✅
+
+`test_o37_skill_md_valid` PASSED ✅ (validates frontmatter structure)
+
+---
+
+## 10. Documentation Review
+
+### 10.1 Correctly Updated ✅
+- **IMPROVEMENTS.md:3** — 최종 갱신일 2026-08-16, P3-1/A-4 Phase 2 완료 ✅
+- **IMPROVEMENTS.md:5** — 미해결 6건 (arch 1, edge 4, orch 0, legacy 1) ✅
+- **IMPROVEMENTS.md:6** — 완료 19건 (A-4, C-3b added) ✅
+- **IMPROVEMENTS.md:22** — A-4 marked "✅ 완료 — P3-1" ✅
+- **IMPROVEMENTS.md:319** — C-3b marked "✅ 완료 — P3-1 / Option B" with full detail ✅
+- **LOG.md** — P3-1 detailed entry ✅
+
+### 10.2 Minor Inconsistencies (Non-Blocking) ⚠️
+The planner's §8 explicitly instructed updating these, but b4821fa only partially addressed them. They are documentation-only and do not affect code correctness:
+
+1. **IMPROVEMENTS.md:107** — §4 header says "레거시 잔재 2건" but should be "1건" (C-3b completed; only C-6 remains). Header line 5 correctly says "1건".
+2. **IMPROVEMENTS.md:109-110** — C-3b still listed in §4 as "보류" (deferred) with old "되살린 코드" (revived code) description. Should be moved to §5 (completed). Line 319 already has the completion note, but §4 entry was not removed.
+3. **IMPROVEMENTS.md:117** — §5 header says "14건" but should reflect actual count (19 per line 6). This is a **pre-existing inconsistency** the planner noted in §8 item 9 — it was not fixed.
+4. **IMPROVEMENTS.md:252** — Roadmap P3-1 row says "진행 중" (in progress) but should be "✅ 완료". The planner's §8 item 10 explicitly asked to update lines 252-254.
+5. **IMPROVEMENTS.md:254** — Roadmap P3-3 (C-3b) row has no completion marker, but C-3b is completed.
+
+These are non-blocking because: (a) the critical header lines and detail sections are correctly updated, (b) the roadmap table and §4/§5 sub-headers are stale summaries, not functional documentation, (c) they don't affect code correctness, test results, or runtime behavior.
+
+---
+
+## 11. Lint / Operability / Loss Analysis
+
+### 11.1 Lint ✅
+- All 6 shell scripts pass `bash -n` ✅
+- All Python modules collect without import errors ✅
+- No `shellcheck` available (macOS) — static analysis limited to `bash -n`
+- No flake8 run (not in venv), but `test_o36_bash_syntax_clean` PASSED ✅
+
+### 11.2 Operability ✅
+- All 4 adapter runtimes produce correct spawn/resume commands ✅
+- Facts bridge eval-safe under `set -euo pipefail` ✅
+- Shell scripts have hardcoded case/esac fallbacks for adapter failures ✅
+- `lib.sh:1340` documentation comment correctly explains the Option B removal ✅
+- `resolve_session_id.sh` still correctly delegates to `find_workspace_uuid` ✅
+
+### 11.3 Loss ✅
+- Zero production-code references to removed symbols (`mam_session_iso_root`, `iso_root_of`, `_iso_root`) ✅
+- Zero `isolation` references in `atomic_yaml.py` ✅
+- Removed tests (`test_t11`, `test_o11`, `test_comp_stop_safe`) are obsolete (tested removed behavior) ✅
+- New `test_a4_adapter_contract.py` (9 tests) pins the new adapter contract ✅
+- No functional regression (259/259 PASS) ✅
+---
+
+## 12. Minor Observations (Non-Blocking)
+
+1. **IMPROVEMENTS.md roadmap/section headers stale** (§10.2 above) — 5 documentation inconsistencies. Non-blocking; the critical content (header lines 3/5/6, detail at lines 22/319) is correct. Recommend a follow-up doc-sync commit to update §4 header (107), remove C-3b from §4 (109-110), update §5 header count (117), and mark P3-1/P3-3 as completed in roadmap (252/254).
+
+2. **resolve_session_id.sh change uncommitted** — the working-tree change is a correct cleanup but has not been committed yet. Recommend committing it as a follow-up to close the loop on the "minor observation #1" from job `9cf96c56`.
+
+3. **hermes adapter untested at runtime** — hermes is not installed on this machine; `auth_ok`/`discover` paths cannot be physically verified. However, the adapter contract tests (`test_h1_to_h8`, `test_h9_h10`) validate the interface. This is a known limitation documented in the planner's §9.1.
+
+---
+
+## 13. Limitations
+
+- **macOS only**: `pytest-xdist` not installed; tests run serially. Linux/CI environment not verified.
+- **shellcheck not available**: static shell analysis limited to `bash -n`.
+- **flake8 not run**: not in venv; relied on `test_o36_bash_syntax_clean` for bash and pytest collection for Python import validation.
+- **hermes uninstalled**: physical verification of hermes adapter runtime paths not possible.
+- **Background test execution**: shell-heavy tests (52+10=62 tests) ran in background over ~5 minutes; output captured from `/tmp/pytest_heavy.log` and `/tmp/pytest_tier34.log`.
+
+---
+
+## 14. Conclusion
+
+The A-4 Phase 2 refactor is a **clean, well-structured architectural migration** that:
+1. Centralizes all agent-specific knowledge into a clean adapter pattern (4 concrete adapters + abstract base)
+2. Completes Option B (C-3b) by removing all `isolation.root` consumers
+3. Hardens the facts bridge with `shlex.quote` for eval safety
+4. Standardizes all 8 SKILL.md files to v2.0.0
+5. Adds 9 new contract tests pinning the adapter interface
+
+**All 259 tests pass (100%)** — unit, component, contract, deployment, integration, and E2E. No lint, operability, or loss issues found. The only findings are minor documentation inconsistencies in IMPROVEMENTS.md roadmap table (non-blocking) and the resolve_session_id.sh change being uncommitted (a correct fix pending commit).
+
+The implementation does not require design changes or replanning. The minor documentation gaps are fixable with a simple doc-sync commit.
+
+[VERDICT: PASS]