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]