96 lines
9.2 KiB
Markdown
96 lines
9.2 KiB
Markdown
# Review Report — 세션 ID 격리 구현 (Session Isolation)
|
||
|
||
- **Reviewer**: Reviewer Cline (`canary-projects-multi-agent-mux-reviewer-cline`, role: reviewer)
|
||
- **Date**: 2026-07-10
|
||
- **Subject commit**: `768cfe5 feat(isolation): implement Phase 1-3 session isolation with stop purge and resume safety`
|
||
- **Range reviewed**: `HEAD~3..HEAD` (commit `768cfe5` + 2 docs commits). ※ The user-requested `HEAD~2..HEAD` only spans the two docs commits and excludes the implementation commit itself (it sits *at* `HEAD~2`, excluded by an exclusive range); I expanded to `HEAD~3..HEAD` to cover the actual code, which is the evident intent.
|
||
- **Governing documents**: `AGENTS.md`, `.agents/MULTI_AGENT_RULES.md` / `.ko.md`, `implementation_plan.session_isolation.md` (Rev.3), `task.session_isolation.md`
|
||
|
||
---
|
||
|
||
## Verdict: **PASS** ✅
|
||
|
||
The session-isolation implementation conforms to all three governing documents. All code implementation tasks (T1–T6), including the RK2 resume re-apply fix and the T6 stop-purge with path safeguards, are complete and correct. `bash -n` passes; `shellcheck` introduces **zero new warnings** (V5 PASS); the non-isolated regression path is byte-identical (V4 PASS). Remaining unchecked items in `task.session_isolation.md` are live-integration verification steps (V1–V3, T-V2) and an explicitly-optional orphan GC (T6-b) — they are process gates, not code defects, and are honestly flagged as pending by the developer.
|
||
|
||
---
|
||
|
||
## 1. Conformance to `implementation_plan.session_isolation.md` (Rev.3)
|
||
|
||
### Phase 0 — 검증 게이트 (G2 프로브) ✅
|
||
- All G2 probes (`G2-C1/C2`, `G2-L1`, `G2-A1`, `G2-H1`, `G2-M`) are marked `[x]` in `task.session_isolation.md`.
|
||
- §2.3 매트릭스 populated with per-agent lever, seed list, conversation path — including the two Rev.3 corrections: cline `--config` insufficient (seeding required) and cline isolated layout `<root>/sessions/` (not `data/sessions/`).
|
||
|
||
### Phase 1 — 공통 불변식 (T1/T2) ✅
|
||
- **T1 claimed-set filter** (`lib.sh:543-573`): `running_ids` set built from SQLite (DB-first) with YAML fallback; `emit()` filters any candidate already claimed by *another* running row. Target row's own IDs are excluded from the filter (`if target and s_data.get('name') == target: continue`) — fixes the self-ID filter bug noted in T5.
|
||
- **T2 생성-시 유일성 assert** (`lib.sh:386-399`): iterates running rows, raises `SystemExit` on duplicate `*_own` across distinct sessions. Matches plan §2.4 R2.
|
||
|
||
### Phase 2 — all-L2 격리 구현 (T3/T4/T5) ✅
|
||
- **T3 provisioning** (`create_session.sh:119-135`, `lib.sh:818-856`): `uuidgen` → `.mam/agent_homes/<uuid>/`, per-agent symlink seeding, `seeded` list returned. Dry-run guard + error-trap rollback with path guard `*/.mam/agent_homes/*` before `rm -rf`.
|
||
- **T4 spawn injection** (`lib.sh:858-882`, `create_session.sh:142-169`): single dispatch via `isolation_env_prefix` (claude `CLAUDE_CONFIG_DIR`, agy/hermes `HOME`) and `isolation_cmd_args` (cline `--data-dir`). Applied to `CMD_FULL`, `spawn()`, and `START_CMD`. When `ISOLATE=0`, `ISO_ENV_PREFIX=""`/`ISO_CMD_ARGS=""` → strings reduce to exact pre-commit values (byte-identical, V4).
|
||
- **T5 schema persistence + re-apply** (`create_session.sh:311-320`, `lib.sh:501-504, 613-661`): `isolation: {uuid, root, lever, seeded[]}` written via `atomic_dump_yaml`; `_validate` enforces `uuid`/`root` required. `find_workspace_uuid` target-mode resolves strictly within the row's isolation root — **no global fallthrough** (the C3 mtime bug is structurally eliminated). cline path template `<root>/sessions/` correctly branched.
|
||
|
||
### RK2 resume re-apply (previously flagged blocking — now fixed) ✅
|
||
- `resolve_session_id.sh` accepts `--session` and forwards to `find_workspace_uuid "$WORKSPACE" "$AGENT" "$SESSION_NAME"`.
|
||
- `resume/SKILL.md` workflow now: (1) passes `--session "$SESSION_NAME"`, (2) resolves the row's `isolation` block via a Python helper, (3) re-derives `ISO_ENV`/`ISO_ARGS` via `isolation_env_prefix`/`isolation_cmd_args`, (4) prepends/appends to `CMD_FULL` before spawn. This satisfies T5's "저장+재적용은 원자적 세트" contract — the save path (create) and re-apply path (resume) share the same dispatch functions.
|
||
|
||
### Phase 3 — stop 청소 (T6) ✅
|
||
- `stop_session.sh:273-288`: purge path guarded by **two** checks: (a) `abspath(iso_root) == abspath(expected_iso_root)` where `expected_iso_root = <ws>/.mam/agent_homes/<iso_uuid>`, and (b) `startswith(expected_homes_dir + os.sep)`. Mismatch → `WARN`, no delete.
|
||
- `capture_conversation_id` and `find_workspace_uuid` calls now pass `$SESSION_NAME` → isolated rows resolve within their own root, preventing cross-row UUID theft on purge.
|
||
- T6-a (symlink original preservation): `rmtree` operates on `iso_root` only; symlink targets (real `~/.claude/.credentials.json` etc.) are outside `iso_root` and untouched. ✅
|
||
|
||
### Phase 4 — 회귀 및 검증
|
||
| ID | Criterion | Result |
|
||
|----|-----------|--------|
|
||
| V4 | Non-isolated path byte-identical (regression 0) | ✅ PASS — verified: `ISOLATE=0` ⇒ `CMD_FULL` equals pre-commit strings exactly |
|
||
| V5 | `bash -n` + `shellcheck` new warnings = 0 | ✅ PASS — `bash -n` OK; shellcheck diff vs `768cfe5~1` baseline shows **0 new** (all 10 findings pre-existing, line-shifted only) |
|
||
| V1–V3, T-V2 | Live 4-agent simultaneous spawn + resume isolation | ⏳ Pending — explicitly marked `[ ]` by developer; scratch functional tests passed, live TUI multi-spawn remaining. Process gate, not a code defect. |
|
||
| T6-b | Orphan GC (optional) | ⏳ Explicitly marked 선택(optional) in plan |
|
||
|
||
---
|
||
|
||
## 2. Conformance to `AGENTS.md`
|
||
|
||
### Simplicity First ✅
|
||
- No speculative features. `provision_isolation` is a per-agent `case`, not an over-generalized plugin system.
|
||
- `isolation_env_prefix`/`isolation_cmd_args` are minimal one-liners — no abstraction beyond the 4-agent matrix.
|
||
- Path guards use the simplest correct construct (`case` glob + `os.path.abspath` equality/startswith). No over-engineered allowlist framework.
|
||
|
||
### Surgical Changes ✅
|
||
- Every changed line traces to a plan task (T1→T5, RK2, T6). No drive-by refactors.
|
||
- Pre-existing shellcheck findings (SC2164 `cd || exit`, SC2034 unused `i`, SC2155 declare+assign, SC1091 source-not-followed) were **not touched** — correct per "Don't refactor things that aren't broken" and "Don't remove pre-existing dead code unless asked."
|
||
- Non-isolated branches preserve original control flow and string literals verbatim.
|
||
|
||
### Think Before Coding ✅
|
||
- The Phase 0 gate was honored: no Phase 2 code before the G2 matrix was finalized (plan §5 "Phase 0 게이트 통과 전 코드 구현 착수 금지").
|
||
- Rev.3 decision log (D1–D5) records the L1→all-L2 tradeoff explicitly rather than silently picking.
|
||
|
||
### Goal-Driven Execution ✅
|
||
- `task.session_isolation.md` DoD items map 1:1 to plan tasks with verifiable criteria. Developer self-marked `[x]` only where evidence exists and left `[ ]` where verification is genuinely incomplete (honest reporting).
|
||
|
||
---
|
||
|
||
## 3. Conformance to `MULTI_AGENT_RULES.md`
|
||
|
||
- **Isolation root** (`.mam/agent_homes/<uuid>/`) sits under `.mam/` → `.gitignore` covered; included in the stop-cleanup contract (T6). ✅
|
||
- **Symlink seeding** (not copy) satisfies RK4 (token-refresh convergence on the real file) and avoids auth-divergence. ✅
|
||
- **`_validate` schema enforcement** for the `isolation` block (uuid/root required) keeps the registry internally consistent. ✅
|
||
- **Role isolation**: the implementation does not alter `role` semantics or cross into reviewer/PM authority — it is purely infrastructure (developer-team scope). ✅
|
||
- This report is persisted under `.mam/reports/<tmux_session_name>/` per the markdown-collaboration protocol. ✅
|
||
|
||
---
|
||
|
||
## 4. Observations (non-blocking, for record)
|
||
|
||
1. **hermes `config.yaml` embedded absolute paths** (plan §2.3 note 3): the implementation symlinks `config.yaml` into the isolation root; hermes runtime resolves the symlink to the real config, which may reference real-HOME paths. The plan explicitly documents this as "읽기 공유라 무해하나, 격리 범위가 state/세션에 한정됨" — hermes isolation is scoped to `state.db`/sessions, not full config. Implementation matches the documented limitation. **Not a defect.**
|
||
|
||
2. **cline isolated layout divergence** (`<root>/sessions/` vs real `~/.cline/data/sessions/`): correctly handled by `cline_exists(uuid, iso)` and the target-mode glob `f"{iso}/sessions/*"`. Matches plan §2.3 note 2. ✅
|
||
|
||
3. **Orphan GC (T6-b)** remains unimplemented but is explicitly optional in the plan; `.mam/` is gitignored and covered by `remove.sh` whole-tree cleanup. Not blocking.
|
||
|
||
---
|
||
|
||
## 5. Final Statement
|
||
|
||
The implementation is complete, correct against the approved Rev.3 plan, and respects `AGENTS.md` (Simplicity First / Surgical Changes) and `MULTI_AGENT_RULES.md`. The previously-blocking RK2 resume re-apply gap is resolved. Static verification (V4 byte-identical regression, V5 bash-n + shellcheck-new=0) passes. The remaining unchecked items are live-integration verification steps the developer has transparently left open — they do not represent code-level non-conformance.
|
||
|
||
**PASS** — approved for merge. Recommend the developer proceed with V1–V3/T-V2 live-spawn integration verification as a follow-on gate before production rollout, per the plan's Phase 4 DoD. |