artifact presenter & global Agents updates & plan for nono profiles

skills/document/artifact-presenter/SKILL.md

@@ -43,7 +43,7 @@ Run the helper using its absolute path resolved from this skill directory:
 python3 <skill-directory>/scripts/present.py <artifact-path>
 ```
 
-The helper reuses the pane labeled `artifact presenter` in the current Herdr workspace. When none exists, it creates a right split without taking focus. It invokes `mcat <artifact-path>` directly so mcat can infer and display documents, source, archives, PDFs, images, and videos. It falls back to artifact-only delivery when Herdr or mcat is unavailable.
+The helper reuses the pane labeled `artifact presenter` in the current Herdr workspace. It serializes pane discovery, creation, and replacement per workspace, with an additional per-pane lock during viewer handoff. When that pane is already displaying an artifact, it closes the active `mcat` viewer and waits for the pane shell before launching the replacement through a shell-neutral helper command with encoded path arguments. It confirms the helper started the viewer rather than trusting terminal input delivery alone. It leaves unrelated foreground programs untouched and creates a new viewer pane instead. When no reusable pane exists, it creates a right split without taking focus. It launches `mcat <artifact-path>` so mcat can infer and display documents, source, archives, PDFs, images, and videos. It falls back to artifact-only delivery when Herdr or mcat is unavailable.
 
 Completion criterion: capture whether the helper opened, reused, or skipped the viewer pane.
 

skills/document/artifact-presenter/scripts/present.py

@@ -4,15 +4,44 @@
 from __future__ import annotations
 
 import argparse
+import base64
+import fcntl
+import hashlib
 import json
 import os
+import re
+import secrets
 import shutil
 import subprocess
+import tempfile
+import time
+from contextlib import contextmanager
 from dataclasses import dataclass
 from pathlib import Path
 from typing import Callable, Mapping, Sequence
 
 PANE_LABEL = "artifact presenter"
+VIEWER_PROCESS_NAME = "mcat"
+PANE_READY_ATTEMPTS = 40
+PANE_READY_DELAY_SECONDS = 0.05
+PANE_SHELL_STABLE_CHECKS = 2
+VIEWER_START_ATTEMPTS = 40
+SAFE_COMMAND_TOKEN = re.compile(r"^[A-Za-z0-9_./=-]+$")
+VIEWER_LAUNCH_SCRIPT = """#!/usr/bin/env python3
+import base64
+import os
+import sys
+from pathlib import Path
+
+
+def decode(value: str) -> str:
+    return base64.urlsafe_b64decode(value.encode("ascii")).decode("utf-8")
+
+
+acknowledgment, token, viewer, artifact = map(decode, sys.argv[1:])
+Path(acknowledgment).write_text(token, encoding="utf-8")
+os.execv(viewer, [viewer, artifact])
+"""
 
 RunCommand = Callable[..., subprocess.CompletedProcess[str]]
 FindExecutable = Callable[[str], str | None]
@@ -74,43 +103,174 @@ def created_pane_id(response: Mapping[str, object]) -> str:
     return pane_id
 
 
+@dataclass(frozen=True)
+class PaneProcessState:
+    shell_pid: int
+    foreground_pids: frozenset[int]
+    foreground_names: frozenset[str]
+
+    @property
+    def shell_is_ready(self) -> bool:
+        return self.shell_pid in self.foreground_pids
+
+    @property
+    def viewer_is_active(self) -> bool:
+        return VIEWER_PROCESS_NAME in self.foreground_names
+
+
+def pane_process_state(response: Mapping[str, object]) -> PaneProcessState:
+    result = response.get("result")
+    process_info = result.get("process_info") if isinstance(result, dict) else None
+    if not isinstance(process_info, dict):
+        raise ValueError("Herdr process response is missing process info")
+
+    shell_pid = process_info.get("shell_pid")
+    processes = process_info.get("foreground_processes")
+    if not isinstance(shell_pid, int) or not isinstance(processes, list):
+        raise ValueError("Herdr process response is missing shell state")
+
+    pids: set[int] = set()
+    names: set[str] = set()
+    for process in processes:
+        if not isinstance(process, dict):
+            continue
+        pid = process.get("pid")
+        name = process.get("name")
+        if isinstance(pid, int):
+            pids.add(pid)
+        if isinstance(name, str) and name:
+            names.add(name)
+    return PaneProcessState(shell_pid, frozenset(pids), frozenset(names))
+
+
+def prepare_pane_for_command(
+    herdr: str,
+    pane_id: str,
+    run: RunCommand,
+    *,
+    allow_startup_transition: bool,
+) -> None:
+    viewer_closed = False
+    stable_shell_checks = 0
+    for attempt in range(PANE_READY_ATTEMPTS):
+        response = run_json(herdr, ["pane", "process-info", "--pane", pane_id], run)
+        state = pane_process_state(response)
+        if state.shell_is_ready:
+            stable_shell_checks += 1
+            if stable_shell_checks >= PANE_SHELL_STABLE_CHECKS:
+                return
+        else:
+            stable_shell_checks = 0
+        if state.viewer_is_active:
+            if not viewer_closed:
+                run(
+                    [herdr, "pane", "send-keys", pane_id, "q"],
+                    check=True,
+                    capture_output=True,
+                    text=True,
+                )
+                viewer_closed = True
+        elif (
+            state.foreground_pids
+            and not state.shell_is_ready
+            and not viewer_closed
+            and not allow_startup_transition
+        ):
+            processes = ", ".join(sorted(state.foreground_names)) or "unknown"
+            raise ValueError(f"pane is running an unrelated foreground process: {processes}")
+
+        if attempt + 1 < PANE_READY_ATTEMPTS:
+            time.sleep(PANE_READY_DELAY_SECONDS)
+
+    raise ValueError("pane shell did not become ready for the artifact command")
+
+
+def encode_argument(value: str) -> str:
+    return base64.urlsafe_b64encode(value.encode("utf-8")).decode("ascii")
+
+
+@contextmanager
+def lock_pane(pane_id: str):
+    runtime_directory = Path(os.environ.get("XDG_RUNTIME_DIR", tempfile.gettempdir()))
+    lock_directory = runtime_directory / "pi-artifact-presenter-locks"
+    lock_directory.mkdir(mode=0o700, exist_ok=True)
+    lock_name = hashlib.sha256(pane_id.encode("utf-8")).hexdigest() + ".lock"
+    descriptor = os.open(
+        lock_directory / lock_name,
+        os.O_CREAT | os.O_RDWR | os.O_CLOEXEC,
+        0o600,
+    )
+    with os.fdopen(descriptor, "w") as handle:
+        fcntl.flock(handle, fcntl.LOCK_EX)
+        try:
+            yield
+        finally:
+            fcntl.flock(handle, fcntl.LOCK_UN)
+
+
 def run_mcat(
     herdr: str,
     mcat: str,
     pane_id: str,
     artifact: Path,
     run: RunCommand,
-) -> None:
-    run(
-        [herdr, "pane", "run", pane_id, mcat, str(artifact)],
-        check=True,
-        capture_output=True,
-        text=True,
-    )
-
-
-def present_document(
-    document: Path,
     *,
-    env: Mapping[str, str] = os.environ,
-    which: FindExecutable = shutil.which,
-    run: RunCommand = subprocess.run,
+    reused: bool = False,
+) -> None:
+    with lock_pane(pane_id):
+        prepare_pane_for_command(
+            herdr,
+            pane_id,
+            run,
+            allow_startup_transition=not reused,
+        )
+
+        token = secrets.token_hex(12)
+        with tempfile.TemporaryDirectory(
+            prefix="pi-artifact-presenter-",
+            dir="/tmp",
+        ) as acknowledgment_directory:
+            temporary = Path(acknowledgment_directory)
+            acknowledgment = temporary / "viewer-started"
+            launcher = temporary / "launch-viewer.py"
+            launcher.write_text(VIEWER_LAUNCH_SCRIPT, encoding="utf-8")
+            command_parts = [
+                "python3",
+                str(launcher),
+                encode_argument(str(acknowledgment)),
+                encode_argument(token),
+                encode_argument(mcat),
+                encode_argument(str(artifact)),
+            ]
+            if not all(SAFE_COMMAND_TOKEN.fullmatch(part) for part in command_parts):
+                raise ValueError("viewer launcher produced an unsafe terminal token")
+            command = " ".join(command_parts)
+            run(
+                [herdr, "pane", "send-text", pane_id, command + "\r"],
+                check=True,
+                capture_output=True,
+                text=True,
+            )
+            for attempt in range(VIEWER_START_ATTEMPTS):
+                try:
+                    if acknowledgment.read_text(encoding="utf-8") == token:
+                        return
+                except FileNotFoundError:
+                    pass
+                if attempt + 1 < VIEWER_START_ATTEMPTS:
+                    time.sleep(PANE_READY_DELAY_SECONDS)
+
+        raise ValueError("artifact command was delivered but did not start")
+
+
+def present_in_workspace(
+    artifact: Path,
+    herdr: str,
+    mcat: str,
+    workspace_id: str,
+    source_pane_id: str,
+    run: RunCommand,
 ) -> PresentationResult:
-    artifact = document.expanduser().resolve()
-    if not artifact.is_file():
-        raise ValueError(f"artifact is not a file: {artifact}")
-
-    herdr = which("herdr")
-    mcat = which("mcat")
-    workspace_id = env.get("HERDR_WORKSPACE_ID")
-    source_pane_id = env.get("HERDR_PANE_ID")
-    if not herdr:
-        return PresentationResult("artifact-only", artifact, reason="herdr is unavailable")
-    if not mcat:
-        return PresentationResult("artifact-only", artifact, reason="mcat is unavailable")
-    if not workspace_id or not source_pane_id:
-        return PresentationResult("artifact-only", artifact, reason="not running in a Herdr pane")
-
     try:
         pane_list = run_json(herdr, ["pane", "list", "--workspace", workspace_id], run)
         existing_panes = panes_with_label(pane_list)
@@ -119,9 +279,9 @@ def present_document(
 
     for existing_pane in existing_panes:
         try:
-            run_mcat(herdr, mcat, existing_pane, artifact, run)
+            run_mcat(herdr, mcat, existing_pane, artifact, run, reused=True)
             return PresentationResult("reused", artifact, pane_id=existing_pane)
-        except (OSError, subprocess.SubprocessError):
+        except (OSError, subprocess.SubprocessError, ValueError):
             try:
                 run(
                     [herdr, "pane", "rename", existing_pane, f"{PANE_LABEL} stale"],
@@ -161,7 +321,7 @@ def present_document(
         )
         run_mcat(herdr, mcat, pane_id, artifact, run)
         return PresentationResult("opened", artifact, pane_id=pane_id)
-    except (OSError, subprocess.SubprocessError) as error:
+    except (OSError, subprocess.SubprocessError, ValueError) as error:
         try:
             run(
                 [herdr, "pane", "close", pane_id],
@@ -174,6 +334,39 @@ def present_document(
         return PresentationResult("artifact-only", artifact, reason=f"could not initialize Herdr viewer: {error}")
 
 
+def present_document(
+    document: Path,
+    *,
+    env: Mapping[str, str] = os.environ,
+    which: FindExecutable = shutil.which,
+    run: RunCommand = subprocess.run,
+) -> PresentationResult:
+    artifact = document.expanduser().resolve()
+    if not artifact.is_file():
+        raise ValueError(f"artifact is not a file: {artifact}")
+
+    herdr = which("herdr")
+    mcat = which("mcat")
+    workspace_id = env.get("HERDR_WORKSPACE_ID")
+    source_pane_id = env.get("HERDR_PANE_ID")
+    if not herdr:
+        return PresentationResult("artifact-only", artifact, reason="herdr is unavailable")
+    if not mcat:
+        return PresentationResult("artifact-only", artifact, reason="mcat is unavailable")
+    if not workspace_id or not source_pane_id:
+        return PresentationResult("artifact-only", artifact, reason="not running in a Herdr pane")
+
+    with lock_pane(f"workspace:{workspace_id}"):
+        return present_in_workspace(
+            artifact,
+            herdr,
+            mcat,
+            workspace_id,
+            source_pane_id,
+            run,
+        )
+
+
 def create_parser() -> argparse.ArgumentParser:
     parser = argparse.ArgumentParser(description=__doc__)
     parser.add_argument("artifact", type=Path, help="Local file to present with mcat")

skills/document/artifact-presenter/tests/test_present.py

@@ -1,10 +1,14 @@
 from __future__ import annotations
 
+import base64
 import importlib.util
 import json
+import os
 import subprocess
 import sys
 import tempfile
+import threading
+import time
 import unittest
 from pathlib import Path
 
@@ -23,21 +27,92 @@ class FakeRunner:
         *,
         failed_run_panes: set[str] | None = None,
         fail_rename: bool = False,
+        foreground_processes: dict[str, list[str]] | None = None,
+        shell_processes: dict[str, str] | None = None,
+        viewer_teardown_processes: dict[str, list[list[str]]] | None = None,
+        write_start_acknowledgment: bool = True,
+        list_delay_seconds: float = 0,
     ) -> None:
         self.panes = panes or []
         self.failed_run_panes = failed_run_panes or set()
         self.fail_rename = fail_rename
+        self.foreground_processes = {
+            pane_id: list(processes)
+            for pane_id, processes in (foreground_processes or {}).items()
+        }
+        self.shell_processes = shell_processes or {}
+        self.viewer_teardown_processes = {
+            pane_id: [list(processes) for processes in snapshots]
+            for pane_id, snapshots in (viewer_teardown_processes or {}).items()
+        }
+        self.viewer_closing_panes: set[str] = set()
+        self.write_start_acknowledgment = write_start_acknowledgment
+        self.pending_commands: dict[str, str] = {}
+        self.list_delay_seconds = list_delay_seconds
         self.calls: list[list[str]] = []
 
     def __call__(self, command, **_kwargs):
         argv = [str(value) for value in command]
         self.calls.append(argv)
-        if argv[1:3] == ["pane", "run"] and argv[3] in self.failed_run_panes:
-            raise subprocess.CalledProcessError(1, argv)
+        if argv[1:3] == ["pane", "send-text"]:
+            pane_id = argv[3]
+            if pane_id in self.failed_run_panes:
+                raise subprocess.CalledProcessError(1, argv)
+            self.pending_commands[pane_id] = argv[4]
+            command_parts = argv[4].strip().split()
+            decoded = [
+                base64.urlsafe_b64decode(value.encode("ascii")).decode("utf-8")
+                for value in command_parts[-4:]
+            ]
+            if self.write_start_acknowledgment and decoded[-2] == "/usr/bin/mcat":
+                Path(decoded[-4]).write_text(decoded[-3], encoding="utf-8")
+        if argv[1:3] == ["pane", "send-keys"] and argv[4] == "q":
+            pane_id = argv[3]
+            self.viewer_closing_panes.add(pane_id)
         if argv[1:3] == ["pane", "rename"] and self.fail_rename:
             raise subprocess.CalledProcessError(1, argv)
+        if argv[1:3] == ["pane", "rename"]:
+            pane_id, label = argv[3], argv[4]
+            pane = next(
+                (candidate for candidate in self.panes if candidate["pane_id"] == pane_id),
+                None,
+            )
+            if pane is None:
+                self.panes.append({"pane_id": pane_id, "label": label})
+            else:
+                pane["label"] = label
         if argv[1:3] == ["pane", "list"]:
-            output = {"result": {"panes": self.panes}}
+            pane_snapshot = [dict(pane) for pane in self.panes]
+            if self.list_delay_seconds:
+                time.sleep(self.list_delay_seconds)
+            output = {"result": {"panes": pane_snapshot}}
+        elif argv[1:3] == ["pane", "process-info"]:
+            pane_id = argv[4]
+            shell_name = self.shell_processes.get(pane_id, "fish")
+            teardown = self.viewer_teardown_processes.get(pane_id, [])
+            if pane_id in self.viewer_closing_panes and teardown:
+                process_names = teardown.pop(0)
+            elif pane_id in self.viewer_closing_panes:
+                process_names = [shell_name]
+                self.viewer_closing_panes.remove(pane_id)
+                self.foreground_processes[pane_id] = process_names
+            else:
+                process_names = self.foreground_processes.get(pane_id, [shell_name])
+            output = {
+                "result": {
+                    "process_info": {
+                        "pane_id": pane_id,
+                        "shell_pid": 100,
+                        "foreground_processes": [
+                            {
+                                "name": name,
+                                "pid": 100 if name == shell_name else index + 101,
+                            }
+                            for index, name in enumerate(process_names)
+                        ],
+                    }
+                }
+            }
         elif argv[1:3] == ["pane", "split"]:
             output = {"result": {"pane": {"pane_id": "workspace:p-new"}}}
         else:
@@ -54,7 +129,10 @@ class ArtifactPresenterTests(unittest.TestCase):
             "HERDR_WORKSPACE_ID": "workspace",
             "HERDR_PANE_ID": "workspace:p-source",
         }
-        self.executables = {"herdr": "/usr/bin/herdr", "mcat": "/usr/bin/mcat"}
+        self.executables = {
+            "herdr": "/usr/bin/herdr",
+            "mcat": "/usr/bin/mcat",
+        }
 
     def tearDown(self) -> None:
         self.tempdir.cleanup()
@@ -62,6 +140,25 @@ class ArtifactPresenterTests(unittest.TestCase):
     def which(self, name: str) -> str | None:
         return self.executables.get(name)
 
+    def mcat_run_call(
+        self,
+        runner: FakeRunner,
+        pane_id: str,
+        artifact: Path | None = None,
+    ) -> list[str]:
+        expected_artifact = str((artifact or self.artifact).resolve())
+        return next(
+            call
+            for call in runner.calls
+            if call[1:3] == ["pane", "send-text"]
+            and call[3] == pane_id
+            and [
+                base64.urlsafe_b64decode(value.encode("ascii")).decode("utf-8")
+                for value in call[4].strip().split()[-2:]
+            ]
+            == ["/usr/bin/mcat", expected_artifact]
+        )
+
     def test_reuses_named_pane(self) -> None:
         runner = FakeRunner(
             [{"pane_id": "workspace:p-viewer", "label": MODULE.PANE_LABEL}],
@@ -76,18 +173,183 @@ class ArtifactPresenterTests(unittest.TestCase):
 
         self.assertEqual(result.status, "reused")
         self.assertEqual(result.pane_id, "workspace:p-viewer")
+        self.mcat_run_call(runner, "workspace:p-viewer")
+        self.assertFalse(any(call[1:3] == ["pane", "split"] for call in runner.calls))
+
+    def test_closes_active_mcat_before_reusing_named_pane(self) -> None:
+        runner = FakeRunner(
+            [{"pane_id": "workspace:p-viewer", "label": MODULE.PANE_LABEL}],
+            foreground_processes={"workspace:p-viewer": ["mcat", "less"]},
+        )
+
+        result = MODULE.present_document(
+            self.artifact,
+            env=self.env,
+            which=self.which,
+            run=runner,
+        )
+
+        self.assertEqual(result.status, "reused")
+        close_viewer = [
+            "/usr/bin/herdr",
+            "pane",
+            "send-keys",
+            "workspace:p-viewer",
+            "q",
+        ]
+        launch_viewer = self.mcat_run_call(runner, "workspace:p-viewer")
+        self.assertIn(close_viewer, runner.calls)
+        self.assertLess(runner.calls.index(close_viewer), runner.calls.index(launch_viewer))
+
+    def test_waits_through_transient_viewer_teardown_process(self) -> None:
+        runner = FakeRunner(
+            [{"pane_id": "workspace:p-viewer", "label": MODULE.PANE_LABEL}],
+            foreground_processes={"workspace:p-viewer": ["mcat", "less"]},
+            viewer_teardown_processes={"workspace:p-viewer": [["less"]]},
+        )
+
+        result = MODULE.present_document(
+            self.artifact,
+            env=self.env,
+            which=self.which,
+            run=runner,
+        )
+
+        self.assertEqual(result.status, "reused")
+        process_checks = [
+            call for call in runner.calls if call[1:3] == ["pane", "process-info"]
+        ]
+        self.assertEqual(len(process_checks), 4)
+        self.mcat_run_call(runner, "workspace:p-viewer")
+
+    def test_requires_start_acknowledgment_after_launch(self) -> None:
+        runner = FakeRunner(
+            [{"pane_id": "workspace:p-viewer", "label": MODULE.PANE_LABEL}],
+        )
+
+        result = MODULE.present_document(
+            self.artifact,
+            env=self.env,
+            which=self.which,
+            run=runner,
+        )
+
+        self.assertEqual(result.status, "reused")
+        launch = self.mcat_run_call(runner, "workspace:p-viewer")
+        command = launch[4].strip().split()
+        decoded = [
+            base64.urlsafe_b64decode(value.encode("ascii")).decode("utf-8")
+            for value in command[-4:]
+        ]
+        acknowledgment = Path(decoded[-4])
+        self.assertEqual(Path(command[1]).name, "launch-viewer.py")
+        self.assertEqual(acknowledgment.name, "viewer-started")
+        self.assertEqual(len(decoded[-3]), 24)
+
+    def test_does_not_report_success_without_start_acknowledgment(self) -> None:
+        runner = FakeRunner(
+            [{"pane_id": "workspace:p-viewer", "label": MODULE.PANE_LABEL}],
+            write_start_acknowledgment=False,
+        )
+        original_attempts = MODULE.VIEWER_START_ATTEMPTS
+        original_delay = MODULE.PANE_READY_DELAY_SECONDS
+        MODULE.VIEWER_START_ATTEMPTS = 2
+        MODULE.PANE_READY_DELAY_SECONDS = 0
+        try:
+            result = MODULE.present_document(
+                self.artifact,
+                env=self.env,
+                which=self.which,
+                run=runner,
+            )
+        finally:
+            MODULE.VIEWER_START_ATTEMPTS = original_attempts
+            MODULE.PANE_READY_DELAY_SECONDS = original_delay
+
+        self.assertEqual(result.status, "artifact-only")
+        self.assertIn("could not initialize Herdr viewer", result.reason)
+
+    def test_accepts_idle_shell_by_pid_without_shell_name_allowlist(self) -> None:
+        runner = FakeRunner(
+            [{"pane_id": "workspace:p-viewer", "label": MODULE.PANE_LABEL}],
+            foreground_processes={"workspace:p-viewer": ["xonsh"]},
+            shell_processes={"workspace:p-viewer": "xonsh"},
+        )
+
+        result = MODULE.present_document(
+            self.artifact,
+            env=self.env,
+            which=self.which,
+            run=runner,
+        )
+
+        self.assertEqual(result.status, "reused")
+        self.assertFalse(
+            any(
+                call[1:3] == ["pane", "send-keys"] and call[4] == "q"
+                for call in runner.calls
+            )
+        )
+
+    def test_empty_foreground_state_is_not_treated_as_ready(self) -> None:
+        runner = FakeRunner(
+            [{"pane_id": "workspace:p-transitioning", "label": MODULE.PANE_LABEL}],
+            foreground_processes={"workspace:p-transitioning": []},
+        )
+        original_attempts = MODULE.PANE_READY_ATTEMPTS
+        original_delay = MODULE.PANE_READY_DELAY_SECONDS
+        MODULE.PANE_READY_ATTEMPTS = 2
+        MODULE.PANE_READY_DELAY_SECONDS = 0
+        try:
+            result = MODULE.present_document(
+                self.artifact,
+                env=self.env,
+                which=self.which,
+                run=runner,
+            )
+        finally:
+            MODULE.PANE_READY_ATTEMPTS = original_attempts
+            MODULE.PANE_READY_DELAY_SECONDS = original_delay
+
+        self.assertEqual(result.status, "opened")
+        process_checks = [
+            call
+            for call in runner.calls
+            if call[1:3] == ["pane", "process-info"]
+            and call[4] == "workspace:p-transitioning"
+        ]
+        self.assertEqual(len(process_checks), 2)
+
+    def test_does_not_interrupt_unrelated_foreground_process(self) -> None:
+        runner = FakeRunner(
+            [{"pane_id": "workspace:p-busy", "label": MODULE.PANE_LABEL}],
+            foreground_processes={"workspace:p-busy": ["vim"]},
+        )
+
+        result = MODULE.present_document(
+            self.artifact,
+            env=self.env,
+            which=self.which,
+            run=runner,
+        )
+
+        self.assertEqual(result.status, "opened")
+        self.assertFalse(
+            any(
+                call[1:3] == ["pane", "send-keys"] and call[4] == "q"
+                for call in runner.calls
+            )
+        )
         self.assertIn(
             [
                 "/usr/bin/herdr",
                 "pane",
-                "run",
-                "workspace:p-viewer",
-                "/usr/bin/mcat",
-                str(self.artifact.resolve()),
+                "rename",
+                "workspace:p-busy",
+                f"{MODULE.PANE_LABEL} stale",
             ],
             runner.calls,
         )
-        self.assertFalse(any(call[1:3] == ["pane", "split"] for call in runner.calls))
 
     def test_tries_each_named_pane_before_creating_another(self) -> None:
         runner = FakeRunner(
@@ -134,18 +396,28 @@ class ArtifactPresenterTests(unittest.TestCase):
         )
 
         self.assertEqual(result.status, "reused")
-        self.assertIn(
-            [
-                "/usr/bin/herdr",
-                "pane",
-                "run",
-                "workspace:p-viewer",
-                "/usr/bin/mcat",
-                str(video.resolve()),
-            ],
-            runner.calls,
+        self.mcat_run_call(runner, "workspace:p-viewer", video)
+
+    def test_encodes_artifact_paths_before_terminal_delivery(self) -> None:
+        artifact = self.artifact.with_name("report '$(touch unsafe)'.md")
+        artifact.write_text("# Safe\n", encoding="utf-8")
+        runner = FakeRunner(
+            [{"pane_id": "workspace:p-viewer", "label": MODULE.PANE_LABEL}],
         )
 
+        result = MODULE.present_document(
+            artifact,
+            env=self.env,
+            which=self.which,
+            run=runner,
+        )
+
+        self.assertEqual(result.status, "reused")
+        launch = self.mcat_run_call(runner, "workspace:p-viewer", artifact)
+        command_parts = launch[4].strip().split()
+        self.assertTrue(all(MODULE.SAFE_COMMAND_TOKEN.fullmatch(part) for part in command_parts))
+        self.assertNotIn("$(touch unsafe)", launch[4])
+
     def test_creates_right_pane_when_viewer_is_absent(self) -> None:
         runner = FakeRunner()
 
@@ -214,6 +486,74 @@ class ArtifactPresenterTests(unittest.TestCase):
         self.assertEqual(result.status, "artifact-only")
         self.assertEqual(result.reason, "mcat is unavailable")
 
+    def test_pane_lock_serializes_concurrent_presenters(self) -> None:
+        first_acquired = threading.Event()
+        release_first = threading.Event()
+        second_acquired = threading.Event()
+        original_runtime = os.environ.get("XDG_RUNTIME_DIR")
+        os.environ["XDG_RUNTIME_DIR"] = self.tempdir.name
+
+        def first_presenter() -> None:
+            with MODULE.lock_pane("workspace:p-viewer"):
+                first_acquired.set()
+                release_first.wait(timeout=1)
+
+        def second_presenter() -> None:
+            first_acquired.wait(timeout=1)
+            with MODULE.lock_pane("workspace:p-viewer"):
+                second_acquired.set()
+
+        first = threading.Thread(target=first_presenter)
+        second = threading.Thread(target=second_presenter)
+        try:
+            first.start()
+            second.start()
+            self.assertTrue(first_acquired.wait(timeout=1))
+            self.assertFalse(second_acquired.wait(timeout=0.05))
+            release_first.set()
+            self.assertTrue(second_acquired.wait(timeout=1))
+        finally:
+            release_first.set()
+            first.join(timeout=1)
+            second.join(timeout=1)
+            if original_runtime is None:
+                os.environ.pop("XDG_RUNTIME_DIR", None)
+            else:
+                os.environ["XDG_RUNTIME_DIR"] = original_runtime
+
+    def test_workspace_lock_prevents_duplicate_viewer_creation(self) -> None:
+        runner = FakeRunner(list_delay_seconds=0.05)
+        results: list[MODULE.PresentationResult] = []
+        results_lock = threading.Lock()
+        original_runtime = os.environ.get("XDG_RUNTIME_DIR")
+        os.environ["XDG_RUNTIME_DIR"] = self.tempdir.name
+
+        def present() -> None:
+            result = MODULE.present_document(
+                self.artifact,
+                env=self.env,
+                which=self.which,
+                run=runner,
+            )
+            with results_lock:
+                results.append(result)
+
+        threads = [threading.Thread(target=present) for _ in range(2)]
+        try:
+            for thread in threads:
+                thread.start()
+            for thread in threads:
+                thread.join(timeout=2)
+        finally:
+            if original_runtime is None:
+                os.environ.pop("XDG_RUNTIME_DIR", None)
+            else:
+                os.environ["XDG_RUNTIME_DIR"] = original_runtime
+
+        self.assertEqual(sorted(result.status for result in results), ["opened", "reused"])
+        splits = [call for call in runner.calls if call[1:3] == ["pane", "split"]]
+        self.assertEqual(len(splits), 1)
+
     def test_rejects_missing_artifact(self) -> None:
         with self.assertRaisesRegex(ValueError, "artifact is not a file"):
             MODULE.present_document(

skills/document/artifact-presenter/tests/test_present_integration.py

@@ -0,0 +1,116 @@
+from __future__ import annotations
+
+import importlib.util
+import os
+import shutil
+import subprocess
+import sys
+import tempfile
+import time
+import unittest
+from pathlib import Path
+
+MODULE_PATH = Path(__file__).parents[1] / "scripts" / "present.py"
+SPEC = importlib.util.spec_from_file_location("artifact_presenter_integration", MODULE_PATH)
+assert SPEC and SPEC.loader
+MODULE = importlib.util.module_from_spec(SPEC)
+sys.modules[SPEC.name] = MODULE
+SPEC.loader.exec_module(MODULE)
+
+
+@unittest.skipUnless(
+    os.environ.get("HERDR_INTEGRATION_TESTS") == "1",
+    "set HERDR_INTEGRATION_TESTS=1 inside a Herdr pane",
+)
+class ArtifactPresenterIntegrationTests(unittest.TestCase):
+    def wait_for_mcat(self, herdr: str, pane_id: str, artifact: Path) -> None:
+        for _attempt in range(100):
+            state = MODULE.run_json(
+                herdr,
+                ["pane", "process-info", "--pane", pane_id],
+                subprocess.run,
+            )
+            processes = state["result"]["process_info"]["foreground_processes"]
+            if any(
+                process.get("name") == "mcat"
+                and str(artifact) in process.get("argv", [])
+                for process in processes
+            ):
+                return
+            time.sleep(0.05)
+        self.fail(f"mcat did not open {artifact}")
+
+    def test_replaces_active_mcat_in_real_herdr_pane(self) -> None:
+        herdr = shutil.which("herdr")
+        mcat = shutil.which("mcat")
+        source_pane = os.environ.get("HERDR_PANE_ID")
+        if not all((herdr, mcat, source_pane)):
+            self.skipTest("Herdr, mcat, and HERDR_PANE_ID are required")
+
+        with tempfile.TemporaryDirectory() as directory:
+            root = Path(directory)
+            first = root / "first.md"
+            second = root / "second.md"
+            first.write_text(
+                "# First artifact\n" + ("first viewer line " * 20 + "\n") * 1_000,
+                encoding="utf-8",
+            )
+            second.write_text(
+                "# Second artifact\n" + ("second viewer line " * 20 + "\n") * 1_000,
+                encoding="utf-8",
+            )
+
+            split = MODULE.run_json(
+                herdr,
+                [
+                    "pane",
+                    "split",
+                    "--pane",
+                    source_pane,
+                    "--direction",
+                    "right",
+                    "--cwd",
+                    directory,
+                    "--env",
+                    "PAGER=less",
+                    "--no-focus",
+                ],
+                subprocess.run,
+            )
+            pane_id = MODULE.created_pane_id(split)
+            try:
+                MODULE.run_mcat(
+                    herdr,
+                    mcat,
+                    pane_id,
+                    first,
+                    subprocess.run,
+                )
+                self.wait_for_mcat(herdr, pane_id, first)
+
+                MODULE.run_mcat(
+                    herdr,
+                    mcat,
+                    pane_id,
+                    second,
+                    subprocess.run,
+                    reused=True,
+                )
+                self.wait_for_mcat(herdr, pane_id, second)
+            finally:
+                subprocess.run(
+                    [herdr, "pane", "send-keys", pane_id, "q"],
+                    check=False,
+                    capture_output=True,
+                    text=True,
+                )
+                subprocess.run(
+                    [herdr, "pane", "close", pane_id],
+                    check=False,
+                    capture_output=True,
+                    text=True,
+                )
+
+
+if __name__ == "__main__":
+    unittest.main()