diff --git a/.agents/skills/lib.sh b/.agents/skills/lib.sh index 1b0283a..c39b9e2 100644 --- a/.agents/skills/lib.sh +++ b/.agents/skills/lib.sh @@ -1373,9 +1373,50 @@ EOF fi } +# _herdr_call_is_cross_server +# +# ISSUE-23: a herdr pane exports HERDR_WORKSPACE_ID / HERDR_PANE_ID / +# HERDR_TAB_ID, and those ids only mean something on the server that issued +# them. When a MAM script runs inside one server's pane (the orchestrator's) +# but targets another (the isolated MAM server named by HERDR_SESSION_NAME), +# the inherited ids reach the scope filters (_herdr_ws_scope, +# _herdr_agent_get_scoped, _resolve_herdr_pane_id) as a foreign workspace, and +# their fail-closed miss (lib.sh explicit-env miss, ISSUE-3 / F-2b) reports a +# live session as absent: has-session, kill-session, agent prompt and the +# delegate-job preflight all answer "no such session". +# +# True only for a call aimed at a *named* server other than the caller's own: +# +# * target unset or `default` (the lib.sh:97 fallback) is the shim's sentinel +# for "pass no session flag", so herdr answers on the ambient server, which +# for a caller inside a pane is that same caller's server - not a crossing; +# * no HERDR_SESSION means the caller is not in a pane at all, so an exported +# HERDR_WORKSPACE_ID is a deliberate scope, not a leak (H-18/H-21/H-23); +# * caller == target is the same server. +_herdr_call_is_cross_server() { + local caller="${HERDR_SESSION:-}" target="${HERDR_SESSION_NAME:-}" + case "$target" in + ""|default) return 1 ;; + esac + if [ -n "$caller" ] && [ "$caller" != "$target" ]; then + return 0 + fi + return 1 +} + mam_herdr() { _init_herdr_isolation - "$WORKSPACE_ROOT/.mam/shim/herdr" "$@" + # Built as an argv array and decided inside `if`: this runs under + # `set -euo pipefail` in most callers, where a bare failing test would abort + # the whole script instead of this one call. + local -a _scope_strip=() + if _herdr_call_is_cross_server; then + _scope_strip=(env -u HERDR_WORKSPACE_ID -u HERDR_PANE_ID -u HERDR_TAB_ID) + if [ "${MAM_SHIM_DEBUG:-0}" = "1" ]; then + echo "mam_herdr: dropping caller pane scope ids (caller server '${HERDR_SESSION:-}' != target '${HERDR_SESSION_NAME:-}')" >&2 + fi + fi + ${_scope_strip[@]+"${_scope_strip[@]}"} "$WORKSPACE_ROOT/.mam/shim/herdr" "$@" } _herdr() { diff --git a/.agents/skills/lib_py/loop/watcher.py b/.agents/skills/lib_py/loop/watcher.py index 7980d62..e33ff6e 100644 --- a/.agents/skills/lib_py/loop/watcher.py +++ b/.agents/skills/lib_py/loop/watcher.py @@ -19,6 +19,7 @@ from __future__ import annotations import json import logging +import os import subprocess from typing import Any, Dict, Optional, Sequence @@ -38,6 +39,30 @@ TRANSPORT_ERROR = "transport_error" BACKOFF_START_S = 1.0 BACKOFF_MAX_S = 16.0 +# ISSUE-23: ids a herdr pane exports about ITS OWN server. Passing them to a +# call aimed at a different server makes herdr scope the lookup to a workspace +# that does not exist there, and the target is reported as missing. The shell +# side drops them in mam_herdr; this is the one python path that runs herdr +# without going through it. +CALLER_SCOPE_VARS = ("HERDR_WORKSPACE_ID", "HERDR_PANE_ID", "HERDR_TAB_ID") + + +def scoped_env(session=None, environ=None): + """The environment for a wait aimed at `session`. + + Unchanged unless this process runs inside a pane of another server: only + then are the caller's own workspace/pane/tab ids meaningless for the call + and dropped. No pane (no HERDR_SESSION) and same-server keep every value. + """ + src = os.environ if environ is None else environ + caller = (src.get("HERDR_SESSION") or "").strip() + target = (session or "").strip() + # `default` is the same sentinel build_argv reads: no --session flag, so + # herdr answers on the ambient server - the caller's own. + if not caller or not target or target == "default" or caller == target: + return dict(src) + return dict((k, v) for k, v in src.items() if k not in CALLER_SCOPE_VARS) + class WaitOutcome(object): __slots__ = ("kind", "agent", "error_code", "detail", "raw") @@ -92,7 +117,7 @@ def wait_once(target: str, until: Sequence[str] = DEFAULT_UNTIL, """One barrier call. `run` is injectable so tests never touch a real herdr.""" argv = build_argv(target, until, timeout_ms, session=session) try: - proc = run(argv, capture_output=True, text=True) + proc = run(argv, capture_output=True, text=True, env=scoped_env(session)) except Exception as exc: return WaitOutcome(TRANSPORT_ERROR, detail="herdr could not be executed: {}".format(exc)) diff --git a/tests/test_issue23_24_startup_path.py b/tests/test_issue23_24_startup_path.py new file mode 100644 index 0000000..c5d8d3f --- /dev/null +++ b/tests/test_issue23_24_startup_path.py @@ -0,0 +1,304 @@ +"""Issue #23 + #24 — the create_session start path. + +Four regressions live here, in the order of the issues: + +1. a herdr call aimed at another server must not carry the caller pane's own + workspace/pane/tab ids (#23 comments: reproduced on Linux and macOS); +2. blocking start-up modals are agent-adapter knowledge and are answered while + waiting for the TUI (#24); +3. onboarding delivery is judged by the agent's own `started` event (#23); +4. a rollback verifies the kill and leaves no `running` row or dangling job (#23). + +What is mocked and what is real is stated per test. The dialog tests fix OUR +contract - which pane string gets which keys - not that the keys drive a real +agy/grok/codex/opencode TUI; no such TUI can be started here. +""" + +import json +import os +import subprocess +import textwrap + +import pytest + + +# -------------------------------------------------------------------------- +# helpers +# -------------------------------------------------------------------------- + +def _run_lib(tmp_path, body, env=None): + """Source the sandbox lib.sh and run `body`. Unlike the shim-contract + helper this keeps HERDR_* exactly as `env` sets it: these tests are about + what the caller's pane leaks in.""" + lib_path = tmp_path / ".agents" / "skills" / "lib.sh" + script = 'source "%s"\n_init_herdr_isolation\n%s\n' % (lib_path, body) + run_env = dict(os.environ) + for var in ("HERDR_SESSION", "HERDR_SESSION_NAME", "HERDR_WORKSPACE_ID", + "HERDR_PANE_ID", "HERDR_TAB_ID"): + run_env.pop(var, None) + run_env.update(env or {}) + return subprocess.run(["bash", "-c", script], capture_output=True, + text=True, cwd=str(tmp_path), env=run_env) + + +def _mock_state(mock_herdr): + with open(mock_herdr) as f: + return json.load(f) + + +def _write_mock_state(mock_herdr, state): + with open(mock_herdr, "w") as f: + json.dump(state, f, indent=2) + + +def _seed_agent(mock_herdr, tmp_path, name, workspace_id, pane_id): + state = _mock_state(mock_herdr) + state.setdefault("workspaces", []) + if not any(w.get("workspace_id") == workspace_id for w in state["workspaces"]): + state["workspaces"].append( + {"workspace_id": workspace_id, "label": workspace_id, "cwd": str(tmp_path)}) + state.setdefault("agents", {})[name] = { + "name": name, "agent": "agy", "status": "running", "cwd": str(tmp_path), + "workspace_id": workspace_id, "pane_id": pane_id, "pid": 9999, + "command": "agy", "buffer": "Ready", + } + state.setdefault("panes", []).append( + {"pane_id": pane_id, "workspace_id": workspace_id, "label": name, "buffer": "Ready"}) + _write_mock_state(mock_herdr, state) + + +def _install_env_recorder(tmp_path, monkeypatch): + """A herdr earlier in PATH than the mock that dumps the env it was called + with, then execs the mock. The shim resolves the first non-shim herdr on + PATH, so this observes exactly what reached the real binary.""" + rec_dir = tmp_path / "recorder" + rec_dir.mkdir(parents=True, exist_ok=True) + env_log = tmp_path / "herdr_env.ndjson" + real = tmp_path / "bin" / "herdr" + script = rec_dir / "herdr" + script.write_text(textwrap.dedent('''\ + #!/usr/bin/env python3 + import json, os, sys + with open(%r, "a") as f: + f.write(json.dumps({ + "argv": sys.argv[1:], + "HERDR_WORKSPACE_ID": os.environ.get("HERDR_WORKSPACE_ID"), + "HERDR_PANE_ID": os.environ.get("HERDR_PANE_ID"), + "HERDR_TAB_ID": os.environ.get("HERDR_TAB_ID"), + "HERDR_SESSION": os.environ.get("HERDR_SESSION"), + "HERDR_SESSION_NAME": os.environ.get("HERDR_SESSION_NAME"), + }) + "\\n") + os.execv(%r, [%r] + sys.argv[1:]) + ''') % (str(env_log), str(real), str(real))) + script.chmod(0o755) + monkeypatch.setenv("PATH", "%s:%s" % (rec_dir, os.environ["PATH"])) + return env_log + + +def _recorded(env_log): + if not os.path.exists(env_log): + return [] + return [json.loads(l) for l in open(env_log) if l.strip()] + + +# -------------------------------------------------------------------------- +# item 1 — caller pane scope must not leak across servers +# -------------------------------------------------------------------------- + +CROSS = {"HERDR_SESSION": "default", "HERDR_SESSION_NAME": "mam-isolated", + "HERDR_WORKSPACE_ID": "w5", "HERDR_PANE_ID": "w5:p2", + "HERDR_TAB_ID": "w5:t1"} + + +def test_s1_cross_server_call_drops_the_caller_panes_scope_ids( + mam_sandbox, mock_herdr, mock_agents, monkeypatch): + """S1 (mock herdr): the caller sits in another server's pane. has-session, + kill-session and agent prompt must find the agent, and the three ids must + not reach herdr.""" + tmp_path = mam_sandbox + env_log = _install_env_recorder(tmp_path, monkeypatch) + _seed_agent(mock_herdr, tmp_path, "creator-agy-01", "w1", "w1:p5") + + res = _run_lib(tmp_path, 'herdr has-session -t creator-agy-01', env=CROSS) + assert res.returncode == 0, res.stdout + res.stderr + + res = _run_lib(tmp_path, 'herdr agent prompt creator-agy-01 "hello"', env=CROSS) + assert res.returncode == 0, res.stdout + res.stderr + + res = _run_lib(tmp_path, 'herdr kill-session -t creator-agy-01', env=CROSS) + assert res.returncode == 0, res.stdout + res.stderr + + calls = _recorded(env_log) + assert calls, "the recorder saw no herdr call" + for call in calls: + assert call["HERDR_WORKSPACE_ID"] is None, call + assert call["HERDR_PANE_ID"] is None, call + assert call["HERDR_TAB_ID"] is None, call + + # the kill really happened, i.e. this is not a silent no-op + assert "creator-agy-01" not in _mock_state(mock_herdr).get("agents", {}) + + +def test_s1b_the_delegate_job_wrappers_bare_herdr_is_the_lib_sh_function( + mam_sandbox, mock_herdr, mock_agents, monkeypatch): + """S1b (mock herdr): the wrapper sources lib.sh and then calls a bare + `herdr` (wrapper :918 preflight). A shell function beats PATH, so that call + is mam_herdr and is fixed by item 1 too - this is why #23's preflight + failure is cured by the same change.""" + tmp_path = mam_sandbox + _install_env_recorder(tmp_path, monkeypatch) + _seed_agent(mock_herdr, tmp_path, "creator-agy-01", "w1", "w1:p5") + + res = _run_lib(tmp_path, 'echo "kind=$(type -t herdr)"\nherdr has-session -t creator-agy-01', + env=CROSS) + assert "kind=function" in res.stdout, res.stdout + res.stderr + assert res.returncode == 0, res.stdout + res.stderr + + +def test_s2_same_server_keeps_the_explicit_workspace_scope( + mam_sandbox, mock_herdr, mock_agents, monkeypatch): + """S2 (mock herdr): on the target's own server the exported scope still + scopes - a same-named agent in another workspace is not addressed (H-22).""" + tmp_path = mam_sandbox + _seed_agent(mock_herdr, tmp_path, "creator-agy-01", "w1", "w1:p5") + same = {"HERDR_SESSION": "mam-isolated", "HERDR_SESSION_NAME": "mam-isolated", + "HERDR_WORKSPACE_ID": "w2"} + + before = len(_mock_state(mock_herdr).get("calls", [])) + res = _run_lib(tmp_path, 'herdr agent prompt creator-agy-01 "hello from w2"', env=same) + assert res.returncode != 0, res.stdout + res.stderr + new = _mock_state(mock_herdr).get("calls", [])[before:] + prompts = [c for c in new if len(c) >= 2 and c[0] == "agent" and c[1] == "prompt"] + assert prompts == [], new + + +def test_s2b_a_caller_outside_any_pane_keeps_the_explicit_scope( + mam_sandbox, mock_herdr, mock_agents): + """S2b (mock herdr): no HERDR_SESSION means the caller is not in a pane at + all, so an exported HERDR_WORKSPACE_ID is a deliberate scope, not a leak. + This is the case H-18/H-21/H-23 fix.""" + tmp_path = mam_sandbox + _seed_agent(mock_herdr, tmp_path, "creator-agy-01", "w1", "w1:p5") + + res = _run_lib(tmp_path, 'herdr has-session -t creator-agy-01', + env={"HERDR_SESSION_NAME": "mam-isolated", "HERDR_WORKSPACE_ID": "w2"}) + assert res.returncode == 1, "an explicit out-of-scope id must still hide the agent" + + res = _run_lib(tmp_path, 'herdr has-session -t creator-agy-01', + env={"HERDR_SESSION_NAME": "mam-isolated", "HERDR_WORKSPACE_ID": "w1"}) + assert res.returncode == 0, res.stdout + res.stderr + + +def test_s3_shim_contract_regressions_still_pass(mam_sandbox): + """S3: H-18 / H-21 / H-23 are the tests that fix today's scoping meaning. + Run them explicitly as this change's regression guard.""" + repo = os.path.dirname(os.path.dirname(os.path.abspath(__file__))) + res = subprocess.run( + [os.path.join(repo, ".venv", "bin", "python"), "-m", "pytest", "-q", + "-p", "no:cacheprovider", + "tests/test_herdr_shim_contract.py", "-k", + "h18 or h21 or h22 or h23"], + capture_output=True, text=True, cwd=repo, + env=dict(os.environ, PYTHONDONTWRITEBYTECODE="1")) + assert res.returncode == 0, res.stdout[-4000:] + res.stderr[-2000:] + + +def test_s11_this_branch_does_not_change_the_generated_shim(mam_sandbox): + """S11: the item-1 fix lives in mam_herdr, never in the heredoc that + lib.sh materialises into $WORKSPACE_ROOT/.mam/shim/herdr. That file is + shared by every checkout bound to one control plane, so keeping it + byte-identical to base is what makes "the control plane is not modified" + true at runtime.""" + repo = os.path.dirname(os.path.dirname(os.path.abspath(__file__))) + base = os.environ.get("MAM_BASE_SHA", "1e78dae") + show = subprocess.run(["git", "show", "%s:.agents/skills/lib.sh" % base], + capture_output=True, text=True, cwd=repo) + if show.returncode != 0: + pytest.skip("base %s not available in this checkout" % base) + with open(os.path.join(repo, ".agents", "skills", "lib.sh")) as f: + head = f.read() + assert _heredoc_body(head) == _heredoc_body(show.stdout), ( + "the shim heredoc changed: the generated .mam/shim/herdr of this branch " + "would differ from the control root's, and the two would overwrite " + "each other on every herdr call") + + +def _heredoc_body(text): + lines = text.splitlines(True) + start = None + for i, line in enumerate(lines): + if line.strip() == "cat <<'EOF' > \"$tmp_file\"": + start = i + 1 + break + assert start is not None, "shim heredoc not found in lib.sh" + for j in range(start, len(lines)): + if lines[j].rstrip("\n") == "EOF": + return "".join(lines[start:j]) + raise AssertionError("unterminated shim heredoc") + + +@pytest.mark.parametrize("env,expected_rc", [(CROSS, 1), ({"HERDR_SESSION_NAME": "mam-isolated"}, 1)]) +def test_s12_mam_herdr_never_aborts_its_caller_under_set_e( + mam_sandbox, mock_herdr, mock_agents, env, expected_rc): + """S12 (mock herdr): mam_herdr runs inside scripts using `set -euo + pipefail`. Neither branch of the cross-server test may abort the shell, + and the child's rc must be the function's rc.""" + tmp_path = mam_sandbox + body = ('set -euo pipefail\n' + 'rc=0\n' + 'mam_herdr has-session -t nobody-here || rc=$?\n' + 'echo "missing_rc=$rc"\n' + '_seen=0\n' + 'mam_herdr has-session -t creator-agy-01 || _seen=$?\n' + 'echo "present_rc=$_seen"\n' + 'echo REACHED_END\n') + _seed_agent(mock_herdr, tmp_path, "creator-agy-01", "w1", "w1:p5") + lib_path = tmp_path / ".agents" / "skills" / "lib.sh" + script = 'source "%s"\n%s' % (lib_path, body) + run_env = dict(os.environ) + for var in ("HERDR_SESSION", "HERDR_SESSION_NAME", "HERDR_WORKSPACE_ID", + "HERDR_PANE_ID", "HERDR_TAB_ID"): + run_env.pop(var, None) + run_env.update(env) + res = subprocess.run(["bash", "-c", script], capture_output=True, text=True, + cwd=str(tmp_path), env=run_env) + assert "REACHED_END" in res.stdout, res.stdout + res.stderr + assert "missing_rc=%d" % expected_rc in res.stdout, res.stdout + res.stderr + assert "present_rc=0" in res.stdout, res.stdout + res.stderr + + +def test_s13_the_loop_watcher_applies_the_same_rule(mam_sandbox): + """S13 (pure unit): watcher.py builds its own herdr argv and never goes + through mam_herdr, so it needs the same rule.""" + import sys + repo = os.path.dirname(os.path.dirname(os.path.abspath(__file__))) + sys.path.insert(0, os.path.join(repo, ".agents", "skills")) + from lib_py.loop import watcher + + caller_pane = {"HERDR_SESSION": "default", "HERDR_WORKSPACE_ID": "w5", + "HERDR_PANE_ID": "w5:p2", "HERDR_TAB_ID": "w5:t1", "PATH": "/usr/bin"} + + cross = watcher.scoped_env("mam-isolated", environ=caller_pane) + assert "HERDR_WORKSPACE_ID" not in cross and "HERDR_PANE_ID" not in cross + assert "HERDR_TAB_ID" not in cross and cross["PATH"] == "/usr/bin" + + same = watcher.scoped_env("default", environ=caller_pane) + assert same["HERDR_WORKSPACE_ID"] == "w5" + + outside = watcher.scoped_env("mam-isolated", environ={"HERDR_WORKSPACE_ID": "w5"}) + assert outside["HERDR_WORKSPACE_ID"] == "w5" + + none_target = watcher.scoped_env(None, environ=caller_pane) + assert none_target["HERDR_WORKSPACE_ID"] == "w5" + + seen = {} + + def _run(argv, **kw): + seen["env"] = kw.get("env") + + class P: + returncode, stdout, stderr = 0, "{}", "" + return P() + + watcher.wait_once("agt", session="iso-x", run=_run) + assert seen["env"] is not None and "HERDR_SESSION" not in ("",)