From a8841361e1f201d904fcd4b374ea716f5dcf35d3 Mon Sep 17 00:00:00 2001 From: onevcat Date: Wed, 26 Aug 2026 22:02:31 +0900 Subject: [PATCH] fix(agents): classify Pi-family sub-agents statelessly and harden the OpenCode launch inputs MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Review of #728 found three real gaps, each pinned by a test first: - The Pi/Oh My Pi extensions recognised sub-agents by the `_.jsonl` session file name, which also matched a session started with a custom `--session-id` and silently dropped every event of it. A stateful replacement ("the first announcer is the pane's session") passed its harness and failed live: the runtime loads a fresh extension instance for every sub-agent session, so module state never reaches the sub-agent's handlers. The rule is now stateless and structural — a sub-agent's session file is nested inside the parent's session directory, so a session whose file has a session-directory ancestor is a sub-agent and that directory names the pane's session; the id itself is never interpreted. `scripts/test_agent_hooks.py` runs the real extensions through Node against a capture CLI and is part of `make check`; the fix is re-verified live for an OMP sub-agent turn and a Pi session with a custom id. - OpenCode's TUI `--replay-limit ` was missing from the value-option table, so `7` would have been registered as the project directory and every hook rejected on the cwd guard without a warning. The table now matches the 1.18.23 `--help`, and because OpenCode refuses to start in a directory that does not exist, a positional that is not an existing directory is treated as an unknown option's value and the launch directory stays inherited. - `ShellEnvironmentProbe` declared a 256 KiB output bound but ran `CodexShellProbeProcess` at its 16 KiB default, degrading a launch whose exported `OPENCODE_CONFIG_CONTENT` was a few pages long. The production runner now carries the probe's bound (and accepts an injected environment so the test never mutates the host's), covered at the process level and end to end. Claude-Session: https://claude.ai/code/session_016P9EWPCSZBP3HoaEqvBdUL --- Resources/agent-hooks/omp/prowl-hooks.ts | 37 ++- Resources/agent-hooks/pi/prowl-hooks.ts | 37 ++- .../010-s3c-plan.md | 17 +- .../011-s3c-action.md | 32 +- .../research-agent-completion-signals.md | 2 +- docs/components/agent-detection.md | 5 +- scripts/test_agent_hooks.py | 277 ++++++++++++++++++ .../AgentManagedHookPreparer.swift | 11 +- .../AgentRuntime/CodexShellProbeProcess.swift | 11 +- .../AgentRuntime/ManagedHookRendering.swift | 8 +- .../AgentRuntime/ShellEnvironmentProbe.swift | 20 +- .../AgentS3cHookRenderingTests.swift | 45 ++- .../CodexShellProbeProcessTests.swift | 34 +++ 13 files changed, 483 insertions(+), 53 deletions(-) create mode 100644 scripts/test_agent_hooks.py diff --git a/Resources/agent-hooks/omp/prowl-hooks.ts b/Resources/agent-hooks/omp/prowl-hooks.ts index bb827950..000a64e8 100644 --- a/Resources/agent-hooks/omp/prowl-hooks.ts +++ b/Resources/agent-hooks/omp/prowl-hooks.ts @@ -21,18 +21,26 @@ const FORWARDED_EVENTS = [ const TOKEN_VARIABLE = "PROWL_AGENT_HOOK_TOKEN"; const CLI = join(dirname(fileURLToPath(import.meta.url)), "..", "..", "prowl-cli", "prowl"); -// A main session's file is `/_.jsonl`; an in-process sub-agent (Oh My -// Pi's `task` tool) runs under its own session id whose file is nested inside the parent's -// session directory and named after the agent (`PongResponder.jsonl`). Its lifecycle events must -// not rotate the pane's session, but an approval it asks for still blocks the user. -const MAIN_SESSION_FILE = /^\d{4}-\d{2}-\d{2}T\d{2}-\d{2}-\d{2}-\d{3}Z_[0-9a-f-]{36}\.jsonl$/i; -let mainSessionId: string | undefined; +// Session files live directly in the session directory as `_.jsonl`, where the id +// is opaque (`--session-id` accepts any name). An in-process sub-agent (Oh My Pi's `task` tool) +// runs under its own session id and stores its file *inside* the parent's session directory +// (`_/.jsonl`, nested again for a sub-agent's sub-agent). The runtime +// loads a fresh extension instance for each of those sessions, so the classification must be +// stateless: a session is a sub-agent when any ancestor directory of its file is a session +// directory, and the pane's session id is that directory's id. Sub-agent lifecycle events must +// not rotate the pane's session, while an approval a sub-agent asks for still blocks the user +// and is reported under the pane's session. +const SESSION_DIRECTORY = /^\d{4}-\d{2}-\d{2}T\d{2}-\d{2}-\d{2}-\d{3}Z_(.+)$/; -function isSubAgentSession(ctx: any): boolean { +function parentSessionId(ctx: any): string | undefined { const file = ctx?.sessionManager?.getSessionFile?.(); - if (typeof file !== "string" || file.length === 0) return false; - const name = file.slice(file.lastIndexOf("/") + 1); - return !MAIN_SESSION_FILE.test(name); + if (typeof file !== "string" || file.length === 0) return undefined; + const components = file.split("/"); + for (const directory of components.slice(0, -1)) { + const match = SESSION_DIRECTORY.exec(directory); + if (match) return match[1]; + } + return undefined; } function relay(name: string, event: any, ctx: any): void { @@ -40,11 +48,10 @@ function relay(name: string, event: any, ctx: any): void { if (!process.env[TOKEN_VARIABLE]) return; let sessionId = ctx?.sessionManager?.getSessionId?.(); if (typeof sessionId !== "string" || sessionId.length === 0) return; - if (isSubAgentSession(ctx)) { - if (name !== "tool_approval_requested" || !mainSessionId) return; - sessionId = mainSessionId; - } else if (name === "session_start" || name === "session_switch") { - mainSessionId = sessionId; + const parent = parentSessionId(ctx); + if (parent !== undefined) { + if (name !== "tool_approval_requested") return; + sessionId = parent; } const reason = typeof event?.reason === "string" ? event.reason : event?.toolName; const payload = { diff --git a/Resources/agent-hooks/pi/prowl-hooks.ts b/Resources/agent-hooks/pi/prowl-hooks.ts index 7d6552c9..f907c6a6 100644 --- a/Resources/agent-hooks/pi/prowl-hooks.ts +++ b/Resources/agent-hooks/pi/prowl-hooks.ts @@ -14,18 +14,26 @@ const FORWARDED_EVENTS = ["session_start", "agent_settled", "session_shutdown"]; const TOKEN_VARIABLE = "PROWL_AGENT_HOOK_TOKEN"; const CLI = join(dirname(fileURLToPath(import.meta.url)), "..", "..", "prowl-cli", "prowl"); -// A main session's file is `/_.jsonl`; an in-process sub-agent (Oh My -// Pi's `task` tool) runs under its own session id whose file is nested inside the parent's -// session directory and named after the agent (`PongResponder.jsonl`). Its lifecycle events must -// not rotate the pane's session, but an approval it asks for still blocks the user. -const MAIN_SESSION_FILE = /^\d{4}-\d{2}-\d{2}T\d{2}-\d{2}-\d{2}-\d{3}Z_[0-9a-f-]{36}\.jsonl$/i; -let mainSessionId: string | undefined; +// Session files live directly in the session directory as `_.jsonl`, where the id +// is opaque (`--session-id` accepts any name). An in-process sub-agent (Oh My Pi's `task` tool) +// runs under its own session id and stores its file *inside* the parent's session directory +// (`_/.jsonl`, nested again for a sub-agent's sub-agent). The runtime +// loads a fresh extension instance for each of those sessions, so the classification must be +// stateless: a session is a sub-agent when any ancestor directory of its file is a session +// directory, and the pane's session id is that directory's id. Sub-agent lifecycle events must +// not rotate the pane's session, while an approval a sub-agent asks for still blocks the user +// and is reported under the pane's session. +const SESSION_DIRECTORY = /^\d{4}-\d{2}-\d{2}T\d{2}-\d{2}-\d{2}-\d{3}Z_(.+)$/; -function isSubAgentSession(ctx: any): boolean { +function parentSessionId(ctx: any): string | undefined { const file = ctx?.sessionManager?.getSessionFile?.(); - if (typeof file !== "string" || file.length === 0) return false; - const name = file.slice(file.lastIndexOf("/") + 1); - return !MAIN_SESSION_FILE.test(name); + if (typeof file !== "string" || file.length === 0) return undefined; + const components = file.split("/"); + for (const directory of components.slice(0, -1)) { + const match = SESSION_DIRECTORY.exec(directory); + if (match) return match[1]; + } + return undefined; } function relay(name: string, event: any, ctx: any): void { @@ -33,11 +41,10 @@ function relay(name: string, event: any, ctx: any): void { if (!process.env[TOKEN_VARIABLE]) return; let sessionId = ctx?.sessionManager?.getSessionId?.(); if (typeof sessionId !== "string" || sessionId.length === 0) return; - if (isSubAgentSession(ctx)) { - if (name !== "tool_approval_requested" || !mainSessionId) return; - sessionId = mainSessionId; - } else if (name === "session_start" || name === "session_switch") { - mainSessionId = sessionId; + const parent = parentSessionId(ctx); + if (parent !== undefined) { + if (name !== "tool_approval_requested") return; + sessionId = parent; } const payload = { hook_event_name: name, diff --git a/docs-ai/064-agent-completion-signals/010-s3c-plan.md b/docs-ai/064-agent-completion-signals/010-s3c-plan.md index 0ed731da..d1d1d11a 100644 --- a/docs-ai/064-agent-completion-signals/010-s3c-plan.md +++ b/docs-ai/064-agent-completion-signals/010-s3c-plan.md @@ -129,10 +129,19 @@ id, so it is the announcing edge the store's rotation rule needs. Pi's `session_start{reason:"reload"}` re-announces the current id, which is idempotent; `/resume` re-announces a retired id, which S3b's resume rule reactivates. -Sub-agent protection for the Pi family lives in the extension: a session whose file name is not -`_.jsonl` (i.e. nested under a session directory) is a sub-agent. Its -`session_start` / `session_shutdown` are dropped; its `tool_approval_requested` — which still -blocks the user — is forwarded under the main session id the extension last saw announced. +Sub-agent protection for the Pi family lives in the extension and must be **stateless**: the +runtime loads a fresh extension instance for every sub-agent session (measured — distinct +module instances sharing one `globalThis`), so nothing learned from the main session's events is +visible where a sub-agent's events arrive. The structural fact that survives this is the file +layout: a main session file sits directly in its session directory as `_.jsonl` +(the id is opaque — `--session-id` accepts any name), while a sub-agent's file is nested inside +the parent's session directory (`_/.jsonl`, deeper again for a +sub-agent's sub-agent). A session whose file has a session-directory ancestor is a sub-agent, +and that directory names the pane's session id. A sub-agent's `session_start` / +`session_shutdown` are dropped; its `tool_approval_requested` — which still blocks the user — +is forwarded under the pane's session id. A session without a file (ephemeral) is treated as the +pane's. `scripts/test_agent_hooks.py` drives the real extensions through Node against a capture +CLI to pin these decisions. Sub-agent protection for OpenCode (two layers, both required — measured above): diff --git a/docs-ai/064-agent-completion-signals/011-s3c-action.md b/docs-ai/064-agent-completion-signals/011-s3c-action.md index 272749c9..26a24c78 100644 --- a/docs-ai/064-agent-completion-signals/011-s3c-action.md +++ b/docs-ai/064-agent-completion-signals/011-s3c-action.md @@ -94,12 +94,34 @@ matches. In fact each in-process `task` sub-agent starts its own session (file n parent's session directory and named after the agent, `_/PongResponder.jsonl`, even `PongResponder.ExactPong.jsonl` one level deeper) and fires its own `session_start`, so the relay announced a rotation, the store retired the main session, and the parent's real -`session_stop` was rejected. The extension now classifies a session by its file name: anything -other than `_.jsonl` is a sub-agent, whose `session_start` / +`session_stop` was rejected. Two attempts preceded the final rule. A file-name test +(`_.jsonl` = main) dropped every event of a Pi session started with a custom +`--session-id`, which review caught. A stateful rule ("the first announcer, or a UI context's +re-announcement, is the main session") passed its harness and failed live: the runtime loads a +**fresh extension instance for every sub-agent session** (measured — distinct module instances +sharing one `globalThis`), so the sub-agent's instance saw its own `session_start` as the first. +The final rule is stateless and structural: a sub-agent's session file is nested inside the +parent's session directory (`_/.jsonl`, deeper for nested sub-agents), +so a session whose file has a session-directory ancestor is a sub-agent and that directory names +the pane's session id — the id itself is never interpreted. Its `session_start` / `session_shutdown` are dropped while its `tool_approval_requested` — which still blocks the user -— is forwarded under the main session id last announced. Pi carries the same guard. Re-verified -live: the approval reported `needs-input` on the main session and the wait resolved on the -parent's `session_stop`. +— is forwarded under the pane's session. Pi carries the same guard, and +`scripts/test_agent_hooks.py` runs the real extensions through Node against a capture CLI +(custom ids, nested and doubly nested files, ephemeral sessions, `/new`, OMP `session_switch`, +sub-agent approvals, OpenCode `parentID` filtering, no-token silence). Re-verified live: the +approval reported `needs-input` on the pane's session and the wait resolved on the parent's +`session_stop`. + +### Review fixes + +- OpenCode's TUI `--replay-limit ` was missing from the value-option table, so `7` would + have been registered as the project directory and every hook rejected on the cwd guard with + no warning. The table now matches the 1.18.23 `--help`, and — because OpenCode refuses to + start in a directory that does not exist — a positional that is not an existing directory is + treated as the value of an unknown option and the launch directory stays inherited. +- `ShellEnvironmentProbe` declared a 256 KiB bound but ran the Codex probe process at its 16 KiB + default, so a ~20 KiB exported `OPENCODE_CONFIG_CONTENT` degraded the launch. The process + bound now follows the probe's; covered at the process level and through the production runner. ### Display sleep is the CREATE_FAILED behind the "intermittent" Profile launches diff --git a/docs-ai/064-agent-completion-signals/research-agent-completion-signals.md b/docs-ai/064-agent-completion-signals/research-agent-completion-signals.md index de8d8eed..819a9033 100644 --- a/docs-ai/064-agent-completion-signals/research-agent-completion-signals.md +++ b/docs-ai/064-agent-completion-signals/research-agent-completion-signals.md @@ -50,7 +50,7 @@ Confidence: **V** verified locally (live run or binary/source) · **D** official | Qwen Code | hooks `Stop` (`last_assistant_message`), `StopFailure`; `SessionStart` / `SessionEnd` | hooks `PermissionRequest`, `PermissionDenied`, `Notification` `permission_prompt`, `idle_prompt` (not focus-gated) | no flag; project `.qwen/settings.json` — live-verified; `QWEN_CODE_SYSTEM_SETTINGS_PATH` exists but did NOT fire hooks in test; `QWEN_HOME` = full relocation | stdin JSON: `session_id`, `transcript_path`, `cwd`, `timestamp`, `permission_mode`, `model` | `general.terminalBell` (default true): OSC 9/99/777/BEL, unfocused only; completion only after ≥20 s turns | `~/.qwen/projects//chats/.jsonl` + `.runtime.json` (pid); no turn marker in 0.21.3 | V | | Grok Build | hooks `Stop` (`reason:"end_turn"`, `lastAssistantMessage`); `[[ui.notifications.hooks]]` on `turn_complete`; `SessionStart` / `SessionEnd` | hooks `Notification` (`permission_prompt`, `idle_prompt`), `PermissionDenied`; `[[ui.notifications.hooks]]` `approval_required` | NO flag/env on the TUI (`GROK_HOME` relocates incl. auth); `~/.grok/hooks/*.json`, trusted `/.grok/hooks/*.json`, Claude/Cursor-compat files | stdin JSON camelCase: `sessionId`, `cwd`, `lastAssistantMessage`; env `GROK_SESSION_ID`, `GROK_EVENT`, `GROK_MESSAGE` | default ON but focus-gated: OSC 777/9/99/BEL; OSC 9;4 `progress_bar=true` | `~/.grok/sessions///updates.jsonl` `turn_completed`; `events.jsonl` `permission_requested`, `turn_ended` | D + binary | | Pi | extension `agent_end` → `agent_settled` (idle; 0.84.3 measured: `session_start{reason:startup}` → `input` → `turn_start` → `agent_start` → `turn_end` → `agent_end` → `agent_settled`); `session_start` (`reason` startup/new/resume/fork/reload) / `session_shutdown` (`reason` quit/new); `/new` = `session_shutdown{new}` + `session_start{new}` with a new id | none (no permission system) | `pi -e /abs/ext.ts` — live-verified; additive, survives `--no-extensions`, loads before `project_trust`, read-only dir OK; **a missing `-e` path aborts startup**; `PI_CODING_AGENT_DIR` = full relocation | in-process: `ctx.sessionManager.getSessionId()` (= UUID in the session file name) / `getSessionFile()`, `ctx.cwd` (resolved), `agent_end.messages`; extensions may `spawn` (Node) | none by default; OSC 9;4 if `terminal.showTerminalProgress`; OSC 133 | `~/.pi/agent/sessions/----/_.jsonl`: assistant `stopReason:"stop"` | V | -| Oh My Pi | extension `session_stop` (documented main-session only; once per prompt; Claude-`Stop`-shaped payload `session_id`/`turn_id`/`stop_hook_active`/`last_assistant_message`) — `agent_end` also fires **per in-process `task` sub-agent** (3× for one sub-agent, 18.0.6), with an undocumented `willContinue` that measured `undefined`; `session_start` at startup only, `/new` = `session_before_switch` → `session_switch` (new id), `session_shutdown` at exit | `tool_approval_requested` {`sessionId`,`toolName`,`toolCallId`,`approvalMode`} / `tool_approval_resolved` {…,`approved`} — fires with the built-in TUI approval prompt under `--approval-mode always-ask` (default config `yolo`); `ask` tool → built-in notification only | `omp --hook /abs/ext.ts` (or `-e`, identical) — live-verified; additive, survives `--no-extensions`; missing path warns and continues; `--config overlay.yml` (repeatable); `--profile` isolates auth+sessions | in-process: `ctx.sessionManager.getSessionId()`, `ctx.cwd` (**logical** shell path, e.g. `/tmp/…`), in-process `task` sub-agents run under their **own** session ids (file nested in the parent's session directory, `_/.jsonl`) and fire their own `session_start` / `agent_end`; their handlers see `ctx.hasUI == false` / `ctx.mode == "print"` | default ON: OSC 9/99/BEL; `PI_NOTIFICATIONS=off`; OSC 9;4 if `terminal.showProgress` | `~/.omp/agent/sessions//_.jsonl`: assistant `stopReason:"stop"`; `custom/session_exit` | V | +| Oh My Pi | extension `session_stop` (documented main-session only; once per prompt; Claude-`Stop`-shaped payload `session_id`/`turn_id`/`stop_hook_active`/`last_assistant_message`) — `agent_end` also fires **per in-process `task` sub-agent** (3× for one sub-agent, 18.0.6), with an undocumented `willContinue` that measured `undefined`; `session_start` at startup only, `/new` = `session_before_switch` → `session_switch` (new id), `session_shutdown` at exit | `tool_approval_requested` {`sessionId`,`toolName`,`toolCallId`,`approvalMode`} / `tool_approval_resolved` {…,`approved`} — fires with the built-in TUI approval prompt under `--approval-mode always-ask` (default config `yolo`); `ask` tool → built-in notification only | `omp --hook /abs/ext.ts` (or `-e`, identical) — live-verified; additive, survives `--no-extensions`; missing path warns and continues; `--config overlay.yml` (repeatable); `--profile` isolates auth+sessions | in-process: `ctx.sessionManager.getSessionId()`, `ctx.cwd` (**logical** shell path, e.g. `/tmp/…`), in-process `task` sub-agents run under their **own** session ids (file nested in the parent's session directory, `_/.jsonl`) and fire their own `session_start` / `agent_end`; their handlers see `ctx.hasUI == false` / `ctx.mode == "print"` and run in a **fresh extension module instance** per sub-agent session (same process, shared `globalThis`) | default ON: OSC 9/99/BEL; `PI_NOTIFICATIONS=off`; OSC 9;4 if `terminal.showProgress` | `~/.omp/agent/sessions//_.jsonl`: assistant `stopReason:"stop"`; `custom/session_exit` | V | ## 2. Per-runtime notes (abridged; sources) diff --git a/docs/components/agent-detection.md b/docs/components/agent-detection.md index aed03872..1cef6eb8 100644 --- a/docs/components/agent-detection.md +++ b/docs/components/agent-detection.md @@ -177,8 +177,9 @@ bridges without writing user, dedicated-home, or project configuration: as `turn-ended` (its `agent_end` fires once per in-process `task` sub-agent and is ignored), `tool_approval_requested` as `needs-input` (the built-in approval prompt under `--approval-mode always-ask`), and `session_shutdown`. Its in-process `task` sub-agents run - under their own session ids; the extension recognises them by their nested session file and - forwards only their approval prompts, attributed to the pane's session. OpenCode reports `session.idle` as + under their own session ids with their session files nested inside the pane session's + directory; the extension recognises them by that nesting and forwards only their approval + prompts, attributed to the pane's session. OpenCode reports `session.idle` as `turn-ended` and `permission.asked` / `question.asked` as `needs-input`, only for the session the pane is talking to — sub-agent sessions are filtered out by their `parentID`. OpenCode announces no session start: its session exists only after the first prompt and diff --git a/scripts/test_agent_hooks.py b/scripts/test_agent_hooks.py new file mode 100644 index 00000000..a8361962 --- /dev/null +++ b/scripts/test_agent_hooks.py @@ -0,0 +1,277 @@ +"""Behavioral tests for the bundled managed-hook extensions in Resources/agent-hooks. + +The extensions are TypeScript relays loaded in-process by Pi, Oh My Pi, and OpenCode. Node runs +them here with the same handler contract those runtimes use, against a capture script standing +in for the bundled `prowl` CLI, so the forwarding decisions (which events, which session id, +sub-agent filtering) are pinned without launching an agent. +""" + +import json +import os +import pathlib +import shutil +import subprocess +import tempfile +import textwrap +import unittest + +ROOT = pathlib.Path(__file__).resolve().parents[1] +HOOKS = ROOT / "Resources" / "agent-hooks" +NODE = shutil.which("node") + +CAPTURE = textwrap.dedent( + """\ + #!/bin/sh + payload=$(cat) + printf '%s\\t%s\\n' "$*" "$payload" >> "$PROWL_TEST_CAPTURE" + """ +) + +PI_FAMILY_HARNESS = textwrap.dedent( + """\ + import { pathToFileURL } from "node:url"; + import { promises as fs } from "node:fs"; + const capturePath = process.env.PROWL_TEST_CAPTURE; + async function captureCount() { + try { return (await fs.readFile(capturePath, "utf8")).split("\\n").filter(Boolean).length; } catch { return 0; } + } + async function settle(before) { + const deadline = Date.now() + 700; + while (Date.now() < deadline) { + await new Promise((resolve) => setTimeout(resolve, 20)); + if ((await captureCount()) > before) { await new Promise((resolve) => setTimeout(resolve, 50)); return; } + } + } + const [extensionPath, scriptPath] = process.argv.slice(2); + const module = await import(pathToFileURL(extensionPath)); + const handlers = new Map(); + module.default({ on(name, handler) { handlers.set(name, handler); } }); + const steps = JSON.parse(await import("node:fs").then((fs) => fs.promises.readFile(scriptPath, "utf8"))); + for (const step of steps) { + const handler = handlers.get(step.event); + if (!handler) continue; + const ctx = { + hasUI: step.hasUI, + mode: step.hasUI ? "tui" : "print", + cwd: step.cwd ?? "/tmp/project", + sessionManager: { + getSessionId: () => step.session, + getSessionFile: () => step.file, + }, + }; + const before = await captureCount(); + await handler(step.payload ?? { type: step.event }, ctx); + // The relay spawns the CLI without awaiting it; wait for a capture to land (or for a + // dropped event's grace period to pass) so the forwarding order stays observable. + await settle(before); + } + """ +) + +OPENCODE_HARNESS = textwrap.dedent( + """\ + import { pathToFileURL } from "node:url"; + import { promises as fs } from "node:fs"; + const capturePath = process.env.PROWL_TEST_CAPTURE; + async function captureCount() { + try { return (await fs.readFile(capturePath, "utf8")).split("\\n").filter(Boolean).length; } catch { return 0; } + } + async function settle(before) { + const deadline = Date.now() + 700; + while (Date.now() < deadline) { + await new Promise((resolve) => setTimeout(resolve, 20)); + if ((await captureCount()) > before) { await new Promise((resolve) => setTimeout(resolve, 50)); return; } + } + } + const [pluginPath, scriptPath] = process.argv.slice(2); + const module = await import(pathToFileURL(pluginPath)); + const hooks = await module.ProwlHooks({ directory: "/tmp/project", worktree: "/tmp/project", project: {}, client: {}, $: null }); + const events = JSON.parse(await import("node:fs").then((fs) => fs.promises.readFile(scriptPath, "utf8"))); + for (const event of events) { + const before = await captureCount(); + await hooks.event({ event }); + await settle(before); + } + """ +) + + +@unittest.skipUnless(NODE, "node is required to run the bundled extensions") +class AgentHookExtensionTests(unittest.TestCase): + def setUp(self): + self.tmp = pathlib.Path(tempfile.mkdtemp(prefix="prowl-agent-hooks-")) + self.addCleanup(shutil.rmtree, self.tmp, ignore_errors=True) + cli_dir = self.tmp / "prowl-cli" + cli_dir.mkdir() + cli = cli_dir / "prowl" + cli.write_text(CAPTURE) + cli.chmod(0o755) + self.capture = self.tmp / "capture.log" + self.capture.write_text("") + for runtime in ("pi", "omp", "opencode"): + target = self.tmp / "agent-hooks" / runtime + target.mkdir(parents=True) + shutil.copy(HOOKS / runtime / "prowl-hooks.ts", target / "prowl-hooks.ts") + + def run_harness(self, harness, extension, steps, token="token-1"): + harness_path = self.tmp / "harness.mjs" + harness_path.write_text(harness) + script_path = self.tmp / "steps.json" + script_path.write_text(json.dumps(steps)) + env = dict(os.environ) + env["PROWL_TEST_CAPTURE"] = str(self.capture) + env.pop("PROWL_AGENT_HOOK_TOKEN", None) + if token is not None: + env["PROWL_AGENT_HOOK_TOKEN"] = token + completed = subprocess.run( + [NODE, "--experimental-strip-types", "--no-warnings", str(harness_path), str(extension), str(script_path)], + env=env, + capture_output=True, + text=True, + timeout=60, + check=False, + ) + self.assertEqual(completed.returncode, 0, completed.stderr) + forwarded = [] + for line in self.capture.read_text().splitlines(): + argv, payload = line.split("\t", 1) + forwarded.append((argv.split(" "), json.loads(payload))) + return forwarded + + def pi_family(self, runtime, steps, token="token-1"): + return self.run_harness(PI_FAMILY_HARNESS, self.tmp / "agent-hooks" / runtime / "prowl-hooks.ts", steps, token) + + # Pi + + MAIN_1 = "/Users/me/.pi/agent/sessions/--tmp-project--/2026-08-26T10-00-00-000Z_main-1.jsonl" + MAIN_2 = "/Users/me/.pi/agent/sessions/--tmp-project--/2026-08-26T10-05-00-000Z_main-2.jsonl" + SUB_1 = "/Users/me/.pi/agent/sessions/--tmp-project--/2026-08-26T10-00-00-000Z_main-1/Worker.jsonl" + SUB_NESTED = "/Users/me/.pi/agent/sessions/--tmp-project--/2026-08-26T10-00-00-000Z_main-1/Worker/Worker.Inner.jsonl" + + def test_pi_forwards_its_lifecycle_with_native_names_and_a_custom_session_id(self): + session = "prowlcustom" + file = "/Users/me/.pi/agent/sessions/--tmp-project--/2026-08-26T12-01-30-384Z_prowlcustom.jsonl" + forwarded = self.pi_family( + "pi", + [ + {"event": "session_start", "hasUI": True, "session": session, "file": file, "payload": {"type": "session_start", "reason": "startup"}}, + {"event": "agent_settled", "hasUI": True, "session": session, "file": file}, + {"event": "agent_end", "hasUI": True, "session": session, "file": file}, + {"event": "session_shutdown", "hasUI": True, "session": session, "file": file, "payload": {"type": "session_shutdown", "reason": "quit"}}, + ], + ) + self.assertEqual( + [(argv, payload["hook_event_name"], payload["session_id"], payload.get("reason")) for argv, payload in forwarded], + [ + (["agents", "_hook", "pi", "session_start"], "session_start", session, "startup"), + (["agents", "_hook", "pi", "agent_settled"], "agent_settled", session, None), + (["agents", "_hook", "pi", "session_shutdown"], "session_shutdown", session, "quit"), + ], + ) + self.assertTrue(all(payload["cwd"] == "/tmp/project" for _, payload in forwarded)) + + def test_pi_sub_agent_sessions_are_recognised_by_their_nested_file_without_shared_state(self): + # The runtime loads a fresh extension instance per sub-agent session, so every step here + # is classified on its own: a headless main (no UI) still counts, a nested file never does. + forwarded = self.pi_family( + "pi", + [ + {"event": "session_start", "hasUI": False, "session": "main-1", "file": self.MAIN_1}, + {"event": "session_start", "hasUI": False, "session": "sub-1", "file": self.SUB_1}, + {"event": "agent_settled", "hasUI": False, "session": "sub-1", "file": self.SUB_1}, + {"event": "session_shutdown", "hasUI": False, "session": "sub-2", "file": self.SUB_NESTED}, + {"event": "agent_settled", "hasUI": False, "session": "main-1", "file": self.MAIN_1}, + {"event": "agent_settled", "hasUI": True, "session": "ephemeral", "file": None}, + ], + ) + self.assertEqual( + [(payload["hook_event_name"], payload["session_id"]) for _, payload in forwarded], + [("session_start", "main-1"), ("agent_settled", "main-1"), ("agent_settled", "ephemeral")], + ) + + def test_pi_new_session_in_the_tui_rotates_the_main_session(self): + forwarded = self.pi_family( + "pi", + [ + {"event": "session_start", "hasUI": True, "session": "main-1", "file": self.MAIN_1}, + {"event": "session_shutdown", "hasUI": True, "session": "main-1", "file": self.MAIN_1, "payload": {"type": "session_shutdown", "reason": "new"}}, + {"event": "session_start", "hasUI": True, "session": "main-2", "file": self.MAIN_2, "payload": {"type": "session_start", "reason": "new"}}, + {"event": "agent_settled", "hasUI": True, "session": "main-2", "file": self.MAIN_2}, + ], + ) + self.assertEqual( + [(payload["hook_event_name"], payload["session_id"]) for _, payload in forwarded], + [("session_start", "main-1"), ("session_shutdown", "main-1"), ("session_start", "main-2"), ("agent_settled", "main-2")], + ) + + def test_pi_without_a_launch_token_spawns_nothing(self): + forwarded = self.pi_family("pi", [{"event": "session_start", "hasUI": True, "session": "main-1", "file": self.MAIN_1}], token=None) + self.assertEqual(forwarded, []) + + # Oh My Pi + + def test_omp_forwards_session_switch_and_sub_agent_approvals_under_the_parent_session(self): + forwarded = self.pi_family( + "omp", + [ + {"event": "session_start", "hasUI": True, "session": "main-1", "file": self.MAIN_1}, + {"event": "tool_approval_requested", "hasUI": True, "session": "main-1", "file": self.MAIN_1, "payload": {"type": "tool_approval_requested", "toolName": "task", "sessionId": "main-1"}}, + {"event": "session_start", "hasUI": False, "session": "sub-1", "file": self.SUB_1}, + {"event": "tool_approval_requested", "hasUI": False, "session": "sub-1", "file": self.SUB_1, "payload": {"type": "tool_approval_requested", "toolName": "write", "sessionId": "sub-1"}}, + {"event": "tool_approval_requested", "hasUI": False, "session": "sub-2", "file": self.SUB_NESTED, "payload": {"type": "tool_approval_requested", "toolName": "bash", "sessionId": "sub-2"}}, + {"event": "agent_end", "hasUI": False, "session": "sub-1", "file": self.SUB_1}, + {"event": "session_shutdown", "hasUI": False, "session": "sub-1", "file": self.SUB_1}, + {"event": "session_stop", "hasUI": True, "session": "main-1", "file": self.MAIN_1, "payload": {"type": "session_stop", "session_id": "main-1", "last_assistant_message": "secret"}}, + {"event": "session_switch", "hasUI": True, "session": "main-2", "file": self.MAIN_2}, + {"event": "session_stop", "hasUI": True, "session": "main-2", "file": self.MAIN_2, "payload": {"type": "session_stop", "session_id": "main-2"}}, + {"event": "session_shutdown", "hasUI": True, "session": "main-2", "file": self.MAIN_2}, + ], + ) + self.assertEqual( + [(payload["hook_event_name"], payload["session_id"], payload.get("reason")) for _, payload in forwarded], + [ + ("session_start", "main-1", None), + ("tool_approval_requested", "main-1", "task"), + ("tool_approval_requested", "main-1", "write"), + ("tool_approval_requested", "main-1", "bash"), + ("session_stop", "main-1", None), + ("session_switch", "main-2", None), + ("session_stop", "main-2", None), + ("session_shutdown", "main-2", None), + ], + ) + self.assertTrue(all(argv[:3] == ["agents", "_hook", "omp"] for argv, _ in forwarded)) + self.assertFalse(any("secret" in json.dumps(payload) for _, payload in forwarded)) + + # OpenCode + + def test_opencode_forwards_top_level_session_events_and_drops_sub_agent_sessions(self): + forwarded = self.run_harness( + OPENCODE_HARNESS, + self.tmp / "agent-hooks" / "opencode" / "prowl-hooks.ts", + [ + {"type": "session.created", "properties": {"sessionID": "ses_main", "info": {"id": "ses_main"}}}, + {"type": "session.status", "properties": {"sessionID": "ses_main", "status": {"type": "busy"}}}, + {"type": "session.created", "properties": {"sessionID": "ses_child", "info": {"id": "ses_child", "parentID": "ses_main"}}}, + {"type": "session.idle", "properties": {"sessionID": "ses_child"}}, + {"type": "permission.asked", "properties": {"sessionID": "ses_child", "permission": "edit"}}, + {"type": "permission.asked", "properties": {"sessionID": "ses_main", "permission": "edit"}}, + {"type": "permission.replied", "properties": {"sessionID": "ses_main", "reply": "once"}}, + {"type": "question.asked", "properties": {"sessionID": "ses_main"}}, + {"type": "session.error", "properties": {"sessionID": "ses_main"}}, + {"type": "session.idle", "properties": {"sessionID": "ses_main"}}, + ], + ) + self.assertEqual( + [(argv, payload["hook_event_name"], payload["session_id"], payload.get("reason")) for argv, payload in forwarded], + [ + (["agents", "_hook", "opencode", "permission.asked"], "permission.asked", "ses_main", "edit"), + (["agents", "_hook", "opencode", "question.asked"], "question.asked", "ses_main", None), + (["agents", "_hook", "opencode", "session.idle"], "session.idle", "ses_main", None), + ], + ) + self.assertTrue(all(payload["cwd"] == "/tmp/project" for _, payload in forwarded)) + + +if __name__ == "__main__": + unittest.main() diff --git a/supacode/Domain/AgentRuntime/AgentManagedHookPreparer.swift b/supacode/Domain/AgentRuntime/AgentManagedHookPreparer.swift index dd3043f6..949a50b0 100644 --- a/supacode/Domain/AgentRuntime/AgentManagedHookPreparer.swift +++ b/supacode/Domain/AgentRuntime/AgentManagedHookPreparer.swift @@ -229,7 +229,7 @@ nonisolated enum AgentManagedHookPreparer { } let arguments = plan.invocation.arguments guard - let launchCWD = ManagedHookWorkingDirectory.effective( + var launchCWD = ManagedHookWorkingDirectory.effective( inherited: inheritedCWD, scan: OpenCodeLaunchDirectory.scan(arguments: arguments, promptArgumentIndex: promptIndex) ) @@ -241,6 +241,15 @@ nonisolated enum AgentManagedHookPreparer { message: "The OpenCode project directory could not be resolved." ) } + // OpenCode refuses to start in a directory that does not exist, so a candidate that is not + // one can only be the value of an option the scanner does not know; the hooks will report + // the launch directory in that case. + var isDirectory: ObjCBool = false + if !FileManager.default.fileExists(atPath: launchCWD.path(percentEncoded: false), isDirectory: &isDirectory) + || !isDirectory.boolValue + { + launchCWD = inheritedCWD + } let overrides = plan.profileEnvironmentOverrides var content = overrides[OpenCodeHookPluginPreparer.contentVariableName] var pure = overrides[OpenCodeHookPluginPreparer.pureVariableName] diff --git a/supacode/Domain/AgentRuntime/CodexShellProbeProcess.swift b/supacode/Domain/AgentRuntime/CodexShellProbeProcess.swift index b24f151f..e93c88f8 100644 --- a/supacode/Domain/AgentRuntime/CodexShellProbeProcess.swift +++ b/supacode/Domain/AgentRuntime/CodexShellProbeProcess.swift @@ -19,6 +19,7 @@ nonisolated struct CodexShellProbeProcess: Sendable { let maximumOutputBytes: Int let shellOverride: URL? let shellOverrideArguments: [String] + let environment: [String: String]? } private struct OutputDescriptor { @@ -45,17 +46,21 @@ nonisolated struct CodexShellProbeProcess: Sendable { let maximumOutputBytes: Int let shellOverride: URL? let shellOverrideArguments: [String] + /// The child's environment; `nil` inherits the app's, which is what every launch probe wants. + let environment: [String: String]? init( timeout: TimeInterval = 1, maximumOutputBytes: Int = 16 * 1_024, shellOverride: URL? = nil, - shellOverrideArguments: [String] = [] + shellOverrideArguments: [String] = [], + environment: [String: String]? = nil ) { self.timeout = max(0.05, timeout) self.maximumOutputBytes = max(1, maximumOutputBytes) self.shellOverride = shellOverride self.shellOverrideArguments = shellOverrideArguments + self.environment = environment } func run(cwd: URL, script: String) async throws -> ShellOutput { @@ -68,7 +73,8 @@ nonisolated struct CodexShellProbeProcess: Sendable { timeout: timeout, maximumOutputBytes: maximumOutputBytes, shellOverride: shellOverride, - shellOverrideArguments: shellOverrideArguments + shellOverrideArguments: shellOverrideArguments, + environment: environment ), processBox: processBox ) @@ -100,6 +106,7 @@ nonisolated struct CodexShellProbeProcess: Sendable { ] } process.currentDirectoryURL = cwd + if let environment = options.environment { process.environment = environment } process.standardInput = FileHandle.nullDevice let output = Pipe() let errors = Pipe() diff --git a/supacode/Domain/AgentRuntime/ManagedHookRendering.swift b/supacode/Domain/AgentRuntime/ManagedHookRendering.swift index a4edecc7..f62adb7a 100644 --- a/supacode/Domain/AgentRuntime/ManagedHookRendering.swift +++ b/supacode/Domain/AgentRuntime/ManagedHookRendering.swift @@ -896,11 +896,13 @@ nonisolated enum OpenCodeHookPluginPreparer { /// `opencode run` (a repeated `--dir` crashes the runtime, so the last one is taken as its /// intent); neither form has an environment equivalent. nonisolated enum OpenCodeLaunchDirectory { - /// Options that consume the next argument, so a value is never mistaken for the project. + /// Options that consume the next argument (OpenCode 1.18.23 `--help`, TUI and `run`), so a + /// value is never mistaken for the project. The preparer additionally requires a project + /// positional to be an existing directory, which catches a value option added later. private static let valueOptions: Set = [ "-m", "--model", "--agent", "--prompt", "--variant", "-s", "--session", "--port", "--hostname", - "--mdns-domain", "--cors", "--log-level", "--dir", "--command", "-f", "--file", "--title", - "--attach", "-p", "--password", "-u", "--username", "--format", + "--mdns-domain", "--cors", "--log-level", "--replay-limit", "--dir", "--command", "-f", "--file", + "--title", "--attach", "-p", "--password", "-u", "--username", "--format", ] static func scan(arguments: [String], promptArgumentIndex: Int?) -> ManagedHookWorkingDirectory.Scan { diff --git a/supacode/Domain/AgentRuntime/ShellEnvironmentProbe.swift b/supacode/Domain/AgentRuntime/ShellEnvironmentProbe.swift index f230d059..017ecbc5 100644 --- a/supacode/Domain/AgentRuntime/ShellEnvironmentProbe.swift +++ b/supacode/Domain/AgentRuntime/ShellEnvironmentProbe.swift @@ -35,10 +35,7 @@ nonisolated enum ShellEnvironmentProbe { run: (@Sendable (URL, String) async throws -> ShellOutput)? = nil ) async -> Resolution { guard !variables.isEmpty, variables.allSatisfy(isShellIdentifier) else { return .failed } - let execute = - run ?? { cwd, script in - try await CodexShellProbeProcess().run(cwd: cwd, script: script) - } + let execute = run ?? defaultRunner() var effectiveScript = script(for: variables) if let pathOverride { effectiveScript = "PATH=\(AgentInvocation.shellQuote(pathOverride)); export PATH\n" + effectiveScript @@ -52,6 +49,21 @@ nonisolated enum ShellEnvironmentProbe { return .values(values) } + /// The production runner: the login-shell probe Codex uses, with this probe's output bound. + /// The Codex config probe defaults to 16 KiB; an exported `OPENCODE_CONFIG_CONTENT` can + /// legitimately be larger. `environment` is for tests that must not mutate the host's. + static func defaultRunner( + timeout: TimeInterval = 1, + environment: [String: String]? = nil + ) -> @Sendable (URL, String) async throws -> ShellOutput { + let process = CodexShellProbeProcess( + timeout: timeout, + maximumOutputBytes: maximumOutputBytes, + environment: environment + ) + return { cwd, script in try await process.run(cwd: cwd, script: script) } + } + private static func isShellIdentifier(_ name: String) -> Bool { guard let first = name.unicodeScalars.first, first == "_" || CharacterSet.letters.contains(first) else { return false diff --git a/supacodeTests/AgentS3cHookRenderingTests.swift b/supacodeTests/AgentS3cHookRenderingTests.swift index 227a15db..1ebf9154 100644 --- a/supacodeTests/AgentS3cHookRenderingTests.swift +++ b/supacodeTests/AgentS3cHookRenderingTests.swift @@ -223,6 +223,20 @@ struct AgentS3cHookRenderingTests { OpenCodeLaunchDirectory.scan(arguments: ["--prompt", "Review this"], promptArgumentIndex: 1) == Scan.inherited) #expect(OpenCodeLaunchDirectory.scan(arguments: ["--auto", "--", "x"], promptArgumentIndex: nil) == Scan.inherited) + // Every OpenCode 1.18.23 TUI option that takes a value must be known, or its value would be + // read as the project (`--replay-limit 7` would register `/7`). + #expect( + OpenCodeLaunchDirectory.scan( + arguments: ["--mini", "--no-replay", "--replay-limit", "7", "--prompt", "Review this"], promptArgumentIndex: 5) + == Scan.inherited) + #expect( + OpenCodeLaunchDirectory.scan( + arguments: [ + "--log-level", "DEBUG", "--port", "4096", "--hostname", "127.0.0.1", "--mdns", "--mdns-domain", "x.local", + "--cors", "a.example", "-s", "ses_1", "--fork", "--agent", "plan", "--variant", "high", + ], + promptArgumentIndex: nil) + == Scan.inherited) #expect(OpenCodeLaunchDirectory.scan(arguments: ["a", "b"], promptArgumentIndex: nil) == Scan.malformed) #expect( OpenCodeLaunchDirectory.scan( @@ -338,13 +352,27 @@ struct AgentS3cHookRenderingTests { #expect(ambiguous.preparedInvocation == nil) #expect(ambiguous.warning?.message.contains("project directory") == true) + try FileManager.default.createDirectory(at: root.appending(path: "nested"), withIntermediateDirectories: true) let project = await AgentManagedHookPreparer.prepare( - plan: makePlan(runtime: .opencode, arguments: ["nested", "--prompt", "Review this"]), + plan: makePlan(runtime: .opencode, arguments: ["nested", "--prompt", "Review this"], prompt: "Review this"), inheritedCWD: root, resources: resources, openCodeEnvironmentResolver: { _, _ in .values(["OPENCODE_CONFIG_CONTENT": nil, "OPENCODE_PURE": nil]) } ) #expect(trimmed(project.launchCWD) == trimmed(root.appending(path: "nested"))) + // OpenCode refuses to start in a directory that does not exist, so a positional that is not + // an existing directory can only be the value of an option the scanner does not know: the + // launch directory stays inherited instead of registering a path the hooks would never report. + let unknownValue = await AgentManagedHookPreparer.prepare( + plan: makePlan( + runtime: .opencode, arguments: ["--future-option", "7", "--prompt", "Review this"], prompt: "Review this"), + inheritedCWD: root, resources: resources, + openCodeEnvironmentResolver: { _, _ in .values(["OPENCODE_CONFIG_CONTENT": nil, "OPENCODE_PURE": nil]) } + ) + #expect(unknownValue.warning == nil) + #expect(unknownValue.launchCWD == root) + #expect(unknownValue.preparedInvocation != nil) + resources.opencodePluginPath = nil let missing = await AgentManagedHookPreparer.prepare( plan: makePlan(runtime: .opencode, arguments: []), @@ -378,6 +406,21 @@ struct AgentS3cHookRenderingTests { #expect(withPath == .values(["PATH": "/custom/bin:/usr/bin:/bin"])) } + /// The production runner is the login-shell probe Codex uses, whose own default output bound + /// is 16 KiB; a 20 KiB exported content must still come back whole. Only the deadline is + /// relaxed, because the parallel suite can starve a real login shell past one second. + @Test func shellEnvironmentProbeCarriesA20KiBValueThroughTheProductionRunner() async { + let name = "PROWL_S3C_PROBE_LARGE_VALUE" + let value = String(repeating: "x", count: 20 * 1_024) + let environment = ProcessInfo.processInfo.environment.merging([name: value]) { $1 } + let resolution = await ShellEnvironmentProbe.resolve( + variables: [name], + cwd: FileManager.default.temporaryDirectory, + run: ShellEnvironmentProbe.defaultRunner(timeout: 15, environment: environment) + ) + #expect(resolution == .values([name: value])) + } + @Test func shellEnvironmentProbeFailsClosedOnBadNamesOrIncompleteOutput() async { let cwd = FileManager.default.temporaryDirectory #expect( diff --git a/supacodeTests/CodexShellProbeProcessTests.swift b/supacodeTests/CodexShellProbeProcessTests.swift index 2775a1f8..01166b1e 100644 --- a/supacodeTests/CodexShellProbeProcessTests.swift +++ b/supacodeTests/CodexShellProbeProcessTests.swift @@ -104,6 +104,40 @@ struct CodexShellProbeProcessTests { } } + /// A shell-exported `OPENCODE_CONFIG_CONTENT` can legitimately be tens of kilobytes, so the + /// environment probe must be able to raise the bound the Codex config probe defaults to. + @Test func outputBoundIsConfigurableForLargeEnvironmentValues() async throws { + let root = temporaryDirectory("shell-large-output") + try FileManager.default.createDirectory(at: root, withIntermediateDirectories: true) + defer { try? FileManager.default.removeItem(at: root) } + let shell = try executableScript( + in: root, + name: "large.sh", + contents: """ + #!/bin/sh + head -c 20480 /dev/zero | tr '\\0' a + """ + ) + let defaultBound = CodexShellProbeProcess( + timeout: 2, + shellOverride: URL(filePath: "/bin/sh"), + shellOverrideArguments: [shell.path(percentEncoded: false)] + ) + await #expect(throws: CodexShellProbeProcessError.outputTooLarge) { + try await defaultBound.run(cwd: root, script: "ignored") + } + + let raised = CodexShellProbeProcess( + timeout: 2, + maximumOutputBytes: ShellEnvironmentProbe.maximumOutputBytes, + shellOverride: URL(filePath: "/bin/sh"), + shellOverrideArguments: [shell.path(percentEncoded: false)] + ) + let output = try await raised.run(cwd: root, script: "ignored") + #expect(output.stdout.utf8.count == 20_480) + #expect(output.exitCode == 0) + } + private func executableScript(in root: URL, name: String, contents: String) throws -> URL { let url = root.appending(path: name, directoryHint: .notDirectory) try contents.write(to: url, atomically: true, encoding: .utf8) -- 2.51.2