Files

T

Godopu d7e19feaf5 refactor(installer): resolve architectural inconsistencies, pyyaml hard check, and migrate reports to tracked paths

2026-07-11 00:41:12 +09:00

9.2 KiB

Raw Blame History

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)

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.
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. ✅
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.

9.2 KiB Raw Blame History Unescape Escape