From af8a54c028570399cd5a02a7a74d02e34f65bb55 Mon Sep 17 00:00:00 2001 From: Godopu Date: Mon, 14 Sep 2026 05:00:02 +0000 Subject: [PATCH] fix(issue-21): review R1-R3 - no env as agent arguments on a reused pane, one jobs dir for a relative MAM_JOBS_DIR, stop keeps the audit-log root Review 9a1adb46 (planner-reviewer-codex-01): - R1 [P1] lib.sh new-session: a forwarded binding reached the agent as arguments. With env flags the idle-pane branch prefixed `env K=V ...` to the command that `herdr agent start ... --kind -- ` passes to the agent executable. An idle pane is now reused only when no environment has to be set; otherwise the pane is created by pane split / workspace create, which take --env. The dead `env` prefix code is removed (its send-text fallback would have typed the same line). - R2 [P2] runner: a relative MAM_JOBS_DIR is resolved once against the control root, the way the wrapper (cwd=workspace_root) reads it, and that absolute dir is used for delegation, wait, report lookup and cleanup. - R3 [P2] stop: the row's control_plane.delegate_job_logs_dir is restored (exported unless the caller set DELEGATE_JOB_LOGS_DIR), so the cancellation history lands in the row's audit-log root. Tests: T3-1 and T3-3 now assert that the binding arrives as --env on the pane-creation calls and that no env/K=V token follows `--` in agent start (fresh and reused pane); T3-4c (audit root on stop) and T2-15 (relative MAM_JOBS_DIR) are new. The design doc's compatibility table and a create comment no longer describe the removed idle-pane env path. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_017R3Eg7MEpSWnm1oB9ZPP6G --- .agents/skills/lib.sh | 13 +-- .agents/skills/lib_py/loop/runner.py | 4 +- .../scripts/create_session.sh | 3 +- .../scripts/stop_session.sh | 9 +- docs/design/worktree-creator-model.md | 4 +- tests/test_issue21_worktree_prereqs.py | 98 +++++++++++++++++-- 6 files changed, 113 insertions(+), 18 deletions(-) diff --git a/.agents/skills/lib.sh b/.agents/skills/lib.sh index c86c5b8..d19040f 100644 --- a/.agents/skills/lib.sh +++ b/.agents/skills/lib.sh @@ -775,14 +775,15 @@ except Exception: pass " 2>/dev/null || echo "") - if [ -n "$idle_pane" ]; then + # An idle pane is reused only when no environment has to be set: the + # pane's shell is already running, and `agent start ... -- ` hands + # every token after -- to the agent executable (--kind), so an + # `env K=V` prefix would reach the agent as arguments, not as its + # environment (Issue #21 R1). With env flags, fall through to + # pane split / workspace create, which take --env. + if [ -n "$idle_pane" ] && [ -z "$env_flags" ]; then existing_ws="$cand_ws" target_pane="$idle_pane" - if [ -n "$env_flags" ] && [ -n "$final_cmd" ]; then - idle_env="${env_flags//--env /}" - idle_env="${idle_env:+$idle_env }HERDR_WORKSPACE_ID=$existing_ws" - final_cmd="env $idle_env $final_cmd" - fi break fi diff --git a/.agents/skills/lib_py/loop/runner.py b/.agents/skills/lib_py/loop/runner.py index 4ae8434..94e0b28 100644 --- a/.agents/skills/lib_py/loop/runner.py +++ b/.agents/skills/lib_py/loop/runner.py @@ -64,7 +64,9 @@ class LoopRunner: # diff, base and head come from; everything else - wrapper, registry, # checkpoint, loop guard, promotion, cleanup - stays on the control # root (workspace_root). - self.jobs_dir = Path(os.environ.get("MAM_JOBS_DIR") or self.mam_dir / "jobs") + # A relative MAM_JOBS_DIR is taken against the control root, as the + # wrapper (run with cwd=workspace_root) takes it. + self.jobs_dir = (self.workspace_root / Path(os.environ.get("MAM_JOBS_DIR") or self.mam_dir / "jobs")).resolve() self.worktree_root: Path = self.workspace_root self.worktree_given = False self.base_ref: Optional[str] = None diff --git a/.agents/skills/multi-agent-mux-create/scripts/create_session.sh b/.agents/skills/multi-agent-mux-create/scripts/create_session.sh index 28c7047..de2e809 100755 --- a/.agents/skills/multi-agent-mux-create/scripts/create_session.sh +++ b/.agents/skills/multi-agent-mux-create/scripts/create_session.sh @@ -205,7 +205,8 @@ fi # Issue #21 (item 3): the control-plane binding. Persisted on the row always; # forwarded to the pane (leading K=V tokens that lib.sh's new-session turns # into --env) only when it differs from what --workspace alone implies. -# lib.sh splits those flags unquoted (and the idle-pane path evals them), so a +# lib.sh splits those flags unquoted (and, with env flags, never reuses an +# idle pane - `agent start --` would pass them to the agent as arguments), so a # binding with whitespace that would have to be forwarded is refused. CP_JSON="$(mam_control_plane_json "$WORKSPACE" 2>/dev/null || true)" CP_PREFIX="" diff --git a/.agents/skills/multi-agent-mux-stop/scripts/stop_session.sh b/.agents/skills/multi-agent-mux-stop/scripts/stop_session.sh index 2ef251b..b237034 100755 --- a/.agents/skills/multi-agent-mux-stop/scripts/stop_session.sh +++ b/.agents/skills/multi-agent-mux-stop/scripts/stop_session.sh @@ -133,7 +133,8 @@ for s in d.get('herdr_sessions', []): cwd = (s.get('pane') or {}).get('cwd', '') jid = s.get('delegate_job_id', '') or '' cp = s.get('control_plane') or {} - print(json.dumps({'cwd': cwd, 'job_id': jid, 'mam_jobs_dir': cp.get('mam_jobs_dir', '') or ''})) + print(json.dumps({'cwd': cwd, 'job_id': jid, 'mam_jobs_dir': cp.get('mam_jobs_dir', '') or '', + 'delegate_job_logs_dir': cp.get('delegate_job_logs_dir', '') or ''})) sys.exit(0) sys.exit(7) ") || { @@ -150,6 +151,12 @@ ROW_JOBS_DIR=$(printf '%s' "$MAPPED_DATA" | python3 -c 'import sys,json; print(j if [ -z "${MAM_JOBS_DIR:-}" ] && [ -n "$ROW_JOBS_DIR" ]; then export MAM_JOBS_DIR="$ROW_JOBS_DIR" fi +# Same for the audit log (R3): the cancellation history belongs next to the +# registry the row names, not under $WORKSPACE_ROOT/.mam/delegate_job_logs. +ROW_LOGS_DIR=$(printf '%s' "$MAPPED_DATA" | python3 -c 'import sys,json; print(json.load(sys.stdin).get("delegate_job_logs_dir",""))') +if [ -z "${DELEGATE_JOB_LOGS_DIR:-}" ] && [ -n "$ROW_LOGS_DIR" ]; then + export DELEGATE_JOB_LOGS_DIR="$ROW_LOGS_DIR" +fi DELEGATE_JOB_TOKEN="" DELEGATE_JOB_ATTEMPT="" if [ -n "$DELEGATE_JOB_ID" ]; then diff --git a/docs/design/worktree-creator-model.md b/docs/design/worktree-creator-model.md index ab123ff..4425536 100644 --- a/docs/design/worktree-creator-model.md +++ b/docs/design/worktree-creator-model.md @@ -116,9 +116,9 @@ bash "$MAIN/.agents/skills/multi-agent-mux-resume/scripts/resume_session.sh" \ | 스킬 | 제어 평면 바인딩 | 워크트리 cwd 행 | 1단계 이후 남은 제한 (항목) | |---|---|---|---| -| create | env 에서 결정 → 행 `control_plane` 에 항상 영속. `--workspace` 기본값과 다르면 pane 에 전파(새 pane 은 `--env`, 재사용 idle pane 은 `env K=V`). 전파가 필요한데 경로에 공백이 있으면 exit 2. `~/.local/bin` wrapper 경로는 기록만 하고 경고 | pane.cwd = `--workspace` | create 가 `$WORKSPACE` 사본으로 reconcile/watchdog 기동, `MAM_NO_SUBSCRIBER_DAEMON` 미기본 (4) | +| create | env 에서 결정 → 행 `control_plane` 에 항상 영속. `--workspace` 기본값과 다르면 새 pane 의 생성 호출(`pane split`/`workspace create`)에 `--env` 로 전파한다. 전파가 필요하면 idle pane 을 재사용하지 않는다(`agent start -- ` 는 `--` 뒤를 에이전트 인자로 넘기므로 `env K=V` 를 쓸 수 없다). 전파가 필요한데 경로에 공백이 있으면 exit 2. `~/.local/bin` wrapper 경로는 기록만 하고 경고 | pane.cwd = `--workspace` | create 가 `$WORKSPACE` 사본으로 reconcile/watchdog 기동, `MAM_NO_SUBSCRIBER_DAEMON` 미기본 (4) | | resume | 행 바인딩 우선(호출자 env 와 다르면 stderr 경고) 재전파. 바인딩 없는 기존 행은 호출자 바인딩으로 채움 | `--workspace` 는 원래 워크트리 경로여야 함 | 워크트리 삭제 시 재개 불가, 종료 헬퍼 (4) | -| stop | `MAM_JOBS_DIR` > 행의 `mam_jobs_dir` > `$WORKSPACE_ROOT/.mam/jobs` 순으로 잡 조회·발행 | 행 cwd 로 uuid 캡처·purge | 해체 순서 헬퍼 (4) | +| stop | `MAM_JOBS_DIR` > 행의 `mam_jobs_dir` > `$WORKSPACE_ROOT/.mam/jobs` 순으로 잡 조회·발행. 감사 로그도 `DELEGATE_JOB_LOGS_DIR` > 행의 `delegate_job_logs_dir` 순 | 행 cwd 로 uuid 캡처·purge | 해체 순서 헬퍼 (4) | | status | `MAM_JOBS_DIR`·행 `mam_jobs_dir`·감사 로그(`DELEGATE_JOB_LOGS_DIR`·행 `delegate_job_logs_dir`)·기존 후보 순 조회 | 행 cwd 로 대화 키 | — | | monitor/reconcile | `MAM_JOBS_DIR` 우선(정적 가드로만 검증) | 기존 행은 행 cwd 로 검증 | 루트 밖 cwd 의 미등록 세션은 자동 등록 안 됨 → create 로만 등록 (4) | | delegate-job | `--registry-dir` > `MAM_JOBS_DIR` > `.mam/jobs`(`--workdir` 기준), 모두 절대화. 발행 명령이 감사 로그 루트(`DELEGATE_JOB_LOGS_DIR`)를 고정 | 문구·명령·리포트 경로 절대 | 작은따옴표가 든 경로는 지원 범위 밖 | diff --git a/tests/test_issue21_worktree_prereqs.py b/tests/test_issue21_worktree_prereqs.py index c1f8345..03e4f45 100644 --- a/tests/test_issue21_worktree_prereqs.py +++ b/tests/test_issue21_worktree_prereqs.py @@ -511,6 +511,22 @@ def _env_flags(mock_state_file, since=0) -> list: return [c[i + 1] for c in calls for i, t in enumerate(c[:-1]) if t == "--env"] +def _agent_args(mock_state_file, since=0) -> list: + """The tokens after `--` of every `herdr agent start` call: herdr passes + them to the canonical agent executable (--kind), never to a shell.""" + calls = json.loads(Path(mock_state_file).read_text()).get("calls", [])[since:] + starts = [c for c in calls if any(c[i:i + 2] == ["agent", "start"] for i in range(len(c) - 1))] + return [c[c.index("--") + 1:] if "--" in c else [] for c in starts] + + +def _assert_no_env_in_agent_args(mock_state_file, since=0) -> None: + arg_lists = _agent_args(mock_state_file, since) + assert arg_lists, "no agent start was recorded" + for args in arg_lists: + assert "env" not in args, args + assert not [a for a in args if a.startswith(tuple(f"{k}=" for k in CP_ENV))], args + + def _ncalls(mock_state_file) -> int: return len(json.loads(Path(mock_state_file).read_text()).get("calls", [])) @@ -563,6 +579,7 @@ def test_t3_1_create_persists_and_forwards_a_foreign_binding(mam_sandbox, mock_h assert f"{k}={v}" in flags, flags assert "WORKSPACE_ROOT=" not in row["pane"]["cmd_full"] assert "WORKSPACE_ROOT=" not in row["start_command"] + _assert_no_env_in_agent_args(mock_herdr) def test_t3_2_default_binding_is_recorded_not_forwarded(mam_sandbox, mock_herdr, mock_agents): @@ -602,14 +619,14 @@ def test_t3_3_4_stop_keeps_the_binding_and_resume_reforwards_it(mam_sandbox, moc "--workspace", str(wt), "--agent", "claude", "--session", name], capture_output=True, text=True, env=env, timeout=180) assert res.returncode == 0, res.stdout + res.stderr - # A fresh pane gets `--env K=V` flags; a reused idle pane (the stopped - # session's) gets the same K=V tokens as `env K=V ... ` (lib.sh D9). - state = json.loads(Path(mock_herdr).read_text()) - calls = state.get("calls", [])[since:] - args = [a for c in calls for a in c if isinstance(a, str)] + _sent_texts(state) - forwarded = {tok for a in args for tok in a.split()} + # The stopped session left an idle pane behind. A binding that must be + # forwarded is never put in front of `agent start --` (herdr passes those + # tokens to the agent as arguments, review R1): resume takes a new pane, + # whose creation call carries the binding as --env. + flags = _env_flags(mock_herdr, since) for k, v in zip(CP_ENV, _binding(tmp).values()): - assert f"{k}={v}" in forwarded, (res.stderr[-1500:], json.dumps(calls)[:1500]) + assert f"{k}={v}" in flags, (res.stderr[-1500:], flags) + _assert_no_env_in_agent_args(mock_herdr, since) (row,) = _rows(tmp) assert row["control_plane"] == _binding(tmp) @@ -690,3 +707,70 @@ d['herdr_sessions'] = [{{ assert res.returncode == 0, res.stdout + res.stderr (row,) = _rows(tmp) assert row["control_plane"] == _binding(tmp) + + +def test_t3_4c_stop_keeps_the_audit_trail_in_the_rows_log_root(mam_sandbox, mock_herdr, mock_agents, monkeypatch): + """Review R3: with no MAM_JOBS_DIR / DELEGATE_JOB_LOGS_DIR in the caller, + stop cancels the job in the row's registry and writes its history to the + row's audit-log root - not to $WORKSPACE_ROOT/.mam/delegate_job_logs.""" + from lib_py.delegation import registry + tmp = mam_sandbox + jobs, logs = tmp / "custom-jobs", tmp / "custom-logs" + # the job was registered under that binding (its `registered` audit entry lives there) + monkeypatch.setenv("DELEGATE_JOB_LOGS_DIR", str(logs)) + jid = registry.register_job("t3-4c", registry_dir=str(jobs), agent_session="herdr:logs-creator-claude") + binding = {**_binding(tmp), "mam_jobs_dir": str(jobs), "delegate_job_logs_dir": str(logs)} + _mutate(tmp, f""" +d['herdr_sessions'] = [{{ + 'name': 'logs-creator-claude', 'status': 'running', 'role': 'creator', + 'delegate_job_id': {jid!r}, 'pane': {{'cwd': {str(tmp)!r}, 'cmd': 'claude'}}, + 'control_plane': {binding!r}, +}}] +""") + env = {k: v for k, v in os.environ.items() if k not in ("MAM_JOBS_DIR", "DELEGATE_JOB_LOGS_DIR")} + res = subprocess.run(["bash", _skill(tmp, "multi-agent-mux-stop", "scripts", "stop_session.sh"), + "--session", "logs-creator-claude", "--agent", "claude"], + capture_output=True, text=True, env=env, timeout=120) + assert res.returncode == 0, res.stdout + res.stderr + assert json.loads((jobs / f"{jid}.json").read_text())["status"] == "cancelled" + assert not (tmp / ".mam" / "delegate_job_logs" / jid).exists(), "history misrouted to the default log root" + audit = [json.loads(l) for l in (logs / jid / "events.ndjson").read_text().splitlines() if l.strip()] + assert {"progress", "cancelled"} <= {e.get("source_event") for e in audit}, audit + assert any(e.get("event") == "status_changed" for e in audit), audit + + +def test_t2_15_relative_mam_jobs_dir_means_the_same_dir_to_runner_and_wrapper(tmp_path, monkeypatch): + """Review R2: the wrapper runs with cwd=workspace_root and absolutises a + relative registry there; the runner must look in that same directory.""" + from lib_py.loop import runner as lr + ctl, caller = tmp_path / "control", tmp_path / "caller" + ctl.mkdir() + caller.mkdir() + monkeypatch.chdir(caller) + monkeypatch.setenv("WORKSPACE_ROOT", str(ctl)) + monkeypatch.delenv("MAM_REAL_ROOT", raising=False) + monkeypatch.setenv("MAM_JOBS_DIR", ".mam/jobs") + r = _runner() + jobs = Path(os.path.realpath(ctl)) / ".mam" / "jobs" + assert r.jobs_dir == jobs and r.jobs_dir.is_absolute() + report = jobs / "j21" / "reviewer-reports" / "report-final.md" + report.parent.mkdir(parents=True) + report.write_text("[VERDICT: PASS]\n") + (jobs / "j21.json").write_text(json.dumps({"status": "completed"})) + assert r.find_report("j21") == report + assert r.wait_for_job("j21", max_wait=2) is True + seen = [] + + def fake_run(cmd, **kw): + seen.append(cmd) + + class P: + returncode = 0 + stdout = "registered job: j22\n" + stderr = "" + return P() + + monkeypatch.setattr(lr.subprocess, "run", fake_run) + r.delegate_job("creator-claude-01", "claude", "Worker", "go") + assert seen[0][seen[0].index("--registry-dir") + 1] == str(jobs), "the wrapper is told the same absolute dir" + assert not (caller / ".mam").exists()