multi-agent-mux/docs/design/reviews/review-codex-7770259b.md

# Design review — creator worktrees and a shared reviewer pool

Reviewed proposal `fe96db1`, `docs/design/worktree-creator-model.md`, through `git show design/worktree-model:...` and the named design worktree. That worktree's HEAD is `fe96db1`; the source checkout remains on `main` at `5611859`. Both working trees were clean. The document-only diff passes `git diff --check`.

**Verdict: NOT PASS as written. C is a viable direction, but the current-code recipe overstates control-plane compatibility and leaves review identity and lifecycle ownership underspecified. Revise those contracts before adopting the recipe. This does not require abandoning C.**

## Answers to the five questions in §6

### 1. Role separation — 보완 (agree with the direction; strengthen the execution contract)

Dedicated creator checkouts prevent an ordinary branch switch in the main checkout from changing a creator's HEAD, and allow independent implementation work. Sharing reviewer conversations is also reasonable. However, §2-C's sentence “읽기 전용” is not true of executing tests in the creator's checkout: tests, build systems and package installers can write tracked files, generated files, caches and dependencies. Multiple reviewers can interfere with each other, and with the creator, even without editing source deliberately.

Replace “리뷰어는 그 경로에서 ... 테스트를 실행한다” with an explicit choice:

- Freeze the creator at a committed head SHA for the whole review round, serialize tests that share writable outputs, and use isolated output/config directories; or
- Keep the reviewer conversation pool, but give each test execution a disposable checkout/worktree of that exact SHA and isolated outputs. Reviewer process/session location does not have to equal test execution location.

The example in §3 says `git diff <base>...HEAD`, whereas §2-C mentions a fixed head. Use an immutable `head_sha` consistently, record it in every verdict, and invalidate approval after a new creator commit. A diff against a SHA followed by tests against a changing working directory is not evidence for that SHA. Define how uncommitted changes are handled; preferably require a clean, committed review snapshot.

The statement “편집·커밋하는 유일한 에이전트” also needs qualification: the orchestrator commits, and the existing loop promotes reports into the source checkout (`lib_py/loop/runner.py:770`, `:780`). Separate product-code ownership, report artifacts and orchestration changes. A pre-commit branch-name check helps but is not an atomic exclusion mechanism. Require expected checkout/branch/head checks and one owner for each writable source tree.

### 2. Shared control plane through environment overrides — 반대 to the present blanket claim

The four variables are not consistently honored across all entry points. The narrow recipe running main-tree scripts from main may happen to align several defaults, but that is not a general split-root contract.

