diff --git a/docs/global.AGENTS.md b/docs/global.AGENTS.md index b86ca82..8ad3b20 100644 --- a/docs/global.AGENTS.md +++ b/docs/global.AGENTS.md @@ -93,6 +93,7 @@ Below you will find my preferences so that we can be more aligned as we work tog - Plan before we implement. Investigate the codebase, do research, and grill me with questions. I'd rather spend some time answering questions first and get a result I am happy with rather than something we'd have to do again. - Prototype or create PoCs for large features first. If we can identify if something will or will not work early, that will save time in the long run. - Do not touch production or live databases unless explicitly instructed to do so. +- When presenting any single local file for my review, or a structured long-form report, plan, ADR, review, or summary, load the [`artifact-presenter`](../skills/document/artifact-presenter/SKILL.md) skill and open the complete file with mcat in its reusable Herdr pane instead of returning the full artifact inline. ## Subagents and Parallel work diff --git a/package.json b/package.json index d1eea8d..00d271f 100644 --- a/package.json +++ b/package.json @@ -13,8 +13,9 @@ "scripts": { "prepare": "effect-tsgo patch", "typecheck": "tsc --noEmit", - "test": "npm run test:runner && npm run test:dictate-service && npm run test:links && node --test --experimental-strip-types extensions/dictate/index.test.ts extensions/firecrawl-search/index.test.ts extensions/shared/*.test.ts extensions/subagents/by-the-way.test.ts extensions/subagents/context-usage.test.ts extensions/subagents/manager.test.ts extensions/subagents/result-delivery.test.ts extensions/subagents/takeover.test.ts extensions/_workflows/*.test.ts extensions/pi-workflow/*.test.ts && vitest run extensions/file-search/index.spec.ts", + "test": "npm run test:runner && npm run test:artifact-presenter && npm run test:dictate-service && npm run test:links && node --test --experimental-strip-types extensions/dictate/index.test.ts extensions/firecrawl-search/index.test.ts extensions/shared/*.test.ts extensions/subagents/by-the-way.test.ts extensions/subagents/context-usage.test.ts extensions/subagents/manager.test.ts extensions/subagents/result-delivery.test.ts extensions/subagents/takeover.test.ts extensions/_workflows/*.test.ts extensions/pi-workflow/*.test.ts && vitest run extensions/file-search/index.spec.ts", "test:runner": "python -m unittest discover -s scripts -p 'test_*.py'", + "test:artifact-presenter": "python -m unittest discover -s skills/document/artifact-presenter/tests -p 'test_*.py'", "test:dictate-service": "PYTHONPATH=services/dictate/src python -m unittest discover -s services/dictate/tests -p 'test_*.py'", "test:links": "node --test scripts/pi-links.test.mjs", "test:live": "node --test --experimental-strip-types extensions/subagents/claude.test.ts extensions/subagents/codex.test.ts" diff --git a/skills/document/artifact-presenter/SKILL.md b/skills/document/artifact-presenter/SKILL.md new file mode 100644 index 0000000..370ec86 --- /dev/null +++ b/skills/document/artifact-presenter/SKILL.md @@ -0,0 +1,70 @@ +--- +name: artifact-presenter +description: Present any single local file the user needs to review, including documents, source, archives, PDFs, images, and videos, through mcat in a reusable Herdr pane. Also use for structured long-form reports, plans, ADRs, reviews, and summaries unless the user requests inline chat. +--- + +# Artifact presenter + +Present reviewable files and substantial generated content in a reusable terminal document pane instead of duplicating the complete artifact in chat. + +## Choose the artifact + +Use this workflow in either case: + +- The user needs to review one local file, regardless of its length or format. +- The expected deliverable is a structured report, plan, ADR, review, or summary with several substantive sections or roughly 1,000 words. + +Treat an explicit destination and format as authoritative. For generated content without a requested destination: + +- Use `/tmp/pi-present/.`. +- Default structured prose to Markdown when no other format is implied. +- Create `/tmp/pi-present/` when needed. +- Add a timestamp only when replacing an earlier artifact would lose useful information. + +Keep ordinary explanations, code diffs, logs, multi-file comparisons, and user-requested inline responses in chat. Present a file only when it is a user-facing review artifact, not merely because the agent read it during investigation. + +Completion criterion: select exactly one existing or generated artifact path before presentation. + +## Prepare generated content + +Write the complete deliverable to the selected path. Keep it self-contained: the reader must not need omitted chat text to understand it. + +Preserve repository documentation conventions. Creating a presentation artifact under `/tmp` does not authorize editing repository files when the request is otherwise read-only. + +When presenting an existing file, pass the original file directly to mcat. Do not create a converted copy merely for display. + +Completion criterion: the selected file exists and contains the complete review artifact. + +## Present with mcat + +Run the helper using its absolute path resolved from this skill directory: + +```bash +python3 /scripts/present.py +``` + +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 ` 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. + +Completion criterion: capture whether the helper opened, reused, or skipped the viewer pane. + +## Convert only when requested + +Use mcat's conversion pipeline when the user requests a derivative artifact: + +```bash +mcat --output md > +mcat --output html > +mcat --output image > +``` + +Use an explicit output path and preserve the source file. Present the resulting single file through the helper when the user needs to review it. + +## Respond concisely + +Return: + +- a one- to three-sentence result summary; +- the artifact path; +- whether the reusable mcat pane was opened or reused, or why presentation fell back to the file only. + +Do not repeat the complete artifact in chat. Honor a later request to paste any section inline. diff --git a/skills/document/artifact-presenter/scripts/present.py b/skills/document/artifact-presenter/scripts/present.py new file mode 100755 index 0000000..695e2e6 --- /dev/null +++ b/skills/document/artifact-presenter/scripts/present.py @@ -0,0 +1,195 @@ +#!/usr/bin/env python3 +"""Open one review artifact in a reusable right-side Herdr mcat pane.""" + +from __future__ import annotations + +import argparse +import json +import os +import shutil +import subprocess +from dataclasses import dataclass +from pathlib import Path +from typing import Callable, Mapping, Sequence + +PANE_LABEL = "artifact presenter" + +RunCommand = Callable[..., subprocess.CompletedProcess[str]] +FindExecutable = Callable[[str], str | None] + + +@dataclass(frozen=True) +class PresentationResult: + status: str + artifact: Path + pane_id: str | None = None + reason: str | None = None + + def message(self) -> str: + if self.status == "artifact-only": + return f"artifact-only: {self.reason}; {self.artifact}" + return f"{self.status}: pane {self.pane_id}; {self.artifact}" + + +def run_json( + executable: str, + args: Sequence[str], + run: RunCommand, +) -> dict[str, object]: + completed = run( + [executable, *args], + check=True, + capture_output=True, + text=True, + ) + value = json.loads(completed.stdout) + if not isinstance(value, dict): + raise ValueError("Herdr returned a non-object response") + return value + + +def panes_with_label(response: Mapping[str, object]) -> list[str]: + result = response.get("result") + if not isinstance(result, dict): + return [] + panes = result.get("panes") + if not isinstance(panes, list): + return [] + pane_ids: list[str] = [] + for pane in panes: + if not isinstance(pane, dict) or pane.get("label") != PANE_LABEL: + continue + pane_id = pane.get("pane_id") + if isinstance(pane_id, str) and pane_id: + pane_ids.append(pane_id) + return pane_ids + + +def created_pane_id(response: Mapping[str, object]) -> str: + result = response.get("result") + pane = result.get("pane") if isinstance(result, dict) else None + pane_id = pane.get("pane_id") if isinstance(pane, dict) else None + if not isinstance(pane_id, str) or not pane_id: + raise ValueError("Herdr split response is missing result.pane.pane_id") + return pane_id + + +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, +) -> 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) + except (json.JSONDecodeError, OSError, subprocess.SubprocessError, ValueError) as error: + return PresentationResult("artifact-only", artifact, reason=f"could not list Herdr panes: {error}") + + for existing_pane in existing_panes: + try: + run_mcat(herdr, mcat, existing_pane, artifact, run) + return PresentationResult("reused", artifact, pane_id=existing_pane) + except (OSError, subprocess.SubprocessError): + try: + run( + [herdr, "pane", "rename", existing_pane, f"{PANE_LABEL} stale"], + check=False, + capture_output=True, + text=True, + ) + except OSError: + pass + + try: + split = run_json( + herdr, + [ + "pane", + "split", + "--pane", + source_pane_id, + "--direction", + "right", + "--cwd", + str(artifact.parent), + "--no-focus", + ], + run, + ) + pane_id = created_pane_id(split) + except (json.JSONDecodeError, OSError, subprocess.SubprocessError, ValueError) as error: + return PresentationResult("artifact-only", artifact, reason=f"could not create Herdr viewer: {error}") + + try: + run( + [herdr, "pane", "rename", pane_id, PANE_LABEL], + check=True, + capture_output=True, + text=True, + ) + run_mcat(herdr, mcat, pane_id, artifact, run) + return PresentationResult("opened", artifact, pane_id=pane_id) + except (OSError, subprocess.SubprocessError) as error: + try: + run( + [herdr, "pane", "close", pane_id], + check=False, + capture_output=True, + text=True, + ) + except OSError: + pass + return PresentationResult("artifact-only", artifact, reason=f"could not initialize Herdr viewer: {error}") + + +def create_parser() -> argparse.ArgumentParser: + parser = argparse.ArgumentParser(description=__doc__) + parser.add_argument("artifact", type=Path, help="Local file to present with mcat") + return parser + + +def main() -> int: + args = create_parser().parse_args() + try: + result = present_document(args.artifact) + except ValueError as error: + print(error, file=os.sys.stderr) + return 2 + print(result.message()) + return 0 + + +if __name__ == "__main__": + raise SystemExit(main()) diff --git a/skills/document/artifact-presenter/tests/test_present.py b/skills/document/artifact-presenter/tests/test_present.py new file mode 100644 index 0000000..aa29d93 --- /dev/null +++ b/skills/document/artifact-presenter/tests/test_present.py @@ -0,0 +1,228 @@ +from __future__ import annotations + +import importlib.util +import json +import subprocess +import sys +import tempfile +import unittest +from pathlib import Path + +MODULE_PATH = Path(__file__).parents[1] / "scripts" / "present.py" +SPEC = importlib.util.spec_from_file_location("artifact_presenter", MODULE_PATH) +assert SPEC and SPEC.loader +MODULE = importlib.util.module_from_spec(SPEC) +sys.modules[SPEC.name] = MODULE +SPEC.loader.exec_module(MODULE) + + +class FakeRunner: + def __init__( + self, + panes: list[dict[str, str]] | None = None, + *, + failed_run_panes: set[str] | None = None, + fail_rename: bool = False, + ) -> None: + self.panes = panes or [] + self.failed_run_panes = failed_run_panes or set() + self.fail_rename = fail_rename + 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", "rename"] and self.fail_rename: + raise subprocess.CalledProcessError(1, argv) + if argv[1:3] == ["pane", "list"]: + output = {"result": {"panes": self.panes}} + elif argv[1:3] == ["pane", "split"]: + output = {"result": {"pane": {"pane_id": "workspace:p-new"}}} + else: + output = {} + return subprocess.CompletedProcess(argv, 0, json.dumps(output), "") + + +class ArtifactPresenterTests(unittest.TestCase): + def setUp(self) -> None: + self.tempdir = tempfile.TemporaryDirectory() + self.artifact = Path(self.tempdir.name) / "report.md" + self.artifact.write_text("# Report\n", encoding="utf-8") + self.env = { + "HERDR_WORKSPACE_ID": "workspace", + "HERDR_PANE_ID": "workspace:p-source", + } + self.executables = {"herdr": "/usr/bin/herdr", "mcat": "/usr/bin/mcat"} + + def tearDown(self) -> None: + self.tempdir.cleanup() + + def which(self, name: str) -> str | None: + return self.executables.get(name) + + def test_reuses_named_pane(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") + self.assertEqual(result.pane_id, "workspace:p-viewer") + self.assertIn( + [ + "/usr/bin/herdr", + "pane", + "run", + "workspace:p-viewer", + "/usr/bin/mcat", + str(self.artifact.resolve()), + ], + 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( + [ + {"pane_id": "workspace:p-stale", "label": MODULE.PANE_LABEL}, + {"pane_id": "workspace:p-viewer", "label": MODULE.PANE_LABEL}, + ], + failed_run_panes={"workspace:p-stale"}, + ) + + result = MODULE.present_document( + self.artifact, + env=self.env, + which=self.which, + run=runner, + ) + + self.assertEqual(result.status, "reused") + self.assertEqual(result.pane_id, "workspace:p-viewer") + self.assertIn( + [ + "/usr/bin/herdr", + "pane", + "rename", + "workspace:p-stale", + f"{MODULE.PANE_LABEL} stale", + ], + runner.calls, + ) + self.assertFalse(any(call[1:3] == ["pane", "split"] for call in runner.calls)) + + def test_presents_non_markdown_file_directly_with_mcat(self) -> None: + video = self.artifact.with_name("review.mp4") + video.write_bytes(b"video fixture") + runner = FakeRunner( + [{"pane_id": "workspace:p-viewer", "label": MODULE.PANE_LABEL}], + ) + + result = MODULE.present_document( + video, + env=self.env, + which=self.which, + run=runner, + ) + + self.assertEqual(result.status, "reused") + self.assertIn( + [ + "/usr/bin/herdr", + "pane", + "run", + "workspace:p-viewer", + "/usr/bin/mcat", + str(video.resolve()), + ], + runner.calls, + ) + + def test_creates_right_pane_when_viewer_is_absent(self) -> None: + runner = FakeRunner() + + result = MODULE.present_document( + self.artifact, + env=self.env, + which=self.which, + run=runner, + ) + + self.assertEqual(result.status, "opened") + split = next(call for call in runner.calls if call[1:3] == ["pane", "split"]) + self.assertEqual( + split, + [ + "/usr/bin/herdr", + "pane", + "split", + "--pane", + "workspace:p-source", + "--direction", + "right", + "--cwd", + str(self.artifact.parent.resolve()), + "--no-focus", + ], + ) + self.assertIn( + [ + "/usr/bin/herdr", + "pane", + "rename", + "workspace:p-new", + MODULE.PANE_LABEL, + ], + runner.calls, + ) + + def test_closes_new_pane_when_rename_fails(self) -> None: + runner = FakeRunner(fail_rename=True) + + result = MODULE.present_document( + self.artifact, + env=self.env, + which=self.which, + run=runner, + ) + + self.assertEqual(result.status, "artifact-only") + self.assertIn("could not initialize Herdr viewer", result.reason) + self.assertIn( + ["/usr/bin/herdr", "pane", "close", "workspace:p-new"], + runner.calls, + ) + + def test_falls_back_to_artifact_when_mcat_is_unavailable(self) -> None: + self.executables.pop("mcat") + + result = MODULE.present_document( + self.artifact, + env=self.env, + which=self.which, + run=FakeRunner(), + ) + + self.assertEqual(result.status, "artifact-only") + self.assertEqual(result.reason, "mcat is unavailable") + + def test_rejects_missing_artifact(self) -> None: + with self.assertRaisesRegex(ValueError, "artifact is not a file"): + MODULE.present_document( + self.artifact.with_name("missing.md"), + env=self.env, + which=self.which, + run=FakeRunner(), + ) + + +if __name__ == "__main__": + unittest.main()