From ed293dcef4c346e0050a31cb46812bcdcf1d67cf Mon Sep 17 00:00:00 2001 From: Godopu Date: Fri, 25 Sep 2026 01:25:48 +0000 Subject: [PATCH] fix(issue-34): a pane caller reaches an agent of another workspace on its own server; an unseen target is not recorded as busy - mam_herdr: for name lookups (agent / has-session / kill-session) a HERDR_WORKSPACE_ID that is the caller pane's own workspace (it matches the HERDR_PANE_ID prefix) is dropped. Live agent names are unique per server (herdr --skill), so this cannot address a same-named agent of another workspace; an id the caller chose, or one set outside a pane, still scopes (H-18/H-21/H-23). The generated shim is unchanged (S11). - wrapper: a preflight that cannot see the target closes the job cancelled with detail target_not_found instead of session_busy; the loop's sub-job table counts it as NOT-DISPATCHED. - tests: T10-T12 with the measured pane env (same-server socket, caller w6, target w2); T8 follows the sub-job-armed guard. Co-Authored-By: Claude Opus 5.5 --- .agents/skills/lib.sh | 30 +++++ .../multi-agent-mux-delegate-job/SKILL.md | 2 +- .../multi-agent-mux-delegate-job | 5 +- tests/test_issue34_job_tree.py | 106 ++++++++++++++++-- 4 files changed, 131 insertions(+), 12 deletions(-) diff --git a/.agents/skills/lib.sh b/.agents/skills/lib.sh index a150395..6c4ca5c 100644 --- a/.agents/skills/lib.sh +++ b/.agents/skills/lib.sh @@ -1446,6 +1446,31 @@ _herdr_call_is_cross_server() { return 1 } +# _herdr_scope_is_pane_inherited +# +# ISSUE-34: true for a call that names an agent (agent / has-session / +# kill-session) from inside a pane whose own workspace is the exported +# HERDR_WORKSPACE_ID. A pane exports it together with HERDR_PANE_ID +# ":"; when the two agree the id says where the caller lives, +# not where to look - a worktree creator in its own workspace must reach a +# reviewer in mam-agent on the same server. Live agent names are unique per +# server (herdr --skill: "Names must ... be unique among live agents", "IDs and +# live agent names are scoped to one server"), so a name lookup without that +# filter cannot pick a same-named agent of another workspace: the ISSUE-3 / +# F-2b hazard exists only across servers (_herdr_call_is_cross_server). An id +# that disagrees with the pane, or one set outside any pane, is still a +# deliberate scope (H-18/H-21/H-23); new-session and pane calls keep it, so a +# pane caller still creates panes in its own workspace. +_herdr_scope_is_pane_inherited() { + while [ "${1:-}" = "-L" ]; do shift 2 || return 1; done + case "${1:-}" in + agent|has-session|kill-session) ;; + *) return 1 ;; + esac + local pane="${HERDR_PANE_ID:-}" + [ -n "${HERDR_WORKSPACE_ID:-}" ] && [ -n "$pane" ] && [ "${pane%%:*}" = "$HERDR_WORKSPACE_ID" ] +} + mam_herdr() { _init_herdr_isolation # Built as an argv array and decided inside `if`: this runs under @@ -1457,6 +1482,11 @@ mam_herdr() { if [ "${MAM_SHIM_DEBUG:-0}" = "1" ]; then echo "mam_herdr: dropping caller pane scope ids (caller server '$(_herdr_caller_server)' != target '${HERDR_SESSION_NAME:-}')" >&2 fi + elif _herdr_scope_is_pane_inherited "$@"; then + _scope_strip=(env -u HERDR_WORKSPACE_ID) + if [ "${MAM_SHIM_DEBUG:-0}" = "1" ]; then + echo "mam_herdr: '$1' looks up by name; the caller pane's own workspace ${HERDR_WORKSPACE_ID} is not a scope" >&2 + fi fi ${_scope_strip[@]+"${_scope_strip[@]}"} "$WORKSPACE_ROOT/.mam/shim/herdr" "$@" } diff --git a/.agents/skills/multi-agent-mux-delegate-job/SKILL.md b/.agents/skills/multi-agent-mux-delegate-job/SKILL.md index c37fb23..44b3c85 100644 --- a/.agents/skills/multi-agent-mux-delegate-job/SKILL.md +++ b/.agents/skills/multi-agent-mux-delegate-job/SKILL.md @@ -170,7 +170,7 @@ Job lifecycle execution events are persistently mirrored to an append-only log u ## Best Practices and Pitfalls - **Subscribe-Before-Publish**: The subscriber must be running before the agent starts publishing. The `submit` command handles this automatically by launching the subscriber in the background first. -- **Sub-delegation carries `--parent-job` (Issue #34)**: an agent that hands part of its job to another agent must pass `--parent-job ` (the injected instructions and `brief.md` say so). Without it the loop does not wait for the sub-job; reviewers see it only as an unattributed open job. From a git worktree the worktree's copy of this wrapper runs; the pane's `MAM_JOBS_DIR` keeps the registry the same one. +- **Sub-delegation carries `--parent-job` (Issue #34)**: an agent that hands part of its job to another agent must pass `--parent-job ` (the injected instructions and `brief.md` say so). Without it the loop does not wait for the sub-job; reviewers see it only as an unattributed open job. From a git worktree the worktree's copy of this wrapper runs; the pane's `MAM_JOBS_DIR` keeps the registry the same one. The target may live in another herdr workspace of the same server: inside a pane, the pane's own `HERDR_WORKSPACE_ID` (the one matching `HERDR_PANE_ID`) is not used as a scope for name lookups, since live agent names are unique per server; a `HERDR_WORKSPACE_ID` you set yourself still scopes. A target the preflight cannot see is cancelled with detail `target_not_found` (exit 5), not `session_busy`. - **Fresh job_id Propagation**: Make sure the worker agent receives the correct `JOB_ID` generated for the current run, rather than reusing stale IDs from previous sessions. - **Brief delivery via file path**: For long or complex prompts, write the instructions to a file (e.g. `/tmp/task-brief.md`) and pass a short prompt pointing to the file path to prevent terminal buffer overflows. - **Prompts injected into a live agent session MUST be English, ASCII-only, and short** — this is exactly what `--prompt`/the `instructions` string sent to `run_agent()` end up as. Any Korean (or other non-ASCII) content the task needs to convey must go in a markdown brief file (e.g. `.mam/jobs//brief.md`, written in Korean is fine) that the injected prompt merely tells the agent to read. Two independent bugs in `send_keys_safe`'s paste-verification (in `lib.sh`) made this matter in practice: (a) its marker was taken with a byte-based `tail -c 24`, which can slice a multi-byte UTF-8 (e.g. Korean) character in half; (b) the rendered pane soft-wraps long lines at the terminal width, which can split the marker across two visual lines. Both are now fixed at the source (character-safe truncation + newline-stripped matching before comparison), but keeping injected prompts short/English/file-referencing is still the cheapest way to avoid ever exercising this edge case at all — it's also simply what `submit`'s own default instruction template already does (see `Core Commands` above). diff --git a/.agents/skills/multi-agent-mux-delegate-job/multi-agent-mux-delegate-job b/.agents/skills/multi-agent-mux-delegate-job/multi-agent-mux-delegate-job index c3ff26c..e3a3a5f 100755 --- a/.agents/skills/multi-agent-mux-delegate-job/multi-agent-mux-delegate-job +++ b/.agents/skills/multi-agent-mux-delegate-job/multi-agent-mux-delegate-job @@ -892,6 +892,8 @@ finalize_unfinished() { if [ "$phase" = "preflight" ] || [ "$phase" = "idle_wait" ]; then close_req="cancelled" detail="session_busy" + # Issue #34: a target the preflight could not see was never busy + [ "$phase" = "preflight" ] && [ -n "${PREFLIGHT_REASON:-}" ] && detail="$PREFLIGHT_REASON" fi local close_args=(--job "$job_id" --request "$close_req") @@ -921,7 +923,7 @@ finalize_unfinished() { local disp_arg=() [ -n "${_disp_tok:-}" ] && disp_arg=(--expect-dispatch-token "$_disp_tok") if [ "$close_req" = "cancelled" ]; then - "$PY" "$SCRIPT_DIR/scripts/publish_event.py" --registry-dir "$REGISTRY_DIR" --job "$job_id" --event cancelled --detail "session_busy" ${disp_arg[@]+"${disp_arg[@]}"} 2>/dev/null || true + "$PY" "$SCRIPT_DIR/scripts/publish_event.py" --registry-dir "$REGISTRY_DIR" --job "$job_id" --event cancelled --detail "$detail" ${disp_arg[@]+"${disp_arg[@]}"} 2>/dev/null || true elif [ "$sub_rc" -ne 1 ] || [ "$phase" = "inject" ]; then local err_detail="deadline_timeout" [ "$phase" = "inject" ] && err_detail="agent_blocked" @@ -947,6 +949,7 @@ preflight_agent() { export HERDR_SESSION_NAME="$(resolve_herdr_session "$sess" "$WORKDIR")" if ! herdr has-session -t "$sess" 2>/dev/null; then + PREFLIGHT_REASON="target_not_found" # Issue #34: not a busy session echo "ERROR: 에이전트 세션 '$sess'이 존재하지 않습니다. 작업을 위임하기 전에 먼저 에이전트 세션을 기동해 주세요." >&2 echo " 팁: 'multi-agent-mux-resume' 또는 'multi-agent-mux-create'를 통해 에이전트를 먼저 생성할 수 있습니다." >&2 return 1 diff --git a/tests/test_issue34_job_tree.py b/tests/test_issue34_job_tree.py index 8ae1ee3..1d5c732 100644 --- a/tests/test_issue34_job_tree.py +++ b/tests/test_issue34_job_tree.py @@ -468,23 +468,33 @@ def test_t6_an_unattributed_open_job_is_shown_and_waited_for_only_on_request(tmp def test_t8_the_stale_review_guard_without_worktree_or_base(tmp_path): + """Armed only when the run has sub-jobs. Without them the loop behaves as + before - its source may hold untracked files that change on every job + (test_tier4_e2e scenario 5: the mock herdr's state file).""" ctl = _loop_ctl(tmp_path) - res = _run_loop(ctl, _loop_env(ctl, FAKE_LATE_WRITE="1"), "--max-loop", "2") + res = _run_loop(ctl, _loop_env(ctl, FAKE_LATE_WRITE="1"), "--max-loop", "1") + assert res.returncode == 0, res.stdout + res.stderr + assert "source changed" not in res.stdout and "100% PASS" in res.stdout + + ctl2 = _loop_ctl(tmp_path / "with-child") + env2 = _loop_env(ctl2, FAKE_WORKER="child", FAKE_LATE_WRITE="1") + res = _run_loop(ctl2, env2, "--max-loop", "2", + on_child=lambda cid: _publish(env2, ctl2 / ".mam" / "jobs", cid, "completed", "done")) out = res.stdout + res.stderr assert res.returncode == 0, out + # once: round 2 is not stale although .mam (jobs, logs, checkpoint) inside + # the source kept changing - the control plane is not part of the fingerprint assert res.stdout.count("approvals discarded, re-reviewing") == 1 - assert len(_review_records(ctl)) == 2 - assert "100% PASS" in res.stdout - # the control plane inside the source (.mam: jobs, logs, checkpoint) is not a change - res_quiet = _run_loop(ctl, _loop_env(ctl), "--max-loop", "1") - assert res_quiet.returncode == 0 and "source changed" not in res_quiet.stdout + assert len(_review_records(ctl2)) == 2 and "100% PASS" in res.stdout # no repository: no fingerprint, the guard is off and says so; behaviour as before plain = _loop_ctl(tmp_path / "plain", git=False) - res_plain = _run_loop(plain, _loop_env(plain, FAKE_LATE_WRITE="1"), "--max-loop", "1") - assert res_plain.returncode == 0, res_plain.stdout + res_plain.stderr - assert "the stale-review guard is off for this run" in res_plain.stdout - assert "100% PASS" in res_plain.stdout + env3 = _loop_env(plain, FAKE_WORKER="child", FAKE_LATE_WRITE="1") + res = _run_loop(plain, env3, "--max-loop", "1", + on_child=lambda cid: _publish(env3, plain / ".mam" / "jobs", cid, "completed", "done")) + assert res.returncode == 0, res.stdout + res.stderr + assert "the stale-review guard is off for this run" in res.stdout + assert "100% PASS" in res.stdout @pytest.mark.parametrize("child_end, rc", [("open", 1), ("error", 1), ("completed", 0)]) @@ -510,3 +520,79 @@ def test_t9_a_correction_round_keeps_the_earlier_creator_jobs_sub_jobs(tmp_path, state = {"open": "OPEN", "error": "error", "completed": "completed"}[child_end] assert re.search(rf"\| {cid} \| .* \| {state} \|", second), second assert ("100% PASS" in res.stdout) == (rc == 0) + + +# --------------------------------------------------------------------------- +# T10-T12 - a pane caller reaches an agent of another workspace on the same +# server (mock herdr; the pane env is the one measured in creator-claude-issue34) +# --------------------------------------------------------------------------- + +SAME_SERVER_PANE = { # herdr 0.9.1, creator in w6 of server multi-agent-mux + "HERDR_SESSION": "multi-agent-mux", + "HERDR_SOCKET_PATH": "/home/someone/.config/herdr/sessions/multi-agent-mux/herdr.sock", + "HERDR_SESSION_NAME": "multi-agent-mux", + "HERDR_WORKSPACE_ID": "w6", "HERDR_PANE_ID": "w6:p1", "HERDR_TAB_ID": "w6:t1", +} + + +def _run_lib(sandbox: Path, body: str, env: dict) -> subprocess.CompletedProcess: + """Source the sandbox lib.sh with exactly these HERDR_* (as test_issue23_24).""" + script = 'source "%s"\n_init_herdr_isolation\n%s\n' % (sandbox / ".agents" / "skills" / "lib.sh", body) + run_env = {k: v for k, v in os.environ.items() if not k.startswith("HERDR_")} + run_env.update(env) + return subprocess.run(["bash", "-c", script], capture_output=True, text=True, cwd=str(sandbox), env=run_env) + + +def _seed_herdr_agent(state_file: Path, sandbox: Path, name: str, ws: str, pane: str) -> None: + state = json.loads(state_file.read_text()) + if not any(w.get("workspace_id") == ws for w in state.setdefault("workspaces", [])): + state["workspaces"].append({"workspace_id": ws, "label": ws, "cwd": str(sandbox)}) + state.setdefault("agents", {})[name] = { + "name": name, "agent": "opencode", "status": "running", "cwd": str(sandbox), "workspace_id": ws, + "pane_id": pane, "pid": 9999, "command": "opencode", "buffer": "Ready"} + state.setdefault("panes", []).append({"pane_id": pane, "workspace_id": ws, "label": name, "buffer": "Ready"}) + state_file.write_text(json.dumps(state, indent=2)) + + +def test_t10_a_pane_caller_reaches_an_agent_of_another_workspace_on_its_server( + mam_sandbox, mock_herdr, mock_agents): + _seed_herdr_agent(mock_herdr, mam_sandbox, "reviewer-opencode-01", "w2", "w2:p1") + res = _run_lib(mam_sandbox, "herdr has-session -t reviewer-opencode-01", SAME_SERVER_PANE) + assert res.returncode == 0, res.stdout + res.stderr + before = len(json.loads(mock_herdr.read_text()).get("calls", [])) + res = _run_lib(mam_sandbox, 'herdr agent prompt reviewer-opencode-01 "hello from w6"', SAME_SERVER_PANE) + assert res.returncode == 0, res.stdout + res.stderr + calls = json.loads(mock_herdr.read_text())["calls"][before:] + assert any(c[-4:] == ["agent", "prompt", "reviewer-opencode-01", "hello from w6"] for c in calls), calls + + +def test_t11_a_workspace_id_the_caller_chose_still_scopes(mam_sandbox, mock_herdr, mock_agents): + """The ISSUE-3 / F-2b contract: an id that is not the pane's own (the pane + id says w6, the caller asks for w5), or one set outside any pane, hides an + agent of another workspace.""" + _seed_herdr_agent(mock_herdr, mam_sandbox, "reviewer-opencode-01", "w2", "w2:p1") + chosen = dict(SAME_SERVER_PANE, HERDR_WORKSPACE_ID="w5") + assert _run_lib(mam_sandbox, "herdr has-session -t reviewer-opencode-01", chosen).returncode == 1 + outside = {"HERDR_SESSION_NAME": "multi-agent-mux", "HERDR_WORKSPACE_ID": "w6"} + assert _run_lib(mam_sandbox, "herdr has-session -t reviewer-opencode-01", outside).returncode == 1 + # the pane's own id is still what new panes are created in: only name lookups drop it + res = _run_lib(mam_sandbox, '_herdr_scope_is_pane_inherited pane list && echo strip || echo keep', + SAME_SERVER_PANE) + assert res.stdout.strip() == "keep", res.stdout + res.stderr + + +def test_t12_a_target_the_preflight_cannot_see_is_not_recorded_as_busy(mam_sandbox, mock_herdr, mock_agents): + reg = mam_sandbox / ".mam" / "jobs" + env = {k: v for k, v in os.environ.items() if not k.startswith("HERDR_")} + env.update(UNREACHABLE, DELEGATE_JOB_PYTHON=sys.executable, MAM_JOB_ID="", + DELEGATE_JOB_LOGS_DIR=str(mam_sandbox / ".mam" / "logs")) + res = subprocess.run(["bash", str(mam_sandbox / ".agents" / "skills" / "multi-agent-mux-delegate-job" + / "multi-agent-mux-delegate-job"), + "submit", "--agent", "opencode", "--agent-session", "herdr:no-such-agent", + "--registry-dir", str(reg), "--prompt", "x"], + capture_output=True, text=True, env=env, cwd=str(mam_sandbox), timeout=120) + assert res.returncode == 5, res.stdout + res.stderr + jid = re.search(r"registered job:\s*(\S+)", res.stdout).group(1) + assert _record(reg, jid)["status"] == "cancelled" + events = (reg / f"{jid}.events.log").read_text() + assert '"detail": "target_not_found"' in events and "session_busy" not in events, events