| Component | Code evidence and assessment |
|---|---|
| Common shell roots | `.agents/skills/lib.sh:26` honors `WORKSPACE_ROOT`; `:35` honors `AGENT_SESSIONS_YAML`. This supports a separate session registry for the command invoking the library. |
| Create | `multi-agent-mux-create/scripts/create_session.sh:219` spawns with `--workspace` as cwd; `:386` writes through the chosen YAML path, and `:428` records the actual pane cwd. Thus registration and cwd can differ. **But pane environment forwarding is not established by exporting variables to the create command.** `CMD_FULL` is an agent command (`:193`), and `lib.sh:667–680` generates pane `--env` flags only from assignments embedded in that command. No explicit forwarding of these four variables is present. A persistent herdr server/pane cannot be assumed to inherit each client's environment. Persist and explicitly pass the control-plane binding during create and resume. |
| Status | `multi-agent-mux-status/scripts/status.sh:59–77` uses each row's cwd for Claude transcript lookup, which is appropriate. Job lookup is different: `:29` derives `PROJECT_ROOT` from the script location; `:357–377` searches cwd/script-relative paths rather than `MAM_JOBS_DIR` or `DELEGATE_JOB_LOGS_DIR`. Calling the worktree copy against central state can therefore show an unknown/wrong job location. |
| Monitor/reconcile | `multi-agent-mux-monitor/scripts/reconcile.sh:117` anchors the monitor lock on `WORKSPACE_ROOT`, but `:239–243` computes the event registry from that root and ignores `MAM_JOBS_DIR`. Existing registered rows use their `pane.cwd` for UUID checks (`:727–757`); an external worktree cwd does **not by itself** mark an existing row as drift. However, discovery of a missing row explicitly rejects cwd outside the root at `:651–654`. Sibling worktrees in the example cannot be rediscovered through this path. |
| Monitor startup | `create_session.sh:526–527` passes the worktree to reconcile/watchdog startup. `lib_py/delegation/daemon.py:98–110` identifies the daemon by its worktree script, and `:70–76` puts its log/cwd there. This is not automatically one central daemon with a central lifetime, even if a central lock prevents duplicate active subscribers. Specify one central daemon owner and suppress worktree-local auto-starts. |
| Stop | `multi-agent-mux-stop/scripts/stop_session.sh:133–143` obtains the row cwd; `:151` honors the job-root override, and `:204–208` captures conversation identity using that cwd. These support the proposed split when the correct environment and server/row identity are supplied. |
| Resume | `multi-agent-mux-resume/scripts/resume_session.sh:42` requires `--workspace`; `:51–52` resolves identity using that argument and `:165` spawns there. It does not automatically substitute the row cwd. Resume must use the original worktree path and persisted control-plane binding. Claude discovery is cwd-keyed (`lib_py/agents/adapters/claude.py:117–118`) and verification checks cwd (`:78`). Deleting or renaming a worktree needs an explicit recovery policy. |
| Delegate job | Python defaults honor both job/log overrides (`lib_py/delegation/mqtt_common.py:152–186`). The shell wrapper instead defaults `REGISTRY_DIR` to relative `.mam/jobs` (`multi-agent-mux-delegate-job:71`, `:103`), changes cwd (`:153`), and passes that explicit registry argument (`:157`), overriding Python's default selection. |
| Loop | `lib_py/loop/runner.py:58–60` prefers `MAM_REAL_ROOT` and derives `.mam` from it. It honors `AGENT_SESSIONS_YAML` (`:195`), but job waiting/report lookup use `self.mam_dir/jobs` (`:348`, `:391`), not `MAM_JOBS_DIR`. The shell freeze path resets roots to its repository (`multi-agent-mux-loop/scripts/run_loop.sh:134–135`). |

Amend §2-C's “... 넘기면 ... 한 곳에 모인다” and §3's “현재 코드 기준” to describe **partial existing support plus required corrections**, not an already-safe environment recipe. Extend §5-3 beyond documentation to explicit propagation, persistence and a create/status/reconcile/stop/resume/delegate/loop compatibility matrix.

### 3. A/B/C costs and alternatives — 보완

C offers the best initial reuse of long-lived reviewer context while isolating writers. B leaves the principal creator checkout risk and gives little implementation parallelism. A provides stronger runtime separation at greater setup cost, but “완전 격리” is too strong: git worktrees still share repository metadata/refs, and shared `.venv`, caches, services and credentials remain shared resources. “현재 코드가 그대로 동작한다” needs qualification for worktree preparation and lifecycle behavior.

The costs are capacity estimates, not established laws: five fresh agents per issue are a policy choice, and token use depends on actual work, not just the number of live sessions. Amend §2-A accordingly. A better variant of C is **persistent reviewer conversations with disposable per-review execution directories**, rather than requiring a fresh reviewer conversation for filesystem isolation.

### 4. Missing/excess work and concurrency sequencing — 보완; start with one loop

§5-1 and §5-2 identify real blockers, but require broader definitions:

