Godopu
b4821fafa8
- Option B / C-3b: Deprecate isolation.root and remove its 4 legacy consumers in lib.sh, workspace_uuid.py, verify_session.py, and stop_session.sh. - M2~M7 Migration: Implement artifact_path, verify_artifact, purge_artifacts, spawn_spec, resume_spec, auth_ok, discover, ready_tokens, exit_key, and delegate_agent_key across BaseAgentAdapter and all 4 adapters (Claude, Agy, Hermes, Cline). - Migrate shell duplications in lib.sh, create_session.sh, resume_session.sh, stop_session.sh, and reconcile.sh to python -m lib_py.agents facts. - Hardening & Corrections: Fix Cline resume_spec flag to '-i --id', apply shlex.quote across all facts variables with MAM_ prefix, add safe fallback in wait_for_tui_ready. - Add comprehensive contract tests in tests/test_a4_adapter_contract.py and sync IMPROVEMENTS.md / LOG.md. - Verified: 100% PASS across unit, component, contract, and integration tests.
155 lines
8.3 KiB
Markdown
155 lines
8.3 KiB
Markdown
# 📋 Cross Review Report — Job 8fc5b0bd (P3-1 / A-4 Phase 2 Reviewer-feedback fix)
|
|
|
|
- **Reviewer**: cline (session: herdr:canary-projects-multi-agent-mux-creator-cline)
|
|
- **Job**: 8fc5b0bd (follow-up to Job 59467505 NOT PASS)
|
|
- **Scope**: Verify the implementation addressed the 4 blocking issues from the prior NOT PASS review.
|
|
- **Date**: 2026-08-16
|
|
|
|
---
|
|
|
|
## 1. Executive Summary
|
|
|
|
The implementer addressed **all 4 blocking issues** raised in the prior NOT PASS review
|
|
(Job 59467505). The `cline resume_spec --resume` bug is fixed (`--id`), `auth_ok`/`discover`
|
|
are implemented in all 4 adapters, `resume_session.sh` and `reconcile.sh` are migrated to the
|
|
adapter layer, and contract tests for `spawn_spec`/`resume_spec`/`auth_ok`/`discover` values
|
|
were added and pass. 132 change-relevant tests pass with 0 failures; `py_compile` is clean;
|
|
`IMPROVEMENTS.md`/`LOG.md` are synchronized. One minor non-blocking observation remains
|
|
(create_session.sh auth not yet wired to `adapter.auth_ok`), which is out of the brief's
|
|
explicit scope.
|
|
|
|
**Verdict: PASS.**
|
|
|
|
---
|
|
|
|
## 2. Prior NOT PASS Issues — Resolution Status
|
|
|
|
### 2.1 [FIXED] cline `resume_spec` used non-existent `--resume` flag
|
|
- **Prior**: `cline.py:76` emitted `--resume`, but `cline --help` only exposes `--id`.
|
|
- **Now**: `cline.py:75-78` emits `f"{binary} -i --id {session_uuid}"` (materialized) /
|
|
`f"{binary} -i"` (non-materialized).
|
|
- **Verification**: `cline --help` → `--id <session-id> Resume an existing session by ID`
|
|
(no `--resume`). Contract test `test_adapter_spawn_and_resume_specs` (line 181-182)
|
|
asserts `cline -i --id u1` (materialized) and `cline -i` (non-materialized). ✅
|
|
|
|
### 2.2 [FIXED] `auth_ok` / `discover` unimplemented (2 of 7 adapter methods)
|
|
- **Prior**: `auth_ok` and `discover` were absent from `base.py` and all adapters.
|
|
- **Now**:
|
|
- `base.py:100-104` declares both as abstract (`raise NotImplementedError`).
|
|
- `claude.py:99-110` — `auth_ok` dual-mode (`run_cmd` callable for test injection /
|
|
`subprocess` for prod; checks `claude auth status` → `"loggedIn":true`).
|
|
- `agy.py:94-96` — `auth_ok` checks `~/.gemini/oauth_creds.json` or antigravity-oauth-token.
|
|
- `hermes.py:80-81` / `cline.py:80-81` — `auth_ok` returns `True` (no auth gate).
|
|
- `claude.py:112-121` — `discover` globs `{claude_dir}/{ws_key}/*.jsonl`, verifies each.
|
|
- `agy.py:98-108` — `discover` reads `last_conversations.json[ws]`, verifies artifact.
|
|
- `hermes.py:83-96` — `discover` queries `state.db` sessions by `cwd`.
|
|
- `cline.py:83-100` — `discover` scans `~/.cline/data/sessions/*`, verifies each.
|
|
- `workspace_uuid.py:74-81` — disk-scan fan-out replaced by `adapter.discover(ctx)`.
|
|
- **Contract tests**: `test_adapter_auth_ok` (line 184-201) and `test_adapter_discover`
|
|
(line 203-256) verify all 4 agents. Both pass. ✅
|
|
|
|
### 2.3 [FIXED] `resume_session.sh` / `reconcile.sh` not migrated to adapters
|
|
- **resume_session.sh** (line 83-93): `CMD_FULL` now computed via
|
|
`adapter.resume_spec('$RESOLVED_BIN', '$UUID', mat)` where `mat = adapter.verify_artifact(...)`.
|
|
The `materialized` flag (artifact exists on disk) selects `-r`/`--session-id` (claude) or
|
|
`--id`/bare (cline) — a behavioral improvement: do not attempt to resume a session whose
|
|
artifact is absent. Hardcoded fallback case retained as a safety net. `_iso_root` branch
|
|
fully removed. ✅
|
|
- **reconcile.sh**:
|
|
- `row_agent(s)` (line 590-591) delegates to `agent_of_row(s)` from registry.
|
|
- `_pin_and_verify_resume` (line 438-441) uses `_get_own_key(agent)` from registry.
|
|
- `OWN_KEY_BY_AGENT` (line 593-595) built from `_get_own_key(a)` for all 4 agents.
|
|
- Auto-register `cmd_full` (line 540-541) uses `_adapter.spawn_spec(agent)`.
|
|
- The 4-way hardcoded spawn/own-key fan-outs are now adapter-driven. ✅
|
|
|
|
### 2.4 [FIXED] No contract tests for `spawn_spec` / `resume_spec` values
|
|
- **Now**: `test_adapter_spawn_and_resume_specs` (line 164-182) asserts exact output strings
|
|
for all 4 agents' `spawn_spec` and `resume_spec` (materialized + non-materialized):
|
|
- claude: `--dangerously-skip-permissions --session-id u1` (spawn) / `-r u1` (resume,mat)
|
|
- agy: `--dangerously-skip-permissions` (spawn) / `--conversation u1` (resume,mat)
|
|
- hermes: `hermes` (spawn) / `hermes --resume u1` (resume,mat)
|
|
- cline: `cline -i` (spawn) / `cline -i --id u1` (resume,mat) / `cline -i` (resume,!mat)
|
|
- Plus `test_adapter_auth_ok` and `test_adapter_discover`. Total: 9 contract tests, all pass. ✅
|
|
|
|
---
|
|
|
|
## 3. Test Execution (Independent)
|
|
|
|
| Group | Files | Result | Time |
|
|
|---|---|---|---|
|
|
| Contract | test_a4_adapter_contract.py | **9 passed** | 0.13s |
|
|
| Unit | test_tier1_unit.py, test_orc_onboard.py | **66 passed** | 13.48s |
|
|
| UUID | test_uuid_target.py | **12 passed** | 78.35s |
|
|
| Tier2 | test_tier2_component.py, test_b4_session_created.py | **45 passed** | 56.05s |
|
|
| **Total** | | **132 passed, 0 failed** | — |
|
|
|
|
- `py_compile` clean on all 9 changed `.py` files.
|
|
- Removed tests (`test_t11_legacy_isolation_row`, `test_comp_stop_safe_path_checking`)
|
|
correctly tested the now-deprecated `isolation.root` feature — removals are justified.
|
|
- Full-suite count per LOG.md: 259 passed (consistent with +3 new contract tests over prior 256).
|
|
|
|
---
|
|
|
|
## 4. Documentation Sync
|
|
|
|
- `IMPROVEMENTS.md`: A-4 marked ✅완료 (P3-1), C-3b ✅완료; completed 17→19, pending 8→6.
|
|
- `LOG.md`: New P3-1 section documents every migrated file (base/adapters/__main__/verify_session/
|
|
workspace_uuid/atomic_yaml/lib.sh/create/resume/reconcile/stop/tests), records the
|
|
`cline resume_spec -i --id` fix, and the 259-pass result.
|
|
|
|
---
|
|
|
|
## 5. Non-Blocking Observations
|
|
|
|
### 5.1 `create_session.sh` auth not yet wired to `adapter.auth_ok`
|
|
`create_session.sh:96-119` still contains a 4-way hardcoded auth fan-out (claude/agy/hermes/cline).
|
|
The `auth_ok` adapter method is now implemented and tested but is **not yet invoked** from this
|
|
script, leaving two sources of truth for auth logic. The brief explicitly scoped shell-script
|
|
migration to `resume_session.sh` and `reconcile.sh` only, so this is **out of scope for this round**
|
|
and not a blocker. Recommendation: wire `create_session.sh` auth to `adapter.auth_ok` in a future
|
|
increment to close the last auth fan-out.
|
|
|
|
### 5.2 reconcile.sh entry-field metadata still agent-branched
|
|
`reconcile.sh:564-583` still branches on agent for entry metadata (claude `tui` block, agy
|
|
`mcp_attachments`, `child_pid`). These are agent-specific *metadata* with no corresponding adapter
|
|
method (no `entry_metadata` defined), so they are arguably not "agent command knowledge" and
|
|
remain acceptable. Not a blocker.
|
|
|
|
### 5.3 Environmental e2e hang (pre-existing, not a regression)
|
|
Orphaned `reconcile.sh --subscribe --idle-timeout 0` processes accumulate from the
|
|
subprocess-spawning test suites (test_tier2/test_b4/test_uuid). These caused the prior review's
|
|
environmental hang and are a pre-existing infrastructure issue, **not** a regression introduced by
|
|
this change. All orphans were cleaned (0 remaining) before final test runs.
|
|
|
|
---
|
|
|
|
## 6. Lint / Compile / Loss Checks
|
|
|
|
- **Lint/compile**: `py_compile` clean on `base.py`, all 4 adapters, `__main__.py`,
|
|
`verify_session.py`, `workspace_uuid.py`, `atomic_yaml.py`.
|
|
- **No lost functionality**: removed `mam_session_iso_root` (lib.sh), `iso_root_of`
|
|
(workspace_uuid.py), isolation validity check (atomic_yaml.py) — all consumers of the
|
|
deprecated `isolation.root` row; removed tests aligned with removed features.
|
|
- **No orphaned imports**: adapters import `os/json/glob/sqlite3/subprocess/shutil` as needed.
|
|
|
|
---
|
|
|
|
## 7. Behavioral-Change Assessment
|
|
|
|
The `materialized` parameter in `resume_spec` is a deliberate, contract-tested behavioral
|
|
improvement: when the session artifact is absent (`verify_artifact` False), the adapter starts a
|
|
fresh session bound to the UUID (`--session-id` for claude, bare `-i` for cline) instead of
|
|
attempting to resume a non-existent history (`-r`/`--id`). This avoids resume failures on missing
|
|
artifacts. The fallback case in `resume_session.sh:86-92` preserves the materialized forms, so the
|
|
shell and adapter agree when artifacts exist.
|
|
|
|
---
|
|
|
|
## 8. Verdict
|
|
|
|
All 4 prior blocking issues are resolved with verified code + passing contract tests. 132
|
|
change-relevant tests pass (0 failures). Documentation is synchronized. The one remaining item
|
|
(create_session.sh auth wiring) is explicitly out of the brief's scope and non-blocking. No
|
|
design-level rework is needed.
|
|
|
|
[VERDICT: PASS] |