- Absolute paths must include brief, report output and every lifecycle command's `--registry-dir`, with proper shell quoting. Current wrapper lines `:178`, `:199`, `:352`, `:455` emit relative report/brief paths. Merely changing the brief path leaves the start command reading the wrong relative registry from a creator's worktree.
- Introduce separate **control root**, **source/worktree root**, **runtime-code root**, and a review identity `(loop_id, base_sha, head_sha)`. Preserve that binding in jobs/checkpoints and verify it on resume. `runner.py:399–408` collects diffs from the control workspace today, and `:564–565` chooses the base there. Report promotion, discovery and cleanup also need intentional root ownership; do not globally redirect `WORKSPACE_ROOT` to fix diff collection.
- §5-4 must account for the existing single-loop guard: `run_loop.sh:144`, `:188–200` rejects a second loop with the same marker before reviewer scheduling. This is an enforced boundary, not just the SKILL warning. Do not bypass it by assigning marker names alone.
- Checkpoints share one filename (`runner.py:60`); atomic replacement (`loop/checkpoint.py:18–30`) prevents torn files but not last-writer replacement, and successful completion removes that same file (`runner.py:801`). Namespacing, recovery ownership and cleanup must be designed together.
- SQLite/YAML corruption is not an automatic consequence of two local writers: `lib_py/atomic_yaml.py:59–86` already uses flock, busy timeout and `BEGIN IMMEDIATE`. Distinguish shared logical loop state and agent ownership from DB transaction safety, and test supported filesystem semantics instead of treating “NFS lock shadowing” as a complete explanation.
- Busy handling is not a queue: `runner.py:313–328` sleeps and retries once. A scheduler needs exclusive reviewer assignment/lease, fair bounded waiting, release on failure, cancellation and crash recovery. Additional reviewers need explicit assignment policy; `--all-reviewer` otherwise includes the whole running reviewer pool.

A safe first milestone is one guarded loop at a time, with separately delegated implementation jobs in distinct creator trees and queued review entry. The current loop itself does not become a multi-creator scheduler merely through a recipe. Defer multi-loop concurrency until its ownership contract and tests exist.

### 5. Order relative to #17 item 4 — 동의 with a staged rollout

Agree with first specifying the root/identity contract, then absorbing it into the shell-layer reduction. Do not bless the current recipe as safe while deferring its correctness gaps. First revise this design, then implement the minimal absolute-path/propagation/diff-root fixes and test one-loop operation; use #17 item 4 to consolidate those semantics rather than creating a second set of shell-specific routing rules. Multi-loop scheduling follows separately. This assessment uses the proposal's description of #17 item 4; it does not claim an independent review of the remote issue discussion.

## Additional failure paths to add to §5

1. **Worktree retirement:** stop/capture the creator first, retain its cwd/UUID and source SHA, finish/cancel owned jobs, then remove its execution tree through an explicit lifecycle. A stale YAML row is not evidence that a removed worktree can still resume. Preserve resumable worktrees or define recreation at the same path and revision.
2. **Test writes and environment isolation:** tests must not inherit production control-plane overrides. Give test registries, logs, monitor settings, ports and temporary directories isolated values. Shared `.venv` is only safe if dependency mutation is prohibited; dependencies differing by issue require separate environments. The proposed one-line preparation helper must validate preexisting paths/submodules rather than imply universal isolation.
3. **Central state is not a security boundary:** shared job credentials permit participation across these local processes. `mqtt_common.py:137`, `:351–352` currently use `python/mqtt/jobs/<job_id>`; the monitor also subscribes to a root fingerprint and the legacy wildcard (`reconcile.sh:288–294`). Correct §2-C/§1 so it does not imply every job topic is fingerprinted by the worktree/control root. Preserve one canonical control identity, per-job HMAC and dispatch/attempt checks. Use an explicit central `MAM_ENV_FILE` policy instead of copying credentials into every tree by default.

## Validation and adoption gate

This was a read-only design/code-trace review. No create/resume/stop/status/monitor command, test suite, checkout switch or worktree mutation was run. No runtime compatibility is claimed from static tracing. The named document and commit were verified; only this report and authorized lifecycle events were written.

Before adopting C, revise the cited sentences in §§2–5 and add acceptance cases for: sibling-worktree creation with a preexisting herdr server; explicit pane environment propagation; correct central brief/start/report routing; main remaining unchanged; fixed-SHA review under concurrent creator activity; row recovery and resume at the original cwd; isolated test state; and rejection of a second loop until a scheduler is implemented. These are targeted revisions within C, not a request to replace the design with A.

[VERDICT: NOT PASS]