diff --git a/docs/decisions/0005-output-budget-protocol.md b/docs/decisions/0005-output-budget-protocol.md index 9fce0050c..6da0e38c7 100644 --- a/docs/decisions/0005-output-budget-protocol.md +++ b/docs/decisions/0005-output-budget-protocol.md @@ -35,9 +35,12 @@ containment; downstream layers are independent backstops rather than substitutes handler representation and uses routing- and shape-preserving projection. This is producer-blind, post-execution enforcement. A response that cannot be persisted or projected safely fails closed without returning the original payload. -3. **Pre-spend command guard:** enumerated high-confidence unbounded shell shapes are - denied before execution and receive a bounded-rewrite instruction. This is static, - pre-execution enforcement for rules R1 through R3. +3. **Pre-spend command guard:** *Retired by ADR-0006 (#4286).* The command-shape + classifier and its `output_budget_guard` are deleted. Codex native shell is now + bounded by a PreToolUse input-rewrite hook (`shell_capture_hook`) that captures + complete output to a mechanism-owned artifact and emits only a bounded inline + slice. The transport ceiling (CODEX_TOOL_OUTPUT_TOKEN_LIMIT) remains as the + backstop for hook-failure paths.* 4. **Producer-aware discipline and derived transport ceiling:** one evidence-output policy is delivered on backend surfaces that support it, while Codex's stored tool/function output receives a derived damage ceiling. The policy is advisory; @@ -61,11 +64,12 @@ an exemption requires re-measurement and a deliberate registry-digest change. | `response_max_bytes = 90_000` | Bound the exact compact serialized handler payload before a coarser transport can clip it. Bytes are authoritative here; this is not a token or full JSON-RPC-envelope estimate. | | `MAX_MCP_OUTPUT_TOKENS = 50_000` | Keep Claude's independently defined setting separate. It has no shared source of truth with `CODEX_TOOL_OUTPUT_TOKEN_LIMIT` and does not control Claude Code's observed disk-persistence gate. Claude's native Bash spill behavior covers shell output on that backend. | -The command guard also reads `small_file_max_bytes = 5_000` for its narrow literal-JSONL -exception and accepts proven byte sinks only through `shell_max_inline_bytes = 12_000`. -Those are classification bounds, not transport limits. Parser limits of 65,536 command -characters and nesting depth 8 bound classification work; exceeding them produces an -unknown disposition and cannot authorize a risky R1-R3 command. +The shell capture hook uses `shell_max_inline_bytes = 12_000` as the inline threshold: +commands whose combined output fits within that budget are inlined in full (artifact +deleted); larger outputs are captured losslessly to a mechanism-owned artifact with a +bounded head/tail slice and provenance marker inlined. +`small_file_max_bytes` was removed — it existed solely for the classifier's +literal-small-JSONL exception, which is no longer needed. ## Ceiling and Backstop Reconciliation @@ -121,9 +125,9 @@ recorded explicitly in the issue body. ## Accepted Gaps -1. Non-JSONL single-file searches and shell verbs outside R1-R3, including `python -c` - file dumps, `git log -p`, and `curl`, are bounded on Codex only by the derived ceiling. - Codex may still truncate and discard their excess output. +1. Non-JSONL single-file searches and arbitrary shell verbs (`python -c` file dumps, + `git log -p`, `curl`) are now captured losslessly on Codex by the shell capture hook + (#4286 / ADR-0006). The transport ceiling remains the backstop for hook-failure paths. 2. The interactive discipline digest does not survive Codex `resume` and cannot guarantee post-compaction reinjection. Those paths retain the command guard, transport ceilings, and file-materialized skill content. @@ -138,8 +142,9 @@ recorded explicitly in the issue body. members must retrieve bounded slices from that artifact. 7. Repeated individually bounded calls can still exhaust cumulative context. No pre-call component owns current context usage, so reserve instructions remain advisory. -8. Subprocess output is still materialized in worker memory before model-context shaping. - The protocol does not bound worker memory or temporary-disk consumption. +8. Closed for the `run_cmd` channel by #4286 (capture files promoted in place; only + bounded slices enter worker memory). Still open for `run_skill` and `test_check`, + whose adjudication requires the full text. ## Operational Signals diff --git a/docs/decisions/0006-output-containment.md b/docs/decisions/0006-output-containment.md new file mode 100644 index 000000000..ac370e9c1 --- /dev/null +++ b/docs/decisions/0006-output-containment.md @@ -0,0 +1,92 @@ +# ADR-0006: Output-Boundary Containment + +**Status:** Accepted +**Date:** 2026-07-18 +**Issue:** #4286 + +## Context + +ADR-0005 (Layer 3) established a pre-execution command-shape classifier +(`output_budget_guard`) to deny unbounded shell commands before they run. The +classifier's failure mode is structural: static pre-execution boundedness proof +over arbitrary shell is unwinnable — every enumerated shape can be expressed in +an equivalent, un-enumerable form. Enforcement must move to the output boundary: +bound what actually enters model context (measured bytes, lossless spill to +durable artifacts, bounded inline slices), per backend. + +## Decision + +Retire the pre-execution command-shape classifier and replace it with per-backend +output-boundary bounding on measured bytes: + +1. **Claude Code native shell** — already bounded by the harness's native Bash + spill mechanism (no AutoSkillit surface needed). +2. **MCP `run_cmd` channel** — lossless capture-file promotion: subprocess output + goes to artifact-directory files; only bounded slices enter worker memory; + oversized outputs are promoted in place with a contract (bytes, sha256, + completeness). +3. **Codex native shell** — PreToolUse input-rewrite hook wraps every shell + command in a capture harness: complete output to a mechanism-owned artifact, + bounded inline slice with provenance marker, exit code preserved. The + transport ceiling (CODEX_TOOL_OUTPUT_TOKEN_LIMIT) remains as the backstop for + hook-failure paths. + +### Sequencing Rule + +The guard is retired on Codex only in the same change that delivers the Codex +lossless mechanism. Claude Code relief is immediate (Phase A). + +### Provenance Rule + +Every residual hook message uses a typed policy event rendered by a shared +formatter. Suggested rewrites are classifier-validated before emission. + +### Pre-Spend Decision + +No shape-based pre-execution backstop remains in the end state. Execution cost is +accepted — bounded by tool timeouts — because context cost is what this mechanism +exists to bound. Catastrophic side-effect prevention belongs to `write_guard` and +the Codex sandbox, not to output budgeting. + +### Unified Exec Assumption + +The hook contract for `exec_command` is identical (tool `"Bash"`, string `command`), +so the rewrite applies there too. AutoSkillit does not enable Codex's experimental +`unified_exec` surface in the config it writes; interactive stdin-driven sessions are +the only case where file-redirected output would change observable behavior. + +### Future Direction + +Route (c) — upstream pre-truncation integration — is the ideal end state but outside +this repo's control. If Codex exposes a pre-truncation hook point, the shell capture +hook can be retired in favor of that mechanism. + +## Accepted Gaps + +1. `disown`ed/job-table-detached children are outside the `wait` drain guarantee. + Their post-exit writes into the artifact are best-effort — mirrors native + detached-output semantics. +2. Fatal self-signals (e.g. `kill -TERM $$`) skip the slice-emission postlude. + Exit-code parity holds, the artifact persists as lossless evidence, but no inline + slice is emitted and the file is not deleted. SIGKILL is untrappable, so full + closure is impossible. +3. Head/tail slices are byte-cut and may split multibyte UTF-8 at slice edges. The + artifact is authoritative. +4. Draining non-detached background jobs makes the harness synchronous for their + duration, bounded by the tool timeout. `disown` is the fire-and-forget escape. +5. A bare trailing backslash at EOF loses its literal backslash from output under + continuation semantics. Exit code is preserved. +6. Vendored-tree version discrepancy: the checkout tag is `rust-v0.143.0-alpha.10` + vs the 0.144.1 description in the issue/ADR. The hook contract must be re-verified + against the deployed Codex version before shipping. + +## Consequences + +- Claude Code sessions no longer see AutoSkillit shell deny or rewrite surfaces. +- The classification engine (`classify_command_output_budget` and supporting + functions) is deleted; shared tokenization utilities are preserved. +- Configuration surface reduces: `small_file_max_bytes` is removed (existed solely + for the classifier's literal-small-JSONL exception). +- `shell_max_inline_bytes` survives with its new capture-threshold meaning. +- Complete Codex shell output is captured to mechanism-owned artifacts at + `/.autoskillit/temp/shell_capture/shell_.log`. diff --git a/docs/decisions/README.md b/docs/decisions/README.md index 63f65d2b1..64fc00742 100644 --- a/docs/decisions/README.md +++ b/docs/decisions/README.md @@ -5,3 +5,4 @@ - [0003-skill-args.md](0003-skill-args.md) — Pass skill inputs as positional arguments, not environment variables - [0004-recipe-redelivery.md](0004-recipe-redelivery.md) — Sanctioned `load_recipe` channel for recipe knowledge re-delivery after Codex context compaction - [0005-output-budget-protocol.md](0005-output-budget-protocol.md) — Bound per-response model-context output with lossless artifacts, pre-spend guards, and derived transport ceilings +- [0006-output-containment.md](0006-output-containment.md) — Retire pre-execution command-shape classification in favor of per-backend output-boundary bounding on measured bytes diff --git a/docs/safety/hooks.md b/docs/safety/hooks.md index 3143eaca2..af826512b 100644 --- a/docs/safety/hooks.md +++ b/docs/safety/hooks.md @@ -84,15 +84,17 @@ and nested-shell forms. Session scope: headless only. Orchestrator sessions are exempt. Per-subcommand allow-overrides are available via `git_ops_policy` in `.hook_config.json` for future recipes that legitimately need these operations. -### `output_budget_guard.py` -**Guarded tools:** `Bash`, `run_cmd` -Denies high-confidence unbounded recursive searches, JSONL reads, and `find` -operations before execution. Commands can prove bounded output by routing both -stdout and stderr through a same-pipeline byte cap, redirecting both descriptors -under `.autoskillit/temp/`, or using a narrowly defined output-free/small-file -exception. The guard applies to interactive and headless sessions. Set -`output_budget.guard_enabled: false` to disable it; byte caps are constrained by -`output_budget.shell_max_inline_bytes`. +### `shell_capture_hook.py` +**Matched tool:** `Bash` +**Scope:** Codex sessions only (#4286 / ADR-0006); Claude Code is unaffected. +PreToolUse input-rewrite hook that wraps every native shell command on Codex in a +capture harness. The original command runs unmodified in a subshell; its complete +combined stdout+stderr goes to a mechanism-owned artifact at +`/.autoskillit/temp/shell_capture/shell_.log`. Only a bounded inline +slice enters context: full content when small (artifact deleted), else head + +provenance marker (bytes, sha256, path) + tail. Exit codes are preserved via a +trap-EXIT postlude. Set `output_budget.guard_enabled: false` to disable; +inline threshold controlled by `output_budget.shell_max_inline_bytes`. ### `generated_file_write_guard.py` **Guarded tools:** `Write`, `Edit` diff --git a/src/autoskillit/config/_config_dataclasses.py b/src/autoskillit/config/_config_dataclasses.py index 3e1105254..d2b96a0c0 100644 --- a/src/autoskillit/config/_config_dataclasses.py +++ b/src/autoskillit/config/_config_dataclasses.py @@ -355,7 +355,6 @@ class OutputBudgetConfig: tail_chars: int = 2500 response_max_bytes: int = 90_000 guard_enabled: bool = True - small_file_max_bytes: int = 5000 shell_max_inline_bytes: int = 12_000 def __post_init__(self) -> None: diff --git a/src/autoskillit/config/defaults.yaml b/src/autoskillit/config/defaults.yaml index 33f6cb83f..11b18ea69 100644 --- a/src/autoskillit/config/defaults.yaml +++ b/src/autoskillit/config/defaults.yaml @@ -136,7 +136,6 @@ output_budget: tail_chars: 2500 response_max_bytes: 90000 guard_enabled: true - small_file_max_bytes: 5000 shell_max_inline_bytes: 12000 branching: diff --git a/src/autoskillit/core/__init__.pyi b/src/autoskillit/core/__init__.pyi index 46736c186..be69329ef 100644 --- a/src/autoskillit/core/__init__.pyi +++ b/src/autoskillit/core/__init__.pyi @@ -252,6 +252,7 @@ from .types import CampaignProtector as CampaignProtector from .types import CanonicalTokenUsage as CanonicalTokenUsage from .types import CapabilityNotSupportedError as CapabilityNotSupportedError from .types import CapabilityResolutionDetail as CapabilityResolutionDetail +from .types import CapturedStream as CapturedStream from .types import CaptureEntrySpec as CaptureEntrySpec from .types import CaptureValueType as CaptureValueType from .types import CaptureValueTypeError as CaptureValueTypeError diff --git a/src/autoskillit/core/types/_type_results.py b/src/autoskillit/core/types/_type_results.py index 5f8e8d4f5..1e4808b66 100644 --- a/src/autoskillit/core/types/_type_results.py +++ b/src/autoskillit/core/types/_type_results.py @@ -30,6 +30,7 @@ "LoadReport", "LoadResult", "ModelIdentity", + "CapturedStream", "SpilledOutput", "SpillSpec", "TestResult", @@ -180,7 +181,12 @@ class WriteBehaviorSpec: @dataclass(frozen=True, slots=True) class SpillSpec: - """Character budgets for lossless artifact-backed output previews.""" + """Character budgets for lossless artifact-backed output previews. + + Dual denomination: ``spill_output`` uses ``inline_max_chars`` as a character + threshold; ``summarize_capture`` uses it as a byte threshold (identical for + ASCII; at most more conservative for multibyte). + """ inline_max_chars: int = 5000 head_chars: int = 2500 @@ -191,6 +197,19 @@ def __post_init__(self) -> None: raise ValueError("spill character budgets must be non-negative") +@dataclass(frozen=True, slots=True) +class CapturedStream: + """Streaming capture result — bounded slices only, never a full read.""" + + path: Path + total_bytes: int + sha256: str + inline_text: str | None + head: str + tail: str + complete: bool + + @dataclass(frozen=True, slots=True) class SpilledOutput: """Lossless spill result with a bounded inline representation.""" diff --git a/src/autoskillit/core/types/_type_subprocess.py b/src/autoskillit/core/types/_type_subprocess.py index 332a81c7a..e69b762d7 100644 --- a/src/autoskillit/core/types/_type_subprocess.py +++ b/src/autoskillit/core/types/_type_subprocess.py @@ -129,6 +129,8 @@ class SubprocessResult: ``SubprocessRunner.__call__`` and the callback completes before process exit. Consumed downstream to annotate termination provenance. """ + stdout_path: Path | None = None + stderr_path: Path | None = None @runtime_checkable @@ -177,4 +179,5 @@ def __call__( workload_basenames: frozenset[str] | None = None, on_session_id_resolved: Callable[[str], None] | None = None, child_deferral_ceiling: float = 0.0, + capture_dir: Path | None = None, ) -> Awaitable[SubprocessResult]: ... diff --git a/src/autoskillit/execution/__init__.py b/src/autoskillit/execution/__init__.py index fab7328d1..34ce6816e 100644 --- a/src/autoskillit/execution/__init__.py +++ b/src/autoskillit/execution/__init__.py @@ -82,12 +82,15 @@ partition_files_by_domain, ) from autoskillit.execution.process import ( + CaptureReadError, + CaptureSetupError, DefaultSubprocessRunner, _has_active_execution_marker, # noqa: F401 — re-exported for cli/app.py signal guard async_kill_process_tree, kill_process_tree, run_managed_async, run_managed_sync, + summarize_capture, ) from autoskillit.execution.quota import ( QUOTA_CACHE_SCHEMA_VERSION, @@ -146,9 +149,12 @@ "CmdSpec", "ClaudeHeadlessCmd", # process + "CaptureReadError", + "CaptureSetupError", "DefaultSubprocessRunner", "run_managed_async", "run_managed_sync", + "summarize_capture", # recording "RecordingSubprocessRunner", "ReplayingSubprocessRunner", diff --git a/src/autoskillit/execution/process/__init__.py b/src/autoskillit/execution/process/__init__.py index d38654568..832fa0cb0 100644 --- a/src/autoskillit/execution/process/__init__.py +++ b/src/autoskillit/execution/process/__init__.py @@ -31,7 +31,13 @@ get_logger, read_starttime_ticks, ) -from autoskillit.execution.process._process_io import create_temp_io, read_temp_output +from autoskillit.execution.process._process_io import ( + CaptureReadError, + CaptureSetupError, + create_temp_io, + read_temp_output, + summarize_capture, +) from autoskillit.execution.process._process_jsonl import ( _jsonl_contains_marker, _jsonl_has_record_type, @@ -96,6 +102,8 @@ "_watch_process", "_watch_session_log", "async_kill_process_tree", + "CaptureReadError", + "CaptureSetupError", "create_temp_io", "decide_termination_action", "execute_termination_action", @@ -105,6 +113,7 @@ "resolve_termination", "run_managed_async", "run_managed_sync", + "summarize_capture", ] @@ -262,6 +271,7 @@ async def run_managed_async( workload_basenames: frozenset[str] | None = None, on_session_id_resolved: Callable[[str], None] | None = None, child_deferral_ceiling: float = 0.0, + capture_dir: Path | None = None, ) -> SubprocessResult: """Async subprocess execution with temp file I/O and process tree cleanup. @@ -280,7 +290,12 @@ async def run_managed_async( if pty_mode: cmd = pty_wrap_command(cmd) - with create_temp_io(input_data) as (stdout_file, stderr_file, stdin_path): + _keep = capture_dir is not None + with create_temp_io(input_data, capture_dir=capture_dir, keep_streams=_keep) as ( + stdout_file, + stderr_file, + stdin_path, + ): stdout_path = Path(stdout_file.name) stderr_path = Path(stderr_file.name) @@ -528,7 +543,15 @@ async def run_managed_async( stdout_file.close() stderr_file.close() - stdout, stderr = read_temp_output(stdout_path, stderr_path) + if capture_dir is not None: + stdout = "" + stderr = "" + _stdout_path = stdout_path + _stderr_path = stderr_path + else: + stdout, stderr = read_temp_output(stdout_path, stderr_path) + _stdout_path = None + _stderr_path = None sub_result = SubprocessResult( returncode=proc.returncode if proc.returncode is not None else -1, @@ -546,6 +569,8 @@ async def run_managed_async( tracked_comm=_tracked_comm, orphaned_tool_result=signals.channel_b_orphaned_tool_result, inspector_verdict=signals.inspector_verdict, + stdout_path=_stdout_path, + stderr_path=_stderr_path, ) proc_log.debug( "run_managed_async_result", @@ -583,13 +608,19 @@ def run_managed_sync( timeout: float, input_data: str | None = None, env: Mapping[str, str] | None = None, + capture_dir: Path | None = None, ) -> SubprocessResult: """Sync subprocess execution with temp file I/O and process tree cleanup. Same composition pattern as run_managed_async but uses subprocess.Popen with start_new_session=True. No channel monitoring — wall-clock timeout only. """ - with create_temp_io(input_data) as (stdout_file, stderr_file, stdin_path): + _keep = capture_dir is not None + with create_temp_io(input_data, capture_dir=capture_dir, keep_streams=_keep) as ( + stdout_file, + stderr_file, + stdin_path, + ): stdout_path = Path(stdout_file.name) stderr_path = Path(stderr_file.name) @@ -628,7 +659,15 @@ def run_managed_sync( stdout_file.close() stderr_file.close() - stdout, stderr = read_temp_output(stdout_path, stderr_path) + if capture_dir is not None: + stdout = "" + stderr = "" + _stdout_path = stdout_path + _stderr_path = stderr_path + else: + stdout, stderr = read_temp_output(stdout_path, stderr_path) + _stdout_path = None + _stderr_path = None return SubprocessResult( returncode=process.returncode if process.returncode is not None else -1, @@ -637,6 +676,8 @@ def run_managed_sync( termination=termination, pid=process.pid, channel_confirmation=ChannelConfirmation.UNMONITORED, + stdout_path=_stdout_path, + stderr_path=_stderr_path, ) except Exception: if process is not None and process.returncode is None: @@ -681,6 +722,7 @@ async def __call__( workload_basenames: frozenset[str] | None = None, on_session_id_resolved: Callable[[str], None] | None = None, child_deferral_ceiling: float = 0.0, + capture_dir: Path | None = None, ) -> SubprocessResult: return await run_managed_async( cmd, @@ -708,4 +750,5 @@ async def __call__( inspector_callback=inspector_callback, workload_basenames=workload_basenames, on_session_id_resolved=on_session_id_resolved, + capture_dir=capture_dir, ) diff --git a/src/autoskillit/execution/process/_process_io.py b/src/autoskillit/execution/process/_process_io.py index fc0d3ec26..b66a45e26 100644 --- a/src/autoskillit/execution/process/_process_io.py +++ b/src/autoskillit/execution/process/_process_io.py @@ -2,26 +2,42 @@ from __future__ import annotations +import hashlib import tempfile from collections.abc import Generator from contextlib import contextmanager from pathlib import Path from typing import IO -from autoskillit.core import get_logger +from autoskillit.core import CapturedStream, SpillSpec, get_logger logger = get_logger(__name__) +class CaptureSetupError(OSError): + """Raised when the capture directory cannot be created.""" + + +class CaptureReadError(OSError): + """Raised when a capture file cannot be read after execution.""" + + @contextmanager def create_temp_io( input_data: str | None = None, + capture_dir: Path | None = None, + keep_streams: bool = False, ) -> Generator[tuple[IO[bytes], IO[bytes], Path | None], None, None]: """Context manager yielding temp file paths for subprocess I/O. Creates temp files for stdout and stderr (and optionally stdin). Cleans up on exit regardless of success/failure. + When *capture_dir* is provided, stdout/stderr files are created inside that + directory (which is created with ``parents=True`` if needed) and + *keep_streams* defaults to ``True`` semantics — the stream files are NOT + deleted on exit (stdin is always deleted). + Yields: Tuple of (stdout_file, stderr_file, stdin_path_or_None) where stdout_file and stderr_file are open file handles ready to pass @@ -33,15 +49,42 @@ def create_temp_io( paths_to_clean: list[Path] = [] try: - stdout_file = tempfile.NamedTemporaryFile( - mode="w+b", prefix="proc_stdout_", suffix=".tmp", delete=False - ) - paths_to_clean.append(Path(stdout_file.name)) - - stderr_file = tempfile.NamedTemporaryFile( - mode="w+b", prefix="proc_stderr_", suffix=".tmp", delete=False - ) - paths_to_clean.append(Path(stderr_file.name)) + if capture_dir is not None: + try: + capture_dir.mkdir(parents=True, exist_ok=True) + except OSError as exc: + raise CaptureSetupError( + f"Cannot create capture directory {capture_dir}: {exc}" + ) from exc + + _dir = str(capture_dir) if capture_dir is not None else None + try: + stdout_file = tempfile.NamedTemporaryFile( + mode="w+b", + prefix="proc_stdout_", + suffix=".tmp", + delete=False, + dir=_dir, + ) + except OSError as exc: + raise CaptureSetupError(f"Cannot create stdout temp file in {_dir}: {exc}") from exc + if not keep_streams and capture_dir is None: + paths_to_clean.append(Path(stdout_file.name)) + + try: + stderr_file = tempfile.NamedTemporaryFile( + mode="w+b", + prefix="proc_stderr_", + suffix=".tmp", + delete=False, + dir=_dir, + ) + except OSError as exc: + stdout_file.close() + Path(stdout_file.name).unlink(missing_ok=True) + raise CaptureSetupError(f"Cannot create stderr temp file in {_dir}: {exc}") from exc + if not keep_streams and capture_dir is None: + paths_to_clean.append(Path(stderr_file.name)) if input_data is not None: stdin_file = tempfile.NamedTemporaryFile( @@ -56,7 +99,6 @@ def create_temp_io( yield stdout_file, stderr_file, stdin_path finally: - # Close file handles if still open for f in (stdout_file, stderr_file): if f is not None: try: @@ -64,7 +106,6 @@ def create_temp_io( except OSError: pass - # Delete temp files for p in paths_to_clean: try: p.unlink(missing_ok=True) @@ -89,3 +130,64 @@ def read_temp_output(stdout_path: Path, stderr_path: Path) -> tuple[str, str]: except OSError: logger.warning("Failed to read stderr temp file: %s", stderr_path) return stdout, stderr + + +_HASH_CHUNK = 65536 + + +def summarize_capture( + path: Path, + spec: SpillSpec, + *, + complete: bool = True, +) -> CapturedStream: + """Streaming capture summary — bounded slices only, never a full read.""" + try: + total_bytes = path.stat().st_size + except OSError as exc: + raise CaptureReadError(f"Cannot stat capture file {path}: {exc}") from exc + + try: + h = hashlib.sha256() + with path.open("rb") as f: + while True: + chunk = f.read(_HASH_CHUNK) + if not chunk: + break + h.update(chunk) + sha256 = h.hexdigest() + except OSError as exc: + raise CaptureReadError(f"Cannot hash capture file {path}: {exc}") from exc + + if total_bytes <= spec.inline_max_chars: + try: + inline_text = path.read_text(errors="replace") + except OSError as exc: + raise CaptureReadError(f"Cannot read capture file {path}: {exc}") from exc + return CapturedStream( + path=path, + total_bytes=total_bytes, + sha256=sha256, + inline_text=inline_text, + head="", + tail="", + complete=complete, + ) + + try: + with path.open("rb") as f: + head_bytes = f.read(spec.head_chars) + f.seek(max(0, total_bytes - spec.tail_chars)) + tail_bytes = f.read(spec.tail_chars) + except OSError as exc: + raise CaptureReadError(f"Cannot read capture slices from {path}: {exc}") from exc + + return CapturedStream( + path=path, + total_bytes=total_bytes, + sha256=sha256, + inline_text=None, + head=head_bytes.decode("utf-8", errors="replace"), + tail=tail_bytes.decode("utf-8", errors="replace"), + complete=complete, + ) diff --git a/src/autoskillit/execution/recording.py b/src/autoskillit/execution/recording.py index 55a18fb8d..b723d833b 100644 --- a/src/autoskillit/execution/recording.py +++ b/src/autoskillit/execution/recording.py @@ -143,6 +143,7 @@ async def __call__( workload_basenames: frozenset[str] | None = None, on_session_id_resolved: Callable[[str], None] | None = None, child_deferral_ceiling: float = 0.0, + capture_dir: Path | None = None, ) -> SubprocessResult: step_name = (env or {}).get(SCENARIO_STEP_NAME_ENV, "") @@ -180,6 +181,7 @@ async def __call__( completion_record_types=completion_record_types, session_record_types=session_record_types, child_deferral_ceiling=child_deferral_ceiling, + capture_dir=capture_dir, ) # Non-Codex, non-PTY with step_name: run inner + record summary. @@ -206,13 +208,15 @@ async def __call__( completion_record_types=completion_record_types, session_record_types=session_record_types, child_deferral_ceiling=child_deferral_ceiling, + capture_dir=capture_dir, ) + _head = (result.stdout or "")[:500] if not capture_dir else "(capture mode)" self.recorder.record_non_session_step( step_name=step_name, tool="run_cmd", result_summary={ "exit_code": result.returncode, - "stdout_head": (result.stdout or "")[:500], + "stdout_head": _head, }, ) return result @@ -241,6 +245,7 @@ async def __call__( completion_record_types=completion_record_types, session_record_types=session_record_types, child_deferral_ceiling=child_deferral_ceiling, + capture_dir=capture_dir, ) async def _record_session( @@ -317,6 +322,7 @@ async def _record_non_pty_session( completion_record_types: frozenset[str] = frozenset({"result"}), session_record_types: frozenset[str] = frozenset({"assistant"}), child_deferral_ceiling: float = 0.0, + capture_dir: Path | None = None, ) -> SubprocessResult: """Record a non-PTY (Codex) session step via cassette files.""" result = await self._inner( @@ -342,6 +348,7 @@ async def _record_non_pty_session( completion_record_types=completion_record_types, session_record_types=session_record_types, child_deferral_ceiling=child_deferral_ceiling, + capture_dir=capture_dir, ) if self._scenario_dir is None: @@ -440,6 +447,7 @@ async def __call__( workload_basenames: frozenset[str] | None = None, on_session_id_resolved: Callable[[str], None] | None = None, child_deferral_ceiling: float = 0.0, + capture_dir: Path | None = None, ) -> SubprocessResult: step_name = (env or {}).get(SCENARIO_STEP_NAME_ENV, "") diff --git a/src/autoskillit/hook_registry.py b/src/autoskillit/hook_registry.py index 532efdfcf..0899d80fe 100644 --- a/src/autoskillit/hook_registry.py +++ b/src/autoskillit/hook_registry.py @@ -30,7 +30,7 @@ class HookDef: codex_status: Literal["works-as-is", "degraded", "fix-required", "not-applicable"] = ( "works-as-is" ) - mechanism: Literal["deny", "additionalContext", "output-rewrite"] = "deny" + mechanism: Literal["deny", "additionalContext", "output-rewrite", "input-rewrite"] = "deny" enforcement_strength: dict[str, str] = field(default_factory=dict) def __post_init__(self) -> None: @@ -64,7 +64,7 @@ def __post_init__(self) -> None: # artifact_download_guard | works-as-is # git_ops_guard | works-as-is # test_runner_guard | works-as-is -# output_budget_guard | works-as-is +# shell_capture_hook | works-as-is (Codex-only input-rewrite, #4286/ADR-0006) # generated_file_write_guard | works-as-is # write_guard | works-as-is # planner_result_naming_guard | works-as-is @@ -202,13 +202,14 @@ def __post_init__(self) -> None: enforcement_strength={"claude_code": "soft", "codex": "works-as-is"}, ), HookDef( - matcher=r"Bash|mcp__.*autoskillit.*__run_cmd", - scripts=["guards/output_budget_guard.py"], + matcher=r"Bash", + scripts=["shell_capture_hook.py"], session_scope="any", - exempt_skills=frozenset(), codex_status="works-as-is", - mechanism="deny", - enforcement_strength={"claude_code": "soft", "codex": "works-as-is"}, + mechanism="input-rewrite", + # In-script Codex gate (#4286 / ADR-0006): exits 0 on non-Codex sessions. + # Matcher excludes mcp__.*run_cmd — that channel is lossless server-side. + enforcement_strength={"claude_code": "not-applicable", "codex": "works-as-is"}, ), HookDef( matcher=r"Write|Edit", @@ -402,6 +403,7 @@ def __post_init__(self) -> None: "unsafe_install_guard.py", "write_guard.py", "pretty_output_hook.py", + "output_budget_guard.py", # Append any future retired basenames here, atomically with the rename commit. } ) @@ -423,7 +425,6 @@ def __post_init__(self) -> None: "git_ops_guard.py", "compose_pr_body_guard.py", "test_runner_guard.py", - "output_budget_guard.py", "fleet_claim_guard.py", "reset_resume_gate.py", "recipe_read_guard.py", diff --git a/src/autoskillit/hooks/AGENTS.md b/src/autoskillit/hooks/AGENTS.md index 71a788d3a..2c29cf331 100644 --- a/src/autoskillit/hooks/AGENTS.md +++ b/src/autoskillit/hooks/AGENTS.md @@ -23,6 +23,8 @@ Sub-packages: guards/ (see guards/AGENTS.md), formatters/ (see formatters/AGENTS | `ingredient_lock_guard.py` | PreToolUse guard script (see guards/AGENTS.md) | | `_hook_utils.py` | Shared stdlib-only utilities for hook scripts (e.g., `find_project_root`) | | `_command_classification.py` | Shared stdlib-only command classification primitives for guard scripts (interpreter/wrapper detection, git command classification) | +| `_policy_event.py` | Typed policy-event formatter for hook provenance messages (stdlib-only) | +| `shell_capture_hook.py` | `PreToolUse`: input-rewrite hook for Codex shell capture — wraps commands in a lossless capture harness (#4286 / ADR-0006) | ## Architecture Notes diff --git a/src/autoskillit/hooks/_command_classification.py b/src/autoskillit/hooks/_command_classification.py index d563e6130..91b601d43 100644 --- a/src/autoskillit/hooks/_command_classification.py +++ b/src/autoskillit/hooks/_command_classification.py @@ -11,8 +11,7 @@ import os import re import shlex -from collections.abc import Callable, Sequence -from enum import Enum +from collections.abc import Sequence from typing import Protocol _INTERPRETER_RE = re.compile( @@ -225,21 +224,6 @@ _PROTECTED_READ_SHELL_OPS: frozenset[str] = frozenset({"&&", "||", ";", "|", "&"}) _WC_FLAG_RE = re.compile(r"-l+|--lines$") -OUTPUT_BUDGET_MAX_COMMAND_CHARS = 65_536 -OUTPUT_BUDGET_MAX_NESTING_DEPTH = 8 - - -class CommandBudgetDisposition(Enum): - BOUNDED = "bounded" - HAZARDOUS = "hazardous" - UNKNOWN = "unknown" - - -# A producer classifier returns whether the producer's stdout and stderr are -# intrinsically bounded, respectively. Returning None means the segment is -# outside the caller's risky-producer policy. -OutputBudgetProducerClassifier = Callable[[list[str]], tuple[bool, bool] | None] - class SearchPattern(Protocol): def search(self, string: str, /): ... @@ -328,369 +312,6 @@ def tokenize_command_segments(command: str) -> list[list[str]]: return segments -def tokenize_command_structure(command: str) -> list[tuple[str, list[str]]] | None: - """Tokenize shell segments while retaining the connector before each segment.""" - if len(command) > OUTPUT_BUDGET_MAX_COMMAND_CHARS: - return None - try: - stripped = _HEREDOC_MARKER_RE.sub(r"\2", strip_heredoc_bodies(command)) - lexer = shlex.shlex( - _normalize_newlines_for_tokenize(stripped), posix=True, punctuation_chars=";&|" - ) - lexer.whitespace_split = True - raw_tokens = list(lexer) - except (ValueError, TypeError): - return None - tokens: list[str] = [] - i = 0 - while i < len(raw_tokens): - token = raw_tokens[i] - # shlex splits the ampersand in descriptor duplication (2>&1) because - # ampersand is also shell punctuation. Recombine only when the token - # before it is a redirect operator and the target is a literal fd. - if ( - re.fullmatch(r"\d*>{1,2}", token) - and i + 2 < len(raw_tokens) - and raw_tokens[i + 1] == "&" - and re.fullmatch(r"\d+|-", raw_tokens[i + 2]) - ): - tokens.append(f"{token}&{raw_tokens[i + 2]}") - i += 3 - continue - tokens.append(token) - i += 1 - result: list[tuple[str, list[str]]] = [] - current: list[str] = [] - connector = "" - for token in tokens: - if token in {"&&", "||", ";", "|", "|&", "&"}: - if current: - result.append((connector, current)) - current = [] - connector = token - else: - current.append(token) - if current: - result.append((connector, current)) - return result - - -_OUTPUT_REDIRECT_RE = re.compile(r"^(?P[012]?)(?P>{1,2})(?P.*)$") -_OUTPUT_FD_DUP_RE = re.compile(r"^(?P[012]?)(?P>{1,2})&(?P\d+|-)$") -_OUTPUT_DYNAMIC_TARGET_RE = re.compile(r"[$`*?\[\]{}]") -_OUTPUT_UNSUPPORTED_SYNTAX_RE = re.compile(r"[<>]\(|<<<|(?:[3-9]\d*)>&|>&-") -_OUTPUT_BYTE_CAP_VERBS: frozenset[str] = frozenset({"head", "tail"}) - -_DEST_MODEL = "model" -_DEST_PIPE = "pipe" -_DEST_SAFE = "safe" -_DEST_UNSAFE = "unsafe" -_DEST_UNKNOWN = "unknown" - - -def _is_project_temp_target(target: str, cwd: str) -> bool: - if target == "/dev/null": - return True - if not target or _OUTPUT_DYNAMIC_TARGET_RE.search(target): - return False - root = os.path.realpath(cwd or os.getcwd()) - temp_root = os.path.join(root, ".autoskillit", "temp") - resolved = os.path.realpath(target if os.path.isabs(target) else os.path.join(root, target)) - try: - return os.path.commonpath((temp_root, resolved)) == temp_root - except ValueError: - return False - - -def _redirect_target_destination(target: str, cwd: str) -> str: - if not target or _OUTPUT_DYNAMIC_TARGET_RE.search(target): - return _DEST_UNKNOWN - return _DEST_SAFE if _is_project_temp_target(target, cwd) else _DEST_UNSAFE - - -def _segment_output_destinations( - tokens: list[str], *, stdout_base: str, stderr_base: str, cwd: str -) -> tuple[str, str, list[str], bool]: - """Apply ordered stdout/stderr redirects and return remaining argv. - - The bool result reports unsupported or ambiguous descriptor syntax. The - destination copy for ``2>&1`` is intentionally immediate, matching shell - left-to-right redirection semantics. - """ - stdout = stdout_base - stderr = stderr_base - argv: list[str] = [] - unknown = False - i = 0 - while i < len(tokens): - token = tokens[i] - fd_dup = _OUTPUT_FD_DUP_RE.fullmatch(token) - if fd_dup: - fd = int(fd_dup.group("fd") or "1") - target = fd_dup.group("target") - if fd not in {1, 2} or target not in {"1", "2"}: - unknown = True - elif fd == 1: - stdout = stdout if target == "1" else stderr - else: - stderr = stdout if target == "1" else stderr - i += 1 - continue - - redirect = _OUTPUT_REDIRECT_RE.fullmatch(token) - if redirect: - fd = int(redirect.group("fd") or "1") - target = redirect.group("target") - if not target: - if i + 1 >= len(tokens): - unknown = True - i += 1 - continue - target = tokens[i + 1] - i += 1 - destination = _redirect_target_destination(target, cwd) - if fd == 1: - stdout = destination - elif fd == 2: - stderr = destination - else: - unknown = True - i += 1 - continue - - if token.startswith("<") or re.match(r"\d+<", token): - # Ordinary literal stdin redirection does not affect model-visible - # output. Dynamic/process forms were rejected before tokenization. - if _OUTPUT_DYNAMIC_TARGET_RE.search(token): - unknown = True - argv.append(token) - i += 1 - continue - argv.append(token) - i += 1 - return (stdout, stderr, argv, unknown) - - -def command_tokens_without_output_redirections(tokens: list[str]) -> list[str] | None: - """Return command argv with stdout/stderr redirects removed. - - ``None`` means descriptor syntax was ambiguous or unsupported. This is - primarily exposed for stdlib-only guards that need to classify arguments - without accidentally treating redirect targets as input paths. - """ - _stdout, _stderr, argv, unknown = _segment_output_destinations( - tokens, stdout_base=_DEST_MODEL, stderr_base=_DEST_MODEL, cwd=os.getcwd() - ) - return None if unknown else argv - - -def _byte_cap_value(argv: list[str], max_inline_bytes: int) -> int | None: - verb, args = command_verb_and_args(argv) - if os.path.basename(verb) not in _OUTPUT_BYTE_CAP_VERBS: - return None - value: str | None = None - for index, arg in enumerate(args): - if arg in {"-c", "--bytes"} and index + 1 < len(args): - value = args[index + 1] - break - if arg.startswith("-c") and len(arg) > 2: - value = arg[2:] - break - if arg.startswith("--bytes="): - value = arg.partition("=")[2] - break - if value is None or not value.isdigit(): - return None - parsed = int(value) - return parsed if 0 < parsed <= max_inline_bytes else None - - -def _combine_budget_dispositions( - left: CommandBudgetDisposition, right: CommandBudgetDisposition -) -> CommandBudgetDisposition: - if CommandBudgetDisposition.UNKNOWN in {left, right}: - return CommandBudgetDisposition.UNKNOWN - if CommandBudgetDisposition.HAZARDOUS in {left, right}: - return CommandBudgetDisposition.HAZARDOUS - return CommandBudgetDisposition.BOUNDED - - -def _trace_pipeline_destination( - parsed: list[tuple[str, list[str]]], - producer_index: int, - destination: str, - *, - max_inline_bytes: int, - cwd: str, - input_already_bounded: bool = False, -) -> CommandBudgetDisposition: - if destination == _DEST_SAFE: - return CommandBudgetDisposition.BOUNDED - if destination in {_DEST_MODEL, _DEST_UNSAFE}: - return CommandBudgetDisposition.HAZARDOUS - if destination == _DEST_UNKNOWN: - return CommandBudgetDisposition.UNKNOWN - - # The content is on a pipeline. It remains unbounded until a terminal - # byte cap; every intermediate stderr must stay in that pipeline or go to - # an approved sink, otherwise that branch remains model-visible. - index = producer_index + 1 - while index < len(parsed) and parsed[index][0] in {"|", "|&"}: - connector, tokens = parsed[index] - next_connector = parsed[index + 1][0] if index + 1 < len(parsed) else "" - has_next_pipe = next_connector in {"|", "|&"} - stdout_base = _DEST_PIPE if has_next_pipe else _DEST_MODEL - stderr_base = _DEST_PIPE if has_next_pipe and next_connector == "|&" else _DEST_MODEL - stdout, stderr, argv, unknown = _segment_output_destinations( - tokens, stdout_base=stdout_base, stderr_base=stderr_base, cwd=cwd - ) - if unknown or os.path.basename(command_verb(argv)) == "tee": - return CommandBudgetDisposition.UNKNOWN - cap = _byte_cap_value(argv, max_inline_bytes) - if cap is not None: - # A downstream transform can amplify a capped stream, so only a - # terminal cap (or one writing directly to a safe sink) proves the - # model-visible response bound. - if has_next_pipe: - return CommandBudgetDisposition.UNKNOWN - if stdout == _DEST_UNKNOWN: - return CommandBudgetDisposition.UNKNOWN - return ( - CommandBudgetDisposition.BOUNDED - if stdout in {_DEST_MODEL, _DEST_SAFE} - else CommandBudgetDisposition.HAZARDOUS - ) - if input_already_bounded: - # An arbitrary transform may amplify even a small source record. - # Without a terminal byte cap or safe-file sink its output bound is - # not statically knowable. - return ( - CommandBudgetDisposition.BOUNDED - if stdout == _DEST_SAFE - else CommandBudgetDisposition.UNKNOWN - ) - if stdout == _DEST_SAFE: - return CommandBudgetDisposition.BOUNDED - if stdout != _DEST_PIPE: - return ( - CommandBudgetDisposition.UNKNOWN - if stdout == _DEST_UNKNOWN - else CommandBudgetDisposition.HAZARDOUS - ) - if stderr not in {_DEST_PIPE, _DEST_SAFE}: - return ( - CommandBudgetDisposition.UNKNOWN - if stderr == _DEST_UNKNOWN - else CommandBudgetDisposition.HAZARDOUS - ) - index += 1 - return CommandBudgetDisposition.HAZARDOUS - - -def _classify_parsed_output_budget( - parsed: list[tuple[str, list[str]]], - producer_classifier: OutputBudgetProducerClassifier, - *, - max_inline_bytes: int, - cwd: str, -) -> CommandBudgetDisposition: - profiles = [producer_classifier(tokens) for _connector, tokens in parsed] - if not any(profile is not None for profile in profiles): - return CommandBudgetDisposition.BOUNDED - - disposition = CommandBudgetDisposition.BOUNDED - for index, profile in enumerate(profiles): - if profile is None: - continue - if any(os.path.basename(command_verb(tokens)) == "tee" for _c, tokens in parsed): - return CommandBudgetDisposition.UNKNOWN - connector_after = parsed[index + 1][0] if index + 1 < len(parsed) else "" - stdout_base = _DEST_PIPE if connector_after in {"|", "|&"} else _DEST_MODEL - stderr_base = _DEST_PIPE if connector_after == "|&" else _DEST_MODEL - stdout, stderr, _argv, unknown = _segment_output_destinations( - parsed[index][1], stdout_base=stdout_base, stderr_base=stderr_base, cwd=cwd - ) - if unknown: - return CommandBudgetDisposition.UNKNOWN - stdout_intrinsic, stderr_intrinsic = profile - for stream_intrinsic, destination in ( - (stdout_intrinsic, stdout), - (stderr_intrinsic, stderr), - ): - if stream_intrinsic: - stream_disposition = ( - _trace_pipeline_destination( - parsed, - index, - destination, - max_inline_bytes=max_inline_bytes, - cwd=cwd, - input_already_bounded=True, - ) - if destination == _DEST_PIPE - else CommandBudgetDisposition.BOUNDED - ) - else: - stream_disposition = _trace_pipeline_destination( - parsed, - index, - destination, - max_inline_bytes=max_inline_bytes, - cwd=cwd, - ) - disposition = _combine_budget_dispositions(disposition, stream_disposition) - return disposition - - -def classify_command_output_budget( - command: str, - producer_classifier: OutputBudgetProducerClassifier, - *, - max_inline_bytes: int, - cwd: str = "", - _depth: int = 0, -) -> CommandBudgetDisposition: - """Classify whether every risky producer has a proven bounded output path. - - The caller supplies the producer policy so this shared parser remains - independent of any one guard. Commands outside that policy are BOUNDED; - malformed, over-limit, deeply nested, or unsupported shell structures are - UNKNOWN and therefore cannot establish a guard exception. - """ - if ( - _depth > OUTPUT_BUDGET_MAX_NESTING_DEPTH - or len(command) > OUTPUT_BUDGET_MAX_COMMAND_CHARS - or max_inline_bytes <= 0 - ): - return CommandBudgetDisposition.UNKNOWN - if _OUTPUT_UNSUPPORTED_SYNTAX_RE.search(command): - return CommandBudgetDisposition.UNKNOWN - parsed = tokenize_command_structure(command) - if parsed is None or (not parsed and command.strip()): - return CommandBudgetDisposition.UNKNOWN - if any(connector == "&" for connector, _tokens in parsed): - return CommandBudgetDisposition.UNKNOWN - - root = cwd or os.getcwd() - disposition = _classify_parsed_output_budget( - parsed, producer_classifier, max_inline_bytes=max_inline_bytes, cwd=root - ) - seen: set[str] = set() - for payload in extract_shell_command_payloads(command): - if payload in seen: - continue - seen.add(payload) - child = classify_command_output_budget( - payload, - producer_classifier, - max_inline_bytes=max_inline_bytes, - cwd=root, - _depth=_depth + 1, - ) - disposition = _combine_budget_dispositions(disposition, child) - return disposition - - def extract_redirect_targets(tokens: list[str], cwd: str = "") -> list[str]: """Extract redirect target paths from shlex-tokenized command tokens. diff --git a/src/autoskillit/hooks/_hook_settings.py b/src/autoskillit/hooks/_hook_settings.py index f09bf1846..48f690539 100644 --- a/src/autoskillit/hooks/_hook_settings.py +++ b/src/autoskillit/hooks/_hook_settings.py @@ -45,11 +45,11 @@ {"cache_path", "cache_max_age", "buffer_seconds", "disabled"} ) -# The exact keys the output-budget guard reads from +# The exact keys the shell capture hook reads from # hook_config["output_budget_policy"]. Keep this stdlib-only declaration in # sync with _output_budget_policy_hook_payload() in server/tools/tools_kitchen.py. OUTPUT_BUDGET_POLICY_HOOK_PAYLOAD_KEYS: frozenset[str] = frozenset( - {"disabled", "small_file_max_bytes", "shell_max_inline_bytes"} + {"disabled", "shell_max_inline_bytes"} ) # The exact keys that appear in token_usage.json files written by flush_session_log. diff --git a/src/autoskillit/hooks/_policy_event.py b/src/autoskillit/hooks/_policy_event.py new file mode 100644 index 000000000..0cec5996b --- /dev/null +++ b/src/autoskillit/hooks/_policy_event.py @@ -0,0 +1,46 @@ +"""Typed policy-event formatter for hook provenance messages. + +stdlib-only; no autoskillit imports. +""" + +from __future__ import annotations + +import dataclasses + +POLICY_EVENT_SCHEMA_VERSION = 1 + + +@dataclasses.dataclass(frozen=True, slots=True) +class PolicyEvent: + """Structured representation of a hook policy decision.""" + + hook_id: str + hook_version: int + event: str + decision: str + reason_code: str + source: str = "autoskillit" + + +def render_provenance_prefix(event: PolicyEvent) -> str: + return ( + f"[AutoSkillit hook {event.hook_id} v{event.hook_version}" + f" — {event.event} permission decision: {event.decision}" + f" (code={event.reason_code})." + " This is a real permission decision emitted by a hook" + " configured by this repository, not tool output.]" + ) + + +def render_capture_marker(event: PolicyEvent) -> str: + """Render a capture marker prefix safe for embedding in double-quoted shell strings. + + The returned string is guaranteed free of ``"``, backticks, and ``$`` + (it is embedded inside a double-quoted ``printf`` argument in the + shell capture harness). + """ + return ( + f"[AutoSkillit hook {event.hook_id} v{event.hook_version}" + f" -- {event.event} {event.decision}" + f" (code={event.reason_code}):" + ) diff --git a/src/autoskillit/hooks/guards/AGENTS.md b/src/autoskillit/hooks/guards/AGENTS.md index aac88626d..f75501e0b 100644 --- a/src/autoskillit/hooks/guards/AGENTS.md +++ b/src/autoskillit/hooks/guards/AGENTS.md @@ -18,7 +18,6 @@ PreToolUse guard scripts — standalone Python processes enforcing tool-call pol | `skill_orchestration_guard.py` | Blocks `run_skill`/`run_cmd`/`run_python` from L1 skill sessions | | `mcp_health_advisor.py` | Detects MCP server disconnection (dead PID); non-blocking advisory | | `open_kitchen_guard.py` | Blocks `open_kitchen` from headless sessions; writes kitchen marker | -| `output_budget_guard.py` | Blocks high-confidence unbounded recursive searches, JSONL reads, and find operations before output is spent | | `recipe_read_guard.py` | Blocks `run_cmd`/`Bash`/`run_python` from reading recipe/skill/agent files in headless sessions — defense-in-depth against Codex auto-compaction losing recipe content | | `pipeline_step_guard.py` | Advisory unmet-dependency warning for `run_skill`; server-side `_check_pipeline_deps` is primary enforcer | | `planner_result_naming_guard.py` | Blocks Write/Edit with non-canonical planner result filenames (e.g. `P1-A1-WP2a_result.json`); denies with correction hint | diff --git a/src/autoskillit/hooks/guards/output_budget_guard.py b/src/autoskillit/hooks/guards/output_budget_guard.py deleted file mode 100644 index 28c0a6edb..000000000 --- a/src/autoskillit/hooks/guards/output_budget_guard.py +++ /dev/null @@ -1,349 +0,0 @@ -#!/usr/bin/env python3 -"""PreToolUse guard for high-confidence unbounded shell output shapes. - -The guard is deliberately enumerated: recursive search (R1), JSONL producers -(R2), and directory traversal with find (R3). Shell syntax and descriptor -flow are delegated to the shared stdlib-only command classifier. - -stdlib-only; no autoskillit imports. -""" - -from __future__ import annotations - -import json -import os -import re -import sys -from pathlib import Path - -_HOOKS_DIR = str(Path(__file__).resolve().parent.parent) -if _HOOKS_DIR not in sys.path: - sys.path.insert(0, _HOOKS_DIR) - -from _command_classification import ( # type: ignore[import-not-found] # noqa: E402 - CommandBudgetDisposition, - classify_command_output_budget, - command_tokens_without_output_redirections, - command_verb_and_args, -) -from _hook_settings import read_merged_hook_config # type: ignore[import-not-found] # noqa: E402 - -OUTPUT_BUDGET_DENY_TRIGGER: str = "Unbounded command output is prohibited" - -# Must stay in sync with the output_budget_guard HookDef in hook_registry.py. -_EXEMPT_SKILLS: frozenset[str] = frozenset() - -_DEFAULT_SMALL_FILE_MAX_BYTES = 5_000 -_DEFAULT_SHELL_MAX_INLINE_BYTES = 12_000 -_JSONL_PRODUCERS: frozenset[str] = frozenset({"cat", "rg", "grep", "sed", "awk", "jq"}) -_GLOB_CHARS = frozenset("*?[]{}") -_RISKY_SURFACE_RE = re.compile(r"\b(?:rg|grep|cat|sed|awk|jq|find|wc)\b") - -_RG_FLAGS_WITH_VALUE: frozenset[str] = frozenset( - { - "-e", - "--regexp", - "-f", - "--file", - "-g", - "--glob", - "--iglob", - "-t", - "--type", - "-T", - "--type-not", - "-M", - "--max-columns", - "-m", - "--max-count", - "-A", - "--after-context", - "-B", - "--before-context", - "-C", - "--context", - "--encoding", - "--engine", - "--max-depth", - "--path-separator", - "--sort", - "--sortr", - "--threads", - "--type-add", - "--type-clear", - } -) - - -def _positive_int(value: object, default: int) -> int: - return ( - value if isinstance(value, int) and not isinstance(value, bool) and value > 0 else default - ) - - -def _read_policy() -> tuple[bool, int, int]: - try: - config = read_merged_hook_config() - section = config.get("output_budget_policy", {}) - if not isinstance(section, dict): - section = {} - except (OSError, AttributeError, TypeError, json.JSONDecodeError): - section = {} - return ( - section.get("disabled") is True, - _positive_int(section.get("small_file_max_bytes"), _DEFAULT_SMALL_FILE_MAX_BYTES), - _positive_int(section.get("shell_max_inline_bytes"), _DEFAULT_SHELL_MAX_INLINE_BYTES), - ) - - -def _has_flag(args: list[str], *names: str) -> bool: - short_chars = frozenset(name[1] for name in names if len(name) == 2 and name.startswith("-")) - for arg in args: - if arg in names: - return True - if any(arg.startswith(f"{name}=") for name in names if name.startswith("--")): - return True - if ( - short_chars - and arg.startswith("-") - and not arg.startswith("--") - and any(c in arg[1:] for c in short_chars) - ): - return True - return False - - -def _rg_targets(args: list[str]) -> list[str]: - """Return literal rg search targets after skipping patterns and option values.""" - positionals: list[str] = [] - explicit_pattern = False - i = 0 - while i < len(args): - arg = args[i] - if arg == "--": - positionals.extend(args[i + 1 :]) - break - if arg in _RG_FLAGS_WITH_VALUE: - if arg in {"-e", "--regexp"}: - explicit_pattern = True - i += 2 - continue - if arg.startswith("--regexp=") or (arg.startswith("-e") and len(arg) > 2): - explicit_pattern = True - i += 1 - continue - if arg.startswith("-"): - i += 1 - continue - positionals.append(arg) - i += 1 - return positionals if explicit_pattern else positionals[1:] - - -def _looks_like_directory(path: str, cwd: Path) -> bool: - if path in {".", "..", "/"} or path.endswith(("/", os.sep)): - return True - if "$" in path or any(char in path for char in _GLOB_CHARS): - return True - candidate = Path(path) - if not candidate.is_absolute(): - candidate = cwd / candidate - try: - if candidate.is_dir(): - return True - except OSError: - return True - # A path with no filename suffix is conservatively directory-shaped. This - # catches not-yet-existing search roots while preserving the explicit - # non-JSONL single-file surface accepted by the protocol. - return not Path(path).suffix - - -def _is_recursive_search(verb: str, args: list[str], cwd: Path) -> bool: - if verb == "grep": - return _has_flag(args, "-r", "-R", "--recursive", "--dereference-recursive") - if verb != "rg": - return False - targets = _rg_targets(args) - return not targets or any(_looks_like_directory(target, cwd) for target in targets) - - -def _jsonl_targets(args: list[str]) -> list[str]: - return [arg for arg in args if ".jsonl" in arg.lower()] - - -def _literal_small_jsonl_files(targets: list[str], cwd: Path, max_bytes: int) -> bool: - if not targets: - return False - total = 0 - root = cwd.resolve() - for raw in targets: - if raw == "-" or "$" in raw or any(char in raw for char in _GLOB_CHARS): - return False - candidate = Path(raw) - if not candidate.is_absolute(): - candidate = root / candidate - try: - if candidate.is_symlink() or not candidate.is_file(): - return False - resolved = candidate.resolve(strict=True) - if os.path.commonpath((str(root), str(resolved))) != str(root): - return False - total += resolved.stat().st_size - except (OSError, RuntimeError, ValueError): - return False - if total > max_bytes: - return False - return True - - -def _is_plain_cat(args: list[str], targets: list[str]) -> bool: - remaining = [arg for arg in args if arg != "--"] - return bool(targets) and remaining == targets - - -def _wc_line_operands(args: list[str]) -> tuple[bool, bool, list[str]]: - """Return line-mode, option validity, and every operand after wc parsing.""" - line_mode = False - valid_options = True - operands: list[str] = [] - options_ended = False - for arg in args: - if not options_ended and arg == "--": - options_ended = True - elif not options_ended and arg in {"-l", "--lines"}: - line_mode = True - elif not options_ended and arg.startswith("-") and arg != "-": - valid_options = False - else: - operands.append(arg) - return line_mode, valid_options, operands - - -def _is_jsonl_operand(raw: str) -> bool: - return Path(raw).suffix.lower() == ".jsonl" - - -def _is_dynamic_operand(raw: str) -> bool: - return raw == "-" or "$" in raw or any(char in raw for char in _GLOB_CHARS) - - -def _producer_classifier( - tokens: list[str], *, cwd: Path, small_file_max_bytes: int -) -> tuple[bool, bool] | None: - argv = command_tokens_without_output_redirections(tokens) - if argv is None: - # The shared classifier will separately turn ambiguous descriptor - # syntax into UNKNOWN. Preserve the risky match when possible. - argv = tokens - raw_verb, args = command_verb_and_args(argv) - verb = os.path.basename(raw_verb) - if not verb: - return None - - profiles: list[tuple[bool, bool]] = [] - if _is_recursive_search(verb, args, cwd): - profiles.append((_has_flag(args, "-q", "--quiet"), False)) - - targets = _jsonl_targets(args) - if verb in _JSONL_PRODUCERS and targets: - if verb == "rg" and _has_flag(args, "-q", "--quiet"): - profiles.append((True, False)) - elif ( - verb == "cat" - and _is_plain_cat(args, targets) - and _literal_small_jsonl_files(targets, cwd, small_file_max_bytes) - ): - profiles.append((True, True)) - else: - profiles.append((False, False)) - - if verb == "wc": - line_mode, valid_options, operands = _wc_line_operands(args) - jsonl_derived = any(_is_jsonl_operand(arg) for arg in operands) - ambiguous = not operands or any(_is_dynamic_operand(arg) for arg in operands) - if line_mode and jsonl_derived: - intrinsically_bounded = ( - valid_options - and all(_is_jsonl_operand(arg) for arg in operands) - and _literal_small_jsonl_files(operands, cwd, small_file_max_bytes) - ) - profiles.append((intrinsically_bounded, intrinsically_bounded)) - elif line_mode and ambiguous: - profiles.append((False, False)) - elif jsonl_derived: - profiles.append((False, False)) - - if verb == "find" and args and _looks_like_directory(args[0], cwd): - profiles.append(("-quit" in args, False)) - - if not profiles: - return None - return (all(profile[0] for profile in profiles), all(profile[1] for profile in profiles)) - - -def _classify_command( - command: str, *, cwd: Path, small_file_max_bytes: int, shell_max_inline_bytes: int -) -> CommandBudgetDisposition: - if not _RISKY_SURFACE_RE.search(command): - return CommandBudgetDisposition.BOUNDED - - def classify(tokens: list[str]) -> tuple[bool, bool] | None: - return _producer_classifier(tokens, cwd=cwd, small_file_max_bytes=small_file_max_bytes) - - return classify_command_output_budget( - command, - classify, - max_inline_bytes=shell_max_inline_bytes, - cwd=str(cwd), - ) - - -def main() -> None: - skill_name = os.environ.get("AUTOSKILLIT_SKILL_NAME", "") - if skill_name in _EXEMPT_SKILLS: - sys.exit(0) - - try: - data = json.loads(sys.stdin.read()) - tool_input = data.get("tool_input", {}) - command = tool_input.get("command", "") or tool_input.get("cmd", "") - except (json.JSONDecodeError, AttributeError, OSError, TypeError): - sys.exit(0) - if not isinstance(command, str) or not command: - sys.exit(0) - - disabled, small_file_max_bytes, shell_max_inline_bytes = _read_policy() - if disabled: - sys.exit(0) - - disposition = _classify_command( - command, - cwd=Path.cwd(), - small_file_max_bytes=small_file_max_bytes, - shell_max_inline_bytes=shell_max_inline_bytes, - ) - if disposition is CommandBudgetDisposition.BOUNDED: - sys.exit(0) - - reason = ( - f"{OUTPUT_BUDGET_DENY_TRIGGER}: this R1-R3 producer has no proven byte-bounded " - "stdout/stderr path. Run `rg -l ... 2>&1 | head -c 4000` for bounded " - "discovery, or redirect both descriptors under `.autoskillit/temp/` then read " - "slices." - ) - payload = json.dumps( - { - "hookSpecificOutput": { - "hookEventName": "PreToolUse", - "permissionDecision": "deny", - "permissionDecisionReason": reason, - } - } - ) - sys.stdout.write(payload + "\n") - sys.exit(0) - - -if __name__ == "__main__": - main() diff --git a/src/autoskillit/hooks/registry.sha256 b/src/autoskillit/hooks/registry.sha256 index 5557c9de2..1a4fd13e4 100644 --- a/src/autoskillit/hooks/registry.sha256 +++ b/src/autoskillit/hooks/registry.sha256 @@ -1 +1 @@ -8d81e614712cca43c4ceb2ed0635ace210a5055687a19437806504115b93d46c +9e6225e994bb5e769312af5d4b4507a66b0f556369900dc95df8723e6228e6a6 diff --git a/src/autoskillit/hooks/shell_capture_hook.py b/src/autoskillit/hooks/shell_capture_hook.py new file mode 100644 index 000000000..1b6abe9d5 --- /dev/null +++ b/src/autoskillit/hooks/shell_capture_hook.py @@ -0,0 +1,164 @@ +#!/usr/bin/env python3 +"""PreToolUse input-rewrite hook for Codex native shell lossless capture. + +Rewrites every native shell command on Codex into a capture harness: the +original command runs unmodified in a subshell, its complete combined output +goes to a project artifact, and only a bounded inline slice enters context. + +Codex-only (#4286 / ADR-0006). Claude Code sessions are unaffected. +stdlib-only; no autoskillit imports. +""" + +from __future__ import annotations + +import json +import os +import shlex +import sys +from pathlib import Path +from uuid import uuid4 + +_HOOKS_DIR = str(Path(__file__).resolve().parent) +if _HOOKS_DIR not in sys.path: + sys.path.insert(0, _HOOKS_DIR) + +from _hook_settings import read_merged_hook_config # type: ignore[import-not-found] # noqa: E402 +from _policy_event import ( # type: ignore[import-not-found] # noqa: E402 + PolicyEvent, + render_capture_marker, +) + +_HARNESS_SENTINEL = "# autoskillit-shell-capture v1" +_DEFAULT_SHELL_MAX_INLINE_BYTES = 12_000 +_CAPTURE_SUBDIR = ".autoskillit/temp/shell_capture" + + +def _is_codex_session(payload: dict) -> bool: + return os.environ.get("AUTOSKILLIT_AGENT_BACKEND") == "codex" or "turn_id" in payload + + +def _positive_int(value: object, default: int) -> int: + return ( + value if isinstance(value, int) and not isinstance(value, bool) and value > 0 else default + ) + + +def _read_policy(cwd: str) -> tuple[bool, int]: + try: + config = read_merged_hook_config(root=Path(cwd)) + section = config.get("output_budget_policy", {}) + if not isinstance(section, dict): + section = {} + except (OSError, AttributeError, TypeError, json.JSONDecodeError): + section = {} + return ( + section.get("disabled") is True, + _positive_int(section.get("shell_max_inline_bytes"), _DEFAULT_SHELL_MAX_INLINE_BYTES), + ) + + +def _build_harness(command: str, cwd: str, inline_bytes: int) -> str: + uid = uuid4().hex[:8] + capture_dir = str(Path(cwd) / _CAPTURE_SUBDIR) + capture_file = str(Path(cwd) / _CAPTURE_SUBDIR / f"shell_{uid}.log") + head = (2 * inline_bytes) // 3 + tail = inline_bytes - head + + marker_event = PolicyEvent( + hook_id="shell_capture_hook", + hook_version=1, + event="PreToolUse", + decision="input rewrite", + reason_code="SHELL_OUTPUT_CAPTURED", + ) + marker_prefix = render_capture_marker(marker_event) + + fail_event = PolicyEvent( + hook_id="shell_capture_hook", + hook_version=1, + event="PreToolUse", + decision="deny", + reason_code="CAPTURE_FAILED", + ) + fail_msg = render_capture_marker(fail_event) + " cannot create capture directory]" + + if not command.endswith("\n"): + command += "\n" + + q_dir = shlex.quote(capture_dir) + q_file = shlex.quote(capture_file) + + sha_line = ( + " if command -v sha256sum >/dev/null 2>&1; then" + ' __as_sha=$(sha256sum "$__as_f" | cut -d" " -f1);' + " else __as_sha=unavailable; fi" + ) + marker_line = ( + f" printf '\\n%s\\n' '{marker_prefix}" + " full output '\"$__as_sz\"' bytes" + " -> '\"$__as_f\"' sha256='\"$__as_sha\"' complete=true]'" + ) + + return f"""{_HARNESS_SENTINEL} +__as_d={q_dir} +__as_f={q_file} +mkdir -p "$__as_d" || {{ echo '{fail_msg}' >&2; exit 1; }} +( +trap '__as_user_ec=$?; wait; exit "$__as_user_ec"' EXIT +{command}) > "$__as_f" 2>&1 +__as_ec=$? +__as_sz=$(wc -c < "$__as_f") +if [ "$__as_sz" -le {inline_bytes} ]; then + cat "$__as_f" + rm -f -- "$__as_f" +else +{sha_line} + head -c {head} "$__as_f" +{marker_line} + tail -c {tail} "$__as_f" +fi +exit $__as_ec +""" + + +def main() -> None: + try: + data = json.loads(sys.stdin.read()) + except (json.JSONDecodeError, AttributeError, OSError, TypeError): + sys.exit(0) + + if not isinstance(data, dict): + sys.exit(0) + + if not _is_codex_session(data): + sys.exit(0) + + cwd = data.get("cwd", "") + if not isinstance(cwd, str) or not cwd or not os.path.isabs(cwd): + sys.exit(0) + + disabled, inline_bytes = _read_policy(cwd) + if disabled: + sys.exit(0) + + tool_input = data.get("tool_input", {}) + command = tool_input.get("command", "") + if not isinstance(command, str) or not command: + sys.exit(0) + + harness = _build_harness(command, cwd, inline_bytes) + payload = json.dumps( + { + "hookSpecificOutput": { + "hookEventName": "PreToolUse", + "permissionDecision": "allow", + "updatedInput": {"command": harness}, + } + } + ) + sys.stdout.write(payload + "\n") + sys.exit(0) + + +if __name__ == "__main__": + main() diff --git a/src/autoskillit/server/_subprocess.py b/src/autoskillit/server/_subprocess.py index 3408aee7f..e578ffce4 100644 --- a/src/autoskillit/server/_subprocess.py +++ b/src/autoskillit/server/_subprocess.py @@ -44,6 +44,13 @@ def _is_github_cli_call(cmd: list[str]) -> bool: return len(cmd) >= 2 and cmd[0] == "gh" and cmd[1] in _GH_API_SUBCOMMANDS +def _validate_cwd(cwd: str) -> None: + if not cwd: + raise ValueError("_run_subprocess: cwd must not be empty") + if not os.path.isdir(cwd): + raise ValueError(f"_run_subprocess: cwd does not exist: {cwd}") + + async def _run_subprocess( cmd: list[str], *, @@ -56,10 +63,7 @@ async def _run_subprocess( Delegates to run_managed_async which uses temp file I/O (immune to pipe-blocking from child FD inheritance) and psutil process tree cleanup. """ - if not cwd: - raise ValueError("_run_subprocess: cwd must not be empty") - if not os.path.isdir(cwd): - raise ValueError(f"_run_subprocess: cwd does not exist: {cwd}") + _validate_cwd(cwd) runner = _get_ctx().runner assert runner is not None, "No subprocess runner configured" @@ -84,3 +88,20 @@ async def _run_subprocess( ) return returncode, stdout, stderr + + +async def _run_subprocess_captured( + cmd: list[str], + *, + cwd: str, + timeout: float, + env: Mapping[str, str] | None = None, + capture_dir: Path | None = None, +) -> SubprocessResult: + """Run a subprocess with capture-mode output. Returns the full SubprocessResult.""" + _validate_cwd(cwd) + + runner = _get_ctx().runner + assert runner is not None, "No subprocess runner configured" + + return await runner(cmd, cwd=Path(cwd), timeout=timeout, env=env, capture_dir=capture_dir) diff --git a/src/autoskillit/server/tools/_execution_helpers.py b/src/autoskillit/server/tools/_execution_helpers.py index b1d36fd6f..4f620f9f9 100644 --- a/src/autoskillit/server/tools/_execution_helpers.py +++ b/src/autoskillit/server/tools/_execution_helpers.py @@ -8,16 +8,20 @@ import time import types import typing +import uuid from pathlib import Path from typing import TYPE_CHECKING from autoskillit.core import ( RUN_PYTHON_SENTINEL_KEYS, + CapturedStream, SpillSpec, + SubprocessResult, get_logger, resolve_temp_dir, spill_output, ) +from autoskillit.execution import CaptureReadError, summarize_capture from autoskillit.server._misc import _hook_config_overlay_path from autoskillit.server._response_budget import shape_json_response @@ -51,6 +55,14 @@ def _spill_spec(tool_ctx: ToolContext) -> SpillSpec: ) +def run_cmd_artifact_root(tool_ctx: ToolContext, cwd: str) -> Path: + if cwd and Path(cwd).is_absolute(): + return ( + resolve_temp_dir(Path(cwd).resolve(), tool_ctx.config.workspace.temp_dir) / "run_cmd" + ) + return tool_ctx.temp_dir / "run_cmd" + + def spill_run_cmd_result( tool_ctx: ToolContext, *, @@ -58,16 +70,41 @@ def spill_run_cmd_result( returncode: int, stdout: str, stderr: str, + stdout_capture: CapturedStream | None = None, + stderr_capture: CapturedStream | None = None, + capture_error: str | None = None, + execution_error: str | None = None, ) -> dict[str, object]: - artifact_root = ( - resolve_temp_dir(Path(cwd).resolve(), tool_ctx.config.workspace.temp_dir) / "run_cmd" - if cwd and Path(cwd).is_absolute() - else tool_ctx.temp_dir / "run_cmd" - ) + if capture_error is not None: + result: dict[str, object] = { + "success": False, + "exit_code": returncode, + "error": f"capture_failed: {capture_error}", + } + for stream_name, capture in [("stdout", stdout_capture), ("stderr", stderr_capture)]: + if capture is not None: + _process_capture_stream(result, stream_name, capture) + return result + + if stdout_capture is not None or stderr_capture is not None: + result = { + "success": returncode == 0 and execution_error is None, + "exit_code": returncode, + "stdout": "", + "stderr": "", + } + if execution_error: + result["error"] = execution_error + for stream_name, capture in [("stdout", stdout_capture), ("stderr", stderr_capture)]: + if capture is not None: + _process_capture_stream(result, stream_name, capture) + return result + + artifact_root = run_cmd_artifact_root(tool_ctx, cwd) spec = _spill_spec(tool_ctx) shaped_stdout = spill_output(stdout, artifact_root, "stdout", spec) shaped_stderr = spill_output(stderr, artifact_root, "stderr", spec) - result: dict[str, object] = { + result = { "success": returncode == 0, "exit_code": returncode, "stdout": shaped_stdout.text, @@ -80,6 +117,78 @@ def spill_run_cmd_result( return result +def _process_capture_stream( + result: dict[str, object], + stream_name: str, + capture: CapturedStream, +) -> None: + if capture.inline_text is not None: + result[stream_name] = capture.inline_text + try: + capture.path.unlink(missing_ok=True) + except OSError: + pass + else: + promoted_name = f"{stream_name}_{_uuid8()}.log" + promoted = capture.path.parent / promoted_name + try: + os.replace(capture.path, promoted) + except OSError as exc: + result["success"] = False + result["error"] = ( + f"capture_failed: promote {stream_name} artifact " + f"{capture.path} -> {promoted}: {exc}" + ) + return + try: + fd = os.open(str(promoted.parent), os.O_RDONLY) + try: + os.fsync(fd) + finally: + os.close(fd) + except OSError: + pass + complete_str = "true" if capture.complete else "false" + marker = ( + f"\n[spilled {capture.total_bytes} bytes -> {promoted}" + f" sha256={capture.sha256} complete={complete_str}]\n" + ) + result[stream_name] = capture.head + marker + capture.tail + result[f"{stream_name}_artifact_path"] = str(promoted) + result[f"{stream_name}_total_bytes"] = capture.total_bytes + result[f"{stream_name}_sha256"] = capture.sha256 + + +def _uuid8() -> str: + return uuid.uuid4().hex[:8] + + +def _summarize_streams( + sub_result: SubprocessResult, + spec: SpillSpec, + complete: bool, +) -> tuple[CapturedStream | None, CapturedStream | None, str | None]: + stdout_capture = None + stderr_capture = None + capture_error: str | None = None + for stream_name in ("stdout", "stderr"): + stream_path = getattr(sub_result, f"{stream_name}_path") + if stream_path is not None: + try: + cap = summarize_capture(stream_path, spec, complete=complete) + if stream_name == "stdout": + stdout_capture = cap + else: + stderr_capture = cap + except CaptureReadError as exc: + capture_error = f"{exc} [orphan={stream_path}]" + try: + stream_path.unlink(missing_ok=True) + except OSError: + pass + return stdout_capture, stderr_capture, capture_error + + def shape_execution_response( tool_ctx: ToolContext, payload: dict[str, typing.Any], diff --git a/src/autoskillit/server/tools/tools_execution.py b/src/autoskillit/server/tools/tools_execution.py index d54557605..25bbe7603 100644 --- a/src/autoskillit/server/tools/tools_execution.py +++ b/src/autoskillit/server/tools/tools_execution.py @@ -30,6 +30,7 @@ ClosureAuthoritySpec, CodingAgentBackend, SkillResult, + TerminationReason, ValidatedAddDir, WriteBehaviorSpec, closure_authority_spec_from_args, @@ -45,6 +46,9 @@ from autoskillit.core import current_order_id as _current_order_id from autoskillit.core import current_step_name as _current_step_name from autoskillit.core import resolve_skill_temp_dir as _resolve_skill_temp_dir +from autoskillit.execution import ( + CaptureSetupError, +) from autoskillit.pipeline import canonical_step_name as _canonical_step_name from autoskillit.pipeline import gate_error_result from autoskillit.server import mcp @@ -68,7 +72,7 @@ get_backend as _get_backend, ) from autoskillit.server._notify import _notify, track_response_size -from autoskillit.server._subprocess import _run_subprocess +from autoskillit.server._subprocess import _run_subprocess_captured from autoskillit.server.tools._backend_compat import ( _check_backend_compat, _is_backend_incompatible, # noqa: F401 (re-exported for tests/arch/test_cross_registry_dispatch_sufficiency.py and tests/server/test_run_skill_backend_compat.py) @@ -76,11 +80,14 @@ from autoskillit.server.tools._cancellation_shield import _cancellation_shield from autoskillit.server.tools._execution_helpers import ( _import_and_call, + _spill_spec, + _summarize_streams, clear_run_skill_state, maybe_promote_work_dir, persist_run_skill_state, propagate_session_deadline, resolve_relative_path_args, + run_cmd_artifact_root, shape_execution_response, spill_run_cmd_result, validate_path_arg_anchoring, @@ -98,6 +105,8 @@ ) INGREDIENT_LOCK_DENY_PREFIX = "INGREDIENT LOCK ENFORCED" + + DEPENDENCY_DENY_PREFIX = "DEPENDENCY UNMET" @@ -398,20 +407,64 @@ async def run_cmd( _env: dict[str, str] | None = ( {**os.environ, SCENARIO_STEP_NAME_ENV: step_name} if step_name else None ) - returncode, stdout, stderr = await _run_subprocess( - ["bash", "-c", cmd], - cwd=cwd, - timeout=float(timeout), - env=_env, + artifact_root = run_cmd_artifact_root(tool_ctx, cwd) + _timeout_f = float(timeout) + try: + sub_result = await _run_subprocess_captured( + ["bash", "-c", cmd], + cwd=cwd, + timeout=_timeout_f, + env=_env, + capture_dir=artifact_root, + ) + except CaptureSetupError as exc: + result = spill_run_cmd_result( + tool_ctx, + cwd=cwd, + returncode=-1, + stdout="", + stderr="", + capture_error=str(exc), + ) + return json.dumps(result) + + spec = _spill_spec(tool_ctx) + returncode = sub_result.returncode + execution_error: str | None = None + complete = True + + term = sub_result.termination + if term == TerminationReason.NATURAL_EXIT: + returncode = sub_result.returncode + elif term == TerminationReason.TIMED_OUT: + returncode = -1 + execution_error = f"Process timed out after {_timeout_f}s" + complete = False + elif term == TerminationReason.SIGNAL_DEATH: + execution_error = ( + f"Process died to signal (returncode={sub_result.returncode})" + ) + complete = False + else: + execution_error = f"Unexpected termination: {term.value}" + complete = False + + stdout_capture, stderr_capture, capture_error = _summarize_streams( + sub_result, spec, complete ) + result = spill_run_cmd_result( tool_ctx, cwd=cwd, returncode=returncode, - stdout=stdout, - stderr=stderr, + stdout="", + stderr="", + stdout_capture=stdout_capture, + stderr_capture=stderr_capture, + capture_error=capture_error, + execution_error=execution_error, ) - if not result["success"]: + if not result.get("success"): await _notify( ctx, "error", diff --git a/src/autoskillit/server/tools/tools_kitchen.py b/src/autoskillit/server/tools/tools_kitchen.py index 8825ac571..91af390a2 100644 --- a/src/autoskillit/server/tools/tools_kitchen.py +++ b/src/autoskillit/server/tools/tools_kitchen.py @@ -211,7 +211,6 @@ class QuotaGuardHookPayload(TypedDict): class OutputBudgetPolicyHookPayload(TypedDict): disabled: bool - small_file_max_bytes: int shell_max_inline_bytes: int @@ -242,7 +241,6 @@ def _output_budget_policy_hook_payload( """ return { "disabled": not cfg.guard_enabled, - "small_file_max_bytes": cfg.small_file_max_bytes, "shell_max_inline_bytes": cfg.shell_max_inline_bytes, } diff --git a/tests/arch/test_import_paths.py b/tests/arch/test_import_paths.py index 328485a67..b75ac54c8 100644 --- a/tests/arch/test_import_paths.py +++ b/tests/arch/test_import_paths.py @@ -142,6 +142,7 @@ def test_req_imp_003_tools_import_namespace(path: Path) -> None: allowed = frozenset( { "autoskillit.core", + "autoskillit.execution", "autoskillit.pipeline", "autoskillit.server", "autoskillit.config", @@ -347,6 +348,7 @@ def test_req_imp_007_server_cli_no_unauthorized_cross_submodule_imports() -> Non Path("server/_factory.py"), Path("server/git.py"), Path("server/tools/tools_kitchen.py"), + Path("server/tools/tools_execution.py"), # capture-mode types from execution gateway Path("cli/app.py"), Path("cli/session/_session_cook.py"), # REQ-IMP-011 Path("cli/_workspace.py"), # REQ-IMP-012 diff --git a/tests/arch/test_layer_enforcement.py b/tests/arch/test_layer_enforcement.py index 4a8ba0dc5..583dec560 100644 --- a/tests/arch/test_layer_enforcement.py +++ b/tests/arch/test_layer_enforcement.py @@ -992,6 +992,9 @@ def test_migration_no_forbidden_imports() -> None: # ── REQ-ARCH-001: No cross-package submodule imports ───────────────────────── +_CROSS_PACKAGE_SUBMODULE_EXEMPTIONS: frozenset[tuple[str, str]] = frozenset() + + def test_no_cross_package_submodule_imports() -> None: """REQ-ARCH-001: No module outside package X may import from autoskillit.X.. @@ -1003,19 +1006,21 @@ def test_no_cross_package_submodule_imports() -> None: for path in _SOURCE_FILES: rel = path.relative_to(AUTOSKILLIT_ROOT) - # Determine this file's immediate package (None for root-level modules) file_package: str | None = rel.parts[0] if len(rel.parts) > 1 else None for node in _runtime_import_froms(path): if node.module is None: continue parts = node.module.split(".") - # Flag: autoskillit.. where != file_package if len(parts) >= 3 and parts[0] == "autoskillit": target_package = parts[1] if file_package != target_package: + rel_str = str(rel) + full_mod = node.module + if (rel_str, full_mod) in _CROSS_PACKAGE_SUBMODULE_EXEMPTIONS: + continue violations.append( - f"{path.relative_to(AUTOSKILLIT_ROOT)}:{node.lineno} " + f"{rel_str}:{node.lineno} " f"imports from autoskillit.{target_package}.{parts[2]}" ) @@ -1034,7 +1039,7 @@ def test_server_tools_import_only_allowed_packages() -> None: and intra-package autoskillit.server.*. TYPE_CHECKING exempt. """ - ALLOWED = {"core", "pipeline", "server", "config", "fleet", "hook_registry"} + ALLOWED = {"core", "execution", "pipeline", "server", "config", "fleet", "hook_registry"} tools_files = [ p for p in _SOURCE_FILES if p.parent.name == "tools" and p.stem.startswith("tools_") ] diff --git a/tests/arch/test_pyright_suppression_allowlist.py b/tests/arch/test_pyright_suppression_allowlist.py index 0663dddd7..2f32e59c6 100644 --- a/tests/arch/test_pyright_suppression_allowlist.py +++ b/tests/arch/test_pyright_suppression_allowlist.py @@ -85,7 +85,7 @@ def test_type_ignore_count_budget() -> None: for line in path.read_text(encoding="utf-8").splitlines(): if "# type: ignore" in line: count += 1 - budget = 105 + budget = 106 assert count <= budget, ( f"type: ignore count ({count}) exceeds budget ({budget}). " "Review new suppressions — they may indicate real type errors." diff --git a/tests/arch/test_python_no_hardcoded_temp.py b/tests/arch/test_python_no_hardcoded_temp.py index 5580913fd..d31bc1ab3 100644 --- a/tests/arch/test_python_no_hardcoded_temp.py +++ b/tests/arch/test_python_no_hardcoded_temp.py @@ -67,7 +67,7 @@ "recipe/_cmd_rpc_issues.py": "ensure_results default temp_subdir matches canonical default", "hooks/skill_load_post_hook.py": "stdlib-only hook; cannot use resolve_temp_dir()", "hooks/guards/skill_load_guard.py": "stdlib-only guard; cannot use resolve_temp_dir()", - "hooks/guards/output_budget_guard.py": ("stdlib-only guard; cannot use resolve_temp_dir()"), + "hooks/shell_capture_hook.py": "stdlib-only hook; cannot use resolve_temp_dir()", "core/runtime/session_provenance.py": "IL-0 stdlib-only module; cannot use resolve_temp_dir()", "core/runtime/kitchen_state.py": "IL-0 stdlib-only; reads hook config from canonical path", "workspace/skill_format.py": "write_paths validation accepts resolved canonical temp prefix", diff --git a/tests/arch/test_subpackage_isolation.py b/tests/arch/test_subpackage_isolation.py index 2be2e32ba..69926f7f2 100644 --- a/tests/arch/test_subpackage_isolation.py +++ b/tests/arch/test_subpackage_isolation.py @@ -866,9 +866,9 @@ def test_no_subpackage_exceeds_10_files() -> None: (interpreter/wrapper detection) for all command-classifying guards. quota_guard_state_post_hook.py is a stdlib-only PostToolUse script that maintains the per-session quota-disable marker. Exempt at 15 files. - output_budget_guard.py is a standalone stdlib-only PreToolUse script that - enforces the output-budget protocol without importing package runtime code. - Exempt at 33 files. + output_budget_guard.py was deleted and retired; its enforcement moved to + shell_capture_hook.py (input-rewrite mechanism) at the hooks/ package root. + Exempt at 32 files. pipeline/ — REQ-CNST-003-E7: pipeline/ added github_api_log.py for session-scoped GitHub API request tracking (DefaultGitHubApiLog accumulator + GitHubApiEntry). Exempt at 12 files. @@ -887,7 +887,7 @@ def test_no_subpackage_exceeds_10_files() -> None: "core": 24, # +closure_hashing +path_containment +closure_verifier "core/types": 32, # +invariant_registry (INVARIANT_REGISTRY) +closure_report "cli": 21, - "hooks": 15, # +recipe_confirmed_post_hook.py, +quota_guard_state_post_hook.py + "hooks": 17, # +recipe_confirmed_post_hook, +quota_guard_state_post_hook, +_policy_event, +shell_capture_hook (#4286) # noqa: E501 "pipeline": 12, "fleet": 23, # +_issue_url_helpers.py # noqa: E501 "recipe/rules": 54, # +commit_guard_regression_route +rules_model +rules_gitignored_deliverable +rules_issue_scope_threading +rules_inventory_gate_bilateral +rules_verdict_context # noqa: E501 @@ -895,7 +895,7 @@ def test_no_subpackage_exceeds_10_files() -> None: # auto-init dependency tracker + REVIEW_BEFORE_PLAN ordering telemetry) # +_backend_compat.py (shared target-resolution + fail-closed compatibility gate # for direct headless executor callers — report_bug, prepare_issue, enrich_issues) - "hooks/guards": 33, # +output_budget_guard (REQ-CNST-003-E6) + "hooks/guards": 32, # -output_budget_guard (#4286) "execution/backends": 12, # +_composite_locator.py, +_probe_cache.py } violations: list[str] = [] diff --git a/tests/contracts/test_protocol_satisfaction.py b/tests/contracts/test_protocol_satisfaction.py index 01484de5c..6f03a8c6c 100644 --- a/tests/contracts/test_protocol_satisfaction.py +++ b/tests/contracts/test_protocol_satisfaction.py @@ -412,6 +412,7 @@ def test_req_api_001_public_params_present(self): "workload_basenames", "on_session_id_resolved", "child_deferral_ceiling", + "capture_dir", } assert expected == public_params, ( f"run_managed_async public params changed.\n" @@ -485,6 +486,7 @@ def test_req_api_002_call_params_match_protocol(self): "workload_basenames", "on_session_id_resolved", "child_deferral_ceiling", + "capture_dir", } assert expected == actual, ( f"DefaultSubprocessRunner.__call__ params changed.\n" @@ -543,7 +545,7 @@ def test_req_api_003_params(self): """run_managed_sync parameter set is unchanged.""" sig = inspect.signature(run_managed_sync) params = set(sig.parameters) - expected = {"cmd", "cwd", "timeout", "input_data", "env"} + expected = {"cmd", "cwd", "timeout", "input_data", "env", "capture_dir"} assert expected == params, ( f"run_managed_sync params changed.\n" f" Missing: {expected - params}\n" @@ -621,7 +623,7 @@ def test_req_api_004_server_subprocess_result_under_type_checking(self): # ------------------------------------------------------------------ def test_req_api_005_subprocess_result_field_names(self): - """SubprocessResult must have exactly the 16 canonical fields.""" + """SubprocessResult must have exactly the 18 canonical fields.""" from autoskillit.core.types import SubprocessResult fields = {f.name for f in dataclasses.fields(SubprocessResult)} @@ -642,6 +644,8 @@ def test_req_api_005_subprocess_result_field_names(self): "tracked_comm", "orphaned_tool_result", "inspector_verdict", + "stdout_path", + "stderr_path", } assert fields == expected, ( f"SubprocessResult fields changed.\n" diff --git a/tests/execution/AGENTS.md b/tests/execution/AGENTS.md index 637997ee8..c8a650664 100644 --- a/tests/execution/AGENTS.md +++ b/tests/execution/AGENTS.md @@ -83,6 +83,7 @@ Subprocess integration, headless session, process lifecycle, and session result | `test_process_kill.py` | Integration tests for process tree kill and async cancellation | | `test_process_pty.py` | Tests for PTY wrapping and pipeline adjudication boundary tests | | `test_process_race.py` | Unit tests for _process_race.py: resolve_termination and ChannelBStatus | +| `test_process_io_capture.py` | Unit tests for capture-mode summarize_capture — inline/spill thresholds, multibyte, errors | | `test_process_run.py` | Integration tests for normal subprocess run, stdin, timeout, temp I/O, and logging | | `test_process_session_log_monitor.py` | Unit tests for _session_log_monitor — core detection + session ID + wiring | | `test_process_session_log_monitor_dispatch_marker.py` | Dispatch marker suppression gate tests for _session_log_monitor | diff --git a/tests/execution/backends/fixtures/codex_ndjson/hook_event_format_snapshot.json b/tests/execution/backends/fixtures/codex_ndjson/hook_event_format_snapshot.json index e4b2bf0c3..36acf7d57 100644 --- a/tests/execution/backends/fixtures/codex_ndjson/hook_event_format_snapshot.json +++ b/tests/execution/backends/fixtures/codex_ndjson/hook_event_format_snapshot.json @@ -1,5 +1,5 @@ { - "_registry_hash": "8d81e614712cca43c4ceb2ed0635ace210a5055687a19437806504115b93d46c", + "_registry_hash": "9e6225e994bb5e769312af5d4b4507a66b0f556369900dc95df8723e6228e6a6", "hooks": { "PostToolUse": [ { @@ -186,14 +186,19 @@ "command": "python3 SANITIZED_HOOKS_DIR/_dispatch.py guards/test_runner_guard", "trusted_hash": "SANITIZED_FOR_DETERMINISM", "type": "command" - }, + } + ], + "matcher": "Bash|mcp__.*autoskillit.*__run_cmd" + }, + { + "hooks": [ { - "command": "python3 SANITIZED_HOOKS_DIR/_dispatch.py guards/output_budget_guard", + "command": "python3 SANITIZED_HOOKS_DIR/_dispatch.py shell_capture_hook", "trusted_hash": "SANITIZED_FOR_DETERMINISM", "type": "command" } ], - "matcher": "Bash|mcp__.*autoskillit.*__run_cmd" + "matcher": "Bash" }, { "hooks": [ diff --git a/tests/execution/backends/test_cli_conformance_probes.py b/tests/execution/backends/test_cli_conformance_probes.py index 2c8582f25..372cebdc4 100644 --- a/tests/execution/backends/test_cli_conformance_probes.py +++ b/tests/execution/backends/test_cli_conformance_probes.py @@ -577,13 +577,14 @@ def test_generated_codex_child_receives_output_discipline( ) -def _run_output_budget_deny_probe(backend: str, tmp_path: Path) -> _DenyRoundTripOutput: +def _run_shell_capture_probe(backend: str, tmp_path: Path) -> _DenyRoundTripOutput: tmp_path.mkdir(parents=True, exist_ok=True) workspace = tmp_path / "workspace" workspace.mkdir() - env, codex_home, claude_config = _isolated_cli_env(tmp_path, workspace) + env, codex_home, _claude_config = _isolated_cli_env(tmp_path, workspace) if backend == "codex": + env["AUTOSKILLIT_AGENT_BACKEND"] = "codex" config_path = codex_home / "config.toml" sync_hooks_to_codex_config(config_path=config_path) init_result = subprocess.run( # noqa: S603 @@ -604,23 +605,6 @@ def _run_output_budget_deny_probe(backend: str, tmp_path: Path) -> _DenyRoundTri "workspace-write", _OUTPUT_BUDGET_CANARY_PROMPT, ] - elif backend == "claude-code": - settings_path = claude_config / "settings.json" - settings_path.write_text( - json.dumps(generate_hooks_json(), indent=2) + "\n", - encoding="utf-8", - ) - command = [ - "claude", - "-p", - _OUTPUT_BUDGET_CANARY_PROMPT, - "--output-format", - "stream-json", - "--verbose", - "--dangerously-skip-permissions", - "--tools", - "Bash", - ] else: # pragma: no cover - callers pass a sealed backend literal raise ValueError(f"unsupported probe backend: {backend}") @@ -635,30 +619,30 @@ def _run_output_budget_deny_probe(backend: str, tmp_path: Path) -> _DenyRoundTri ) transcript = result.stdout + "\n" + result.stderr if result.returncode != 0: - raise OSError(f"{backend} deny probe failed with rc={result.returncode}: {transcript}") + raise OSError( + f"{backend} shell-capture probe failed with rc={result.returncode}: {transcript}" + ) return _DenyRoundTripOutput( transcript=transcript, cli_version=_cli_version(command[0], env), ) -def _assert_output_budget_deny_round_trip(output: _DenyRoundTripOutput) -> None: +def _assert_shell_capture_round_trip(output: _DenyRoundTripOutput) -> None: assert _OUTPUT_BUDGET_CANARY_COMMAND in output.transcript - assert "rg -l" in output.transcript - assert "head -c 4000" in output.transcript - assert ".autoskillit/temp/" in output.transcript + assert "autoskillit-shell-capture" in output.transcript -def _exercise_output_budget_deny_probe(backend: str, tmp_path: Path) -> None: +def _exercise_shell_capture_probe(backend: str, tmp_path: Path) -> None: workspace = tmp_path / "version-workspace" workspace.mkdir() version_env, _, _ = _isolated_cli_env(tmp_path / "version-env", workspace) binary = "codex" if backend == "codex" else "claude" cli_version = _cli_version(binary, version_env) - cache_path = tmp_path / f"{backend}-output-budget-probe-cache.json" + cache_path = tmp_path / f"{backend}-shell-capture-probe-cache.json" cached = read_probe_cache(cache_path, cli_version, PROBE_POLICY_IDENTITY) if cached is not None and cached.passed: - pytest.skip(f"Output-budget deny probe cached as passed for {cli_version}") + pytest.skip(f"Shell-capture probe cached as passed for {cli_version}") def _record(passed: bool, version: str, detail: str | None) -> None: write_probe_cache( @@ -684,25 +668,19 @@ def _record_failure( _record(False, version, f"{kind.value}:{probe_name}:{detail}") _run_probe_with_discrimination( - f"output_budget_deny_round_trip_{backend}", + f"shell_capture_round_trip_{backend}", cli_version, - lambda: _run_output_budget_deny_probe(backend, tmp_path / "round-trip"), - _assert_output_budget_deny_round_trip, + lambda: _run_shell_capture_probe(backend, tmp_path / "round-trip"), + _assert_shell_capture_round_trip, record_success=_record_success, record_failure=_record_failure, ) @_skip_unless_codex_output_budget_smoke -class TestCodexOutputBudgetDenyRoundTrip: - def test_hook_fires_and_reason_reaches_model(self, tmp_path: Path) -> None: - _exercise_output_budget_deny_probe("codex", tmp_path) - - -@_skip_unless_claude_output_budget_smoke -class TestClaudeCodeOutputBudgetDenyRoundTrip: - def test_hook_fires_and_reason_reaches_model(self, tmp_path: Path) -> None: - _exercise_output_budget_deny_probe("claude-code", tmp_path) +class TestCodexShellCaptureRoundTrip: + def test_hook_fires_and_command_is_rewritten(self, tmp_path: Path) -> None: + _exercise_shell_capture_probe("codex", tmp_path) _SOURCE_SPILL_THRESHOLD = OutputBudgetConfig().inline_max_chars diff --git a/tests/execution/backends/test_hook_deny_efficacy_probe.py b/tests/execution/backends/test_hook_deny_efficacy_probe.py index 7b3b3258a..2fef4e753 100644 --- a/tests/execution/backends/test_hook_deny_efficacy_probe.py +++ b/tests/execution/backends/test_hook_deny_efficacy_probe.py @@ -319,28 +319,29 @@ def test_probe_collection_count() -> None: assert len(_collect_probe_params()) == EXPECTED_TOTAL_PROBE_COUNT -def test_output_budget_guard_has_non_inert_bash_and_run_cmd_rows(tmp_path: Path) -> None: - script = "guards/output_budget_guard.py" +def test_shell_capture_hook_is_input_rewrite_and_excluded_from_deny_matrix( + tmp_path: Path, +) -> None: + script = "shell_capture_hook.py" hookdef_idx, hookdef = next( (idx, hookdef) for idx, hookdef in enumerate(HOOK_REGISTRY) if script in hookdef.scripts ) - expected_rows = { - (tool_class, session_mode, backend) - for tool_class in ("Bash", "mcp_run_cmd") - for session_mode in SESSION_MODES - for backend in BACKENDS - } + assert hookdef.mechanism == "input-rewrite" + + # An input-rewrite hook emits allow+updatedInput, never a JSON deny, so it + # is not part of the deny-mechanism probe matrix collected by + # _collect_probe_params() (which filters on mechanism == "deny"). actual_rows = { (values[2], values[3], values[4]) for parameter in _collect_probe_params() if (values := parameter.values)[0] == hookdef_idx and values[1] == script } - assert actual_rows == expected_rows + assert actual_rows == set() codex_entries = generate_codex_hooks_config()["PreToolUse"] codex_entry = next(entry for entry in codex_entries if entry["matcher"] == hookdef.matcher) codex_hook = next( - hook for hook in codex_entry["hooks"] if "guards/output_budget_guard" in hook["command"] + hook for hook in codex_entry["hooks"] if "shell_capture_hook" in hook["command"] ) assert codex_hook["trusted_hash"] @@ -350,23 +351,18 @@ def test_output_budget_guard_has_non_inert_bash_and_run_cmd_rows(tmp_path: Path) "AUTOSKILLIT_CWD": str(tmp_path), } (tmp_path / ".autoskillit" / "temp").mkdir(parents=True) - for tool_class in ("Bash", "mcp_run_cmd"): - payload = _build_payload(tool_class, "interactive") - tool_input = payload["tool_input"] - if tool_class == "Bash": - tool_input["command"] = _OUTPUT_BUDGET_PROBE_COMMAND - else: - tool_input.pop("command") - tool_input["cmd"] = _OUTPUT_BUDGET_PROBE_COMMAND - result = _invoke_guard( - HOOKS_DIR / script, - payload, - env, - tmp_path, - ) - hook_output = result["hookSpecificOutput"] - assert hook_output["permissionDecision"] == "deny" - reason = hook_output["permissionDecisionReason"] - assert "rg -l" in reason - assert "head -c 4000" in reason - assert ".autoskillit/temp/" in reason + payload = _build_payload("Bash", "interactive") + payload["tool_input"]["command"] = _OUTPUT_BUDGET_PROBE_COMMAND + payload["cwd"] = str(tmp_path) + payload["turn_id"] = "probe-turn" + result = _invoke_guard( + HOOKS_DIR / script, + payload, + env, + tmp_path, + ) + hook_output = result["hookSpecificOutput"] + assert hook_output["permissionDecision"] == "allow" + updated_command = hook_output["updatedInput"]["command"] + assert _OUTPUT_BUDGET_PROBE_COMMAND in updated_command + assert "autoskillit-shell-capture" in updated_command diff --git a/tests/execution/test_process_io_capture.py b/tests/execution/test_process_io_capture.py new file mode 100644 index 000000000..908dab3d8 --- /dev/null +++ b/tests/execution/test_process_io_capture.py @@ -0,0 +1,65 @@ +"""Tests for capture-mode summarize_capture in _process_io.""" + +from __future__ import annotations + +from pathlib import Path + +import pytest + +from autoskillit.core import SpillSpec +from autoskillit.execution.process._process_io import ( + CaptureReadError, + summarize_capture, +) + +pytestmark = [pytest.mark.layer("execution"), pytest.mark.small] + +SPEC = SpillSpec(inline_max_chars=100, head_chars=40, tail_chars=30) + + +def test_summarize_capture_small_inlines_full_text(tmp_path: Path) -> None: + f = tmp_path / "small.txt" + f.write_text("hello world\n") + result = summarize_capture(f, SPEC) + assert result.inline_text == "hello world\n" + assert result.total_bytes == 12 + assert len(result.sha256) == 64 + assert result.complete is True + + +def test_summarize_capture_large_returns_bounded_slices(tmp_path: Path) -> None: + content = "A" * 200 + f = tmp_path / "large.txt" + f.write_text(content) + result = summarize_capture(f, SPEC) + assert result.inline_text is None + assert result.total_bytes == 200 + assert len(result.head) <= 40 + assert len(result.tail) <= 30 + assert result.head == "A" * 40 + assert result.tail == "A" * 30 + + +def test_summarize_capture_multibyte_boundary_replaces(tmp_path: Path) -> None: + head_part = "X" * 38 + "é" + tail_part = "é" + "Y" * 28 + content = head_part + "Z" * 100 + tail_part + f = tmp_path / "multi.txt" + f.write_bytes(content.encode("utf-8")) + spec = SpillSpec(inline_max_chars=50, head_chars=40, tail_chars=30) + result = summarize_capture(f, spec) + assert result.inline_text is None + assert isinstance(result.head, str) + assert isinstance(result.tail, str) + + +def test_summarize_capture_missing_file_raises(tmp_path: Path) -> None: + with pytest.raises(CaptureReadError): + summarize_capture(tmp_path / "missing.txt", SPEC) + + +def test_summarize_capture_incomplete_flag(tmp_path: Path) -> None: + f = tmp_path / "partial.txt" + f.write_text("partial output") + result = summarize_capture(f, SPEC, complete=False) + assert result.complete is False diff --git a/tests/execution/test_process_run.py b/tests/execution/test_process_run.py index 55dc1c773..b098b5ede 100644 --- a/tests/execution/test_process_run.py +++ b/tests/execution/test_process_run.py @@ -401,3 +401,104 @@ async def test_run_managed_async_idle_stall_kills_hanging_process(self, tmp_path elapsed = time.monotonic() - start assert result.termination == TerminationReason.IDLE_STALL assert elapsed < 12.0 + + +class TestCaptureMode: + """Capture-mode retains stream files and populates path fields.""" + + @pytest.mark.anyio + async def test_capture_mode_retains_stream_files(self, tmp_path): + capture_dir = tmp_path / "capture" + script = tmp_path / "write.py" + script.write_text(CLEAN_OUTPUT_SCRIPT) + result = await run_managed_async( + [sys.executable, str(script)], + cwd=tmp_path, + timeout=10, + capture_dir=capture_dir, + ) + assert result.stdout_path is not None + assert result.stderr_path is not None + assert result.stdout_path.exists() + assert result.stderr_path.exists() + assert result.stdout == "" + assert result.stderr == "" + content = result.stdout_path.read_text() + for i in range(10): + assert f"line {i}" in content + + @pytest.mark.anyio + async def test_default_mode_unchanged(self, tmp_path): + script = tmp_path / "write.py" + script.write_text(CLEAN_OUTPUT_SCRIPT) + result = await run_managed_async( + [sys.executable, str(script)], + cwd=tmp_path, + timeout=10, + ) + assert result.stdout_path is None + assert result.stderr_path is None + assert "line 0" in result.stdout + + @pytest.mark.anyio + async def test_capture_dir_uncreatable_raises_capture_setup_error(self, tmp_path): + blocker = tmp_path / "blocker" + blocker.write_text("file") + from autoskillit.execution.process._process_io import CaptureSetupError + + with pytest.raises(CaptureSetupError): + await run_managed_async( + [sys.executable, "-c", "pass"], + cwd=tmp_path, + timeout=10, + capture_dir=blocker / "nested", + ) + + +class TestCaptureModeSync: + """Behavioral capture-mode tests for run_managed_sync.""" + + def test_sync_capture_mode_retains_stream_files(self, tmp_path): + capture_dir = tmp_path / "capture" + script = tmp_path / "write.py" + script.write_text(CLEAN_OUTPUT_SCRIPT) + result = run_managed_sync( + [sys.executable, str(script)], + cwd=tmp_path, + timeout=10, + capture_dir=capture_dir, + ) + assert result.stdout_path is not None + assert result.stderr_path is not None + assert result.stdout_path.exists() + assert result.stderr_path.exists() + assert result.stdout == "" + assert result.stderr == "" + content = result.stdout_path.read_text() + for i in range(10): + assert f"line {i}" in content + + def test_sync_capture_dir_uncreatable_raises_capture_setup_error(self, tmp_path): + from autoskillit.execution.process._process_io import CaptureSetupError + + blocker = tmp_path / "blocker" + blocker.write_text("file") + with pytest.raises(CaptureSetupError): + run_managed_sync( + [sys.executable, "-c", "pass"], + cwd=tmp_path, + timeout=10, + capture_dir=blocker / "nested", + ) + + def test_sync_default_mode_clears_output(self, tmp_path): + script = tmp_path / "write.py" + script.write_text(CLEAN_OUTPUT_SCRIPT) + result = run_managed_sync( + [sys.executable, str(script)], + cwd=tmp_path, + timeout=10, + ) + assert result.stdout_path is None + assert result.stderr_path is None + assert "line 0" in result.stdout diff --git a/tests/execution/test_process_submodules.py b/tests/execution/test_process_submodules.py index 2a15c3934..29420a3d3 100644 --- a/tests/execution/test_process_submodules.py +++ b/tests/execution/test_process_submodules.py @@ -14,6 +14,8 @@ _EXPECTED_PROCESS_SYMBOLS: frozenset[str] = frozenset( { + "CaptureReadError", + "CaptureSetupError", "DefaultSubprocessRunner", "_extract_stdout_session_id", "_resolve_session_id", @@ -42,6 +44,7 @@ "resolve_termination", "run_managed_async", "run_managed_sync", + "summarize_capture", } ) diff --git a/tests/fakes.py b/tests/fakes.py index be6ab738c..db58abef2 100644 --- a/tests/fakes.py +++ b/tests/fakes.py @@ -9,6 +9,7 @@ import asyncio import dataclasses import re +import uuid from collections import deque from collections.abc import Awaitable, Callable, Mapping, Sequence from pathlib import Path @@ -865,4 +866,19 @@ async def __call__( on_pid_resolved = kwargs.get("on_pid_resolved") if callable(on_pid_resolved) and result.pid > 0: on_pid_resolved(result.pid, 0) + capture_dir = kwargs.get("capture_dir") + if capture_dir is not None: + capture_dir_path = Path(capture_dir) # type: ignore[arg-type] + capture_dir_path.mkdir(parents=True, exist_ok=True) + stdout_path = capture_dir_path / f"proc_stdout_{uuid.uuid4().hex[:8]}.tmp" + stderr_path = capture_dir_path / f"proc_stderr_{uuid.uuid4().hex[:8]}.tmp" + stdout_path.write_text(result.stdout, encoding="utf-8") + stderr_path.write_text(result.stderr, encoding="utf-8") + result = dataclasses.replace( + result, + stdout="", + stderr="", + stdout_path=stdout_path, + stderr_path=stderr_path, + ) return result diff --git a/tests/hooks/AGENTS.md b/tests/hooks/AGENTS.md index 2e82eb93e..19cb1a0d5 100644 --- a/tests/hooks/AGENTS.md +++ b/tests/hooks/AGENTS.md @@ -36,7 +36,8 @@ Hook script behavior, registration, and bridge tests. | `test_compose_pr_body_guard.py` | Tests for compose_pr_body_guard.py PreToolUse hook — body file Closes #N validation | | `test_command_classification.py` | Tests for the shared _command_classification.py command classification primitives | | `test_command_classification_git.py` | Tests for the git command classification primitives (is_git_command, extract_git_subcommand_and_flags) | -| `test_output_budget_guard.py` | Tests for output_budget_guard.py PreToolUse classification, bounded exceptions, config bridge, and registration | +| `test_shell_capture_hook.py` | Tests for shell_capture_hook.py PreToolUse input-rewrite hook — Codex scope, harness generation, fail-open | +| `test_shell_capture_conformance.py` | Semantic conformance gate: wrapped vs raw execution agreement on exit codes and captured bytes | | `test_pr_create_guard.py` | Tests for pr_create_guard.py interpreter bypass detection | | `test_planner_gh_discovery_guard.py` | Tests for planner_gh_discovery_guard.py interpreter bypass detection | | `test_ingredient_lock_guard.py` | Tests for ingredient_lock_guard.py PreToolUse hook: deny/allow, fail-open, pipeline scoping | @@ -49,6 +50,7 @@ Hook script behavior, registration, and bridge tests. | `test_reset_resume_gate.py` | Tests for reset_resume_gate.py PreToolUse guard — deny/allow, name→UUID resolution, REFUSED bypass, fail-open | | `test_recipe_confirmed_post_hook.py` | Tests for recipe_confirmed_post_hook.py PostToolUse hook — marker write, idempotency, fail-open | | `test_quota_guard_state_post_hook.py` | Tests for quota_guard_state_post_hook.py PostToolUse hook — per-session quota-disable marker write/clear, atomic-failure rewrite, session isolation | +| `test_policy_event.py` | Tests for _policy_event.py typed policy-event formatter — provenance prefix rendering | ## Architecture Notes diff --git a/tests/hooks/test_codex_hooks_format_contract.py b/tests/hooks/test_codex_hooks_format_contract.py index cdc9a0c14..8d5b02f96 100644 --- a/tests/hooks/test_codex_hooks_format_contract.py +++ b/tests/hooks/test_codex_hooks_format_contract.py @@ -55,10 +55,8 @@ def test_no_event_scalar_in_toml_entries(self): f"Entry under hooks.{event_type} has redundant 'event' scalar" ) - def test_output_budget_guard_is_wired_with_identical_matchers(self): - expected = next( - hook for hook in HOOK_REGISTRY if "guards/output_budget_guard.py" in hook.scripts - ) + def test_shell_capture_hook_is_wired_with_identical_matchers(self): + expected = next(hook for hook in HOOK_REGISTRY if "shell_capture_hook.py" in hook.scripts) codex_entries = generate_codex_hooks_config()["PreToolUse"] codex_entry = next( entry for entry in codex_entries if entry["matcher"] == expected.matcher @@ -68,6 +66,6 @@ def test_output_budget_guard_is_wired_with_identical_matchers(self): entry for entry in claude_entries if entry["matcher"] == expected.matcher ) - assert any("output_budget_guard" in hook["command"] for hook in codex_entry["hooks"]) - assert any("output_budget_guard" in hook["command"] for hook in claude_entry["hooks"]) + assert any("shell_capture_hook" in hook["command"] for hook in codex_entry["hooks"]) + assert any("shell_capture_hook" in hook["command"] for hook in claude_entry["hooks"]) assert codex_entry["matcher"].encode() == claude_entry["matcher"].encode() diff --git a/tests/hooks/test_command_classification.py b/tests/hooks/test_command_classification.py index bd049f08e..437e09ac5 100644 --- a/tests/hooks/test_command_classification.py +++ b/tests/hooks/test_command_classification.py @@ -860,130 +860,3 @@ def test_strip_heredoc_bodies(command: str, expected_stripped: str) -> None: from autoskillit.hooks._command_classification import strip_heredoc_bodies assert strip_heredoc_bodies(command) == expected_stripped - - -class TestOutputBudgetStructure: - def test_preserves_pipeline_and_sequence_connectors(self): - from autoskillit.hooks._command_classification import tokenize_command_structure - - parsed = tokenize_command_structure("risky | one |& two && three; four") - assert parsed is not None - assert [connector for connector, _tokens in parsed] == ["", "|", "|&", "&&", ";"] - - @pytest.mark.parametrize("prefix", ["do", "then", "elif", "else"]) - def test_compound_control_prefix_is_not_the_verb(self, prefix): - from autoskillit.hooks._command_classification import command_verb_and_args - - assert command_verb_and_args([prefix, "rg", "pat", "src/"])[0] == "rg" - - def test_descriptor_duplication_remains_one_ordered_token(self): - from autoskillit.hooks._command_classification import tokenize_command_structure - - parsed = tokenize_command_structure("risky 2>&1 | head -c 4") - assert parsed == [("", ["risky", "2>&1"]), ("|", ["head", "-c", "4"])] - - -class TestOutputBudgetDisposition: - @staticmethod - def _classify(command: str, *, cwd: str = "/workspace", max_bytes: int = 12_000): - from autoskillit.hooks._command_classification import ( - classify_command_output_budget, - command_verb, - ) - - def producer(tokens): - return (False, False) if command_verb(tokens) == "risky" else None - - return classify_command_output_budget( - command, producer, max_inline_bytes=max_bytes, cwd=cwd - ) - - @pytest.mark.parametrize( - "command", - [ - "risky 2>&1 | head -c 4000", - "risky |& head -c 4000", - "risky >.autoskillit/temp/out 2>&1", - ], - ) - def test_proven_bounds(self, command): - from autoskillit.hooks._command_classification import CommandBudgetDisposition - - assert self._classify(command) is CommandBudgetDisposition.BOUNDED - - @pytest.mark.parametrize( - "command", - [ - "risky | head -c 4000", - "risky && head -c 4000", - "risky 2>err.txt", - "risky 2>&1 >.autoskillit/temp/out", - ], - ) - def test_unbounded_descriptor_paths(self, command): - from autoskillit.hooks._command_classification import CommandBudgetDisposition - - assert self._classify(command) is CommandBudgetDisposition.HAZARDOUS - - @pytest.mark.parametrize( - "command", - [ - "risky 2>&1 | tee copy | head -c 4000", - "risky >$DYNAMIC 2>&1", - "cat <(risky)", - "risky 3>&1 2>&1 | head -c 4000", - ], - ) - def test_unsupported_shell_shapes_are_unknown(self, command): - from autoskillit.hooks._command_classification import CommandBudgetDisposition - - assert self._classify(command) is CommandBudgetDisposition.UNKNOWN - - def test_command_length_boundary(self): - from autoskillit.hooks._command_classification import ( - OUTPUT_BUDGET_MAX_COMMAND_CHARS, - CommandBudgetDisposition, - ) - - at_limit = "risky" + (" " * (OUTPUT_BUDGET_MAX_COMMAND_CHARS - len("risky"))) - over_limit = at_limit + " " - assert self._classify(at_limit) is CommandBudgetDisposition.HAZARDOUS - assert self._classify(over_limit) is CommandBudgetDisposition.UNKNOWN - - def test_nested_shell_depth_boundary(self): - import shlex - - from autoskillit.hooks._command_classification import ( - OUTPUT_BUDGET_MAX_NESTING_DEPTH, - CommandBudgetDisposition, - ) - - def nest(depth: int) -> str: - command = "risky" - for _ in range(depth): - command = f"bash -c {shlex.quote(command)}" - return command - - assert self._classify(nest(OUTPUT_BUDGET_MAX_NESTING_DEPTH)) is ( - CommandBudgetDisposition.HAZARDOUS - ) - assert self._classify(nest(OUTPUT_BUDGET_MAX_NESTING_DEPTH + 1)) is ( - CommandBudgetDisposition.UNKNOWN - ) - - def test_bounded_small_input_followed_by_transform_is_unknown(self): - from autoskillit.hooks._command_classification import ( - CommandBudgetDisposition, - classify_command_output_budget, - command_verb, - ) - - def small_producer(tokens): - return (True, True) if command_verb(tokens) == "small" else None - - disposition = classify_command_output_budget( - "small | awk '{while (1) print}'", - small_producer, - max_inline_bytes=12_000, - ) - assert disposition is CommandBudgetDisposition.UNKNOWN diff --git a/tests/hooks/test_hook_config_bridge.py b/tests/hooks/test_hook_config_bridge.py index 6f5dbb32c..b0a3de282 100644 --- a/tests/hooks/test_hook_config_bridge.py +++ b/tests/hooks/test_hook_config_bridge.py @@ -216,7 +216,6 @@ def test_output_budget_policy_serializer_matches_stdlib_bridge_keys(): payload = _output_budget_policy_hook_payload( OutputBudgetConfig( guard_enabled=False, - small_file_max_bytes=321, shell_max_inline_bytes=654, ) ) @@ -224,7 +223,6 @@ def test_output_budget_policy_serializer_matches_stdlib_bridge_keys(): assert set(payload) == OUTPUT_BUDGET_POLICY_HOOK_PAYLOAD_KEYS assert payload == { "disabled": True, - "small_file_max_bytes": 321, "shell_max_inline_bytes": 654, } @@ -240,7 +238,6 @@ def test_output_budget_policy_overlay_overrides_snapshot(tmp_path): { "output_budget_policy": { "disabled": False, - "small_file_max_bytes": 5000, "shell_max_inline_bytes": 12000, } } @@ -251,7 +248,6 @@ def test_output_budget_policy_overlay_overrides_snapshot(tmp_path): { "output_budget_policy": { "disabled": True, - "small_file_max_bytes": 17, } } ) @@ -261,7 +257,6 @@ def test_output_budget_policy_overlay_overrides_snapshot(tmp_path): assert policy == { "disabled": True, - "small_file_max_bytes": 17, "shell_max_inline_bytes": 12000, } diff --git a/tests/hooks/test_hook_dispatch.py b/tests/hooks/test_hook_dispatch.py index 45b8df0e4..40a91a7ee 100644 --- a/tests/hooks/test_hook_dispatch.py +++ b/tests/hooks/test_hook_dispatch.py @@ -202,6 +202,7 @@ def test_generate_hooks_json_uses_dispatcher(self) -> None: "pipeline_step_post_hook", "resume_gate_post_hook", "recipe_confirmed_post_hook", + "shell_capture_hook", ), f"Unexpected logical name format: {logical_name}" diff --git a/tests/hooks/test_hook_registry.py b/tests/hooks/test_hook_registry.py index 0daa89511..d9431eca6 100644 --- a/tests/hooks/test_hook_registry.py +++ b/tests/hooks/test_hook_registry.py @@ -230,6 +230,7 @@ def test_find_broken_hook_scripts_does_not_flag_user_python_scripts(tmp_path: Pa "guards/grep_pattern_lint_guard.py", "guards/mcp_health_advisor.py", "guards/pipeline_step_guard.py", + "shell_capture_hook.py", } ) diff --git a/tests/hooks/test_hook_registry_codex_status.py b/tests/hooks/test_hook_registry_codex_status.py index 4c2e23e8c..74b0eb219 100644 --- a/tests/hooks/test_hook_registry_codex_status.py +++ b/tests/hooks/test_hook_registry_codex_status.py @@ -67,11 +67,16 @@ def test_all_registry_entries_have_valid_mechanism(self): "deny", "additionalContext", "output-rewrite", + "input-rewrite", ), f"Hook {hook_def.scripts} has invalid mechanism: {hook_def.mechanism!r}" def test_pretooluse_deny_mechanism_is_set(self): for hook_def in HOOK_REGISTRY: - if hook_def.event_type == "PreToolUse" and hook_def.codex_status != "not-applicable": + if ( + hook_def.event_type == "PreToolUse" + and hook_def.codex_status != "not-applicable" + and hook_def.mechanism != "input-rewrite" + ): assert hook_def.mechanism == "deny", ( f"Hook {hook_def.scripts} has mechanism={hook_def.mechanism!r}, " "expected 'deny' for PreToolUse hooks" @@ -105,7 +110,11 @@ def test_all_registry_entries_have_enforcement_strength_keys(self): def test_enforcement_strength_claude_code_values_are_valid(self): for hd in HOOK_REGISTRY: - assert hd.enforcement_strength["claude_code"] in ("hard", "soft"), ( + assert hd.enforcement_strength["claude_code"] in ( + "hard", + "soft", + "not-applicable", + ), ( f"Hook {hd.scripts} has invalid claude_code: " f"{hd.enforcement_strength['claude_code']!r}" ) diff --git a/tests/hooks/test_output_budget_guard.py b/tests/hooks/test_output_budget_guard.py deleted file mode 100644 index 7f0c59e29..000000000 --- a/tests/hooks/test_output_budget_guard.py +++ /dev/null @@ -1,297 +0,0 @@ -"""Focused behavior tests for the output-budget PreToolUse guard.""" - -# ruff: noqa: E501 -- verbatim incident commands intentionally preserve long lines. - -from __future__ import annotations - -import io -import json -import shutil -from contextlib import redirect_stdout -from pathlib import Path - -import pytest - -pytestmark = [pytest.mark.layer("hooks"), pytest.mark.small] - -INCIDENT_LOG_SEARCH = r"""for base in /home/talon/.local/share/autoskillit/logs \ - /home/talon/Library/Application\ Support/autoskillit/logs; do - if [ -d "$base" ]; then - rg -n -F \ - -e '019f669e-dce8-72a0-9d40-c564681bd145' \ - -e '019f66ba-4c32-7800-b8cf-787d47abaf61' \ - -e 'remediation-4226' \ - "$base/sessions.jsonl" "$base" \ - --glob '*.jsonl' \ - --glob '!codex-sessions/**' 2>/dev/null | - head -n 200 - fi -done""" - -INCIDENT_RESOLVE_REVIEW_SEARCH = """nl -ba src/autoskillit/skills_extended/resolve-review/SKILL.md | - sed -n '1,300p' && -rg -n \ - "resolve-review|expected_output_patterns|verdict" \ - src/autoskillit/skills_extended/resolve-review \ - src/autoskillit/recipes \ - src/autoskillit/recipe \ - tests/contracts \ - tests/server \ - --glob '*.yaml' \ - --glob '*.md' \ - --glob '*.json' \ - --glob '*.py'""" - - -def _build_event(command: str, *, run_cmd: bool = False) -> dict: - key = "cmd" if run_cmd else "command" - tool = "mcp__autoskillit__local__autoskillit__run_cmd" if run_cmd else "Bash" - return {"tool_name": tool, "tool_input": {key: command}} - - -def _run_hook(event: dict | None, monkeypatch, *, raw_stdin: str | None = None) -> str: - from autoskillit.hooks.guards.output_budget_guard import main # noqa: PLC0415 - - stdin_text = raw_stdin if raw_stdin is not None else json.dumps(event) - monkeypatch.setattr("sys.stdin", io.StringIO(stdin_text)) - output = io.StringIO() - with pytest.raises(SystemExit), redirect_stdout(output): - main() - return output.getvalue() - - -def _is_denied(output: str) -> bool: - if not output: - return False - payload = json.loads(output) - return payload["hookSpecificOutput"]["permissionDecision"] == "deny" - - -@pytest.mark.parametrize( - "command", - [ - 'rg -n "pat" src/ tests/', - 'grep -r "pat" .', - "rg -l pat src/", - "rg --count pat src/", - "rg -m 5 pat src/", - "cat sessions.jsonl", - "sed -n '1,300p' sessions.jsonl", - "rg pat sessions.jsonl | head -n 200", - "jq -r '.payload' sessions.jsonl", - "find / -maxdepth 2 -name '*.py'", - 'bash -c "rg -n pat src/"', - ], -) -def test_denies_unbounded_r1_r2_r3_shapes(command, monkeypatch, tmp_path): - (tmp_path / "src").mkdir(exist_ok=True) - (tmp_path / "tests").mkdir(exist_ok=True) - assert _is_denied(_run_hook(_build_event(command), monkeypatch)) - - -@pytest.mark.parametrize("run_cmd", [False, True]) -def test_reads_both_command_input_keys(run_cmd, monkeypatch, tmp_path): - (tmp_path / "src").mkdir() - assert _is_denied(_run_hook(_build_event("rg pat src/", run_cmd=run_cmd), monkeypatch)) - - -@pytest.mark.parametrize("command", [INCIDENT_LOG_SEARCH, INCIDENT_RESOLVE_REVIEW_SEARCH]) -@pytest.mark.parametrize("run_cmd", [False, True]) -def test_denies_verbatim_incident_commands_with_concrete_rewrite(command, run_cmd, monkeypatch): - output = _run_hook(_build_event(command, run_cmd=run_cmd), monkeypatch) - assert _is_denied(output) - reason = json.loads(output)["hookSpecificOutput"]["permissionDecisionReason"] - assert "2>&1 | head -c 4000" in reason - assert ".autoskillit/temp/" in reason - - -@pytest.mark.parametrize( - "command", - [ - "rg pat src/ 2>&1 | head -c 4000", - "rg pat src/ |& head -c 4000", - "jq -r .payload sessions.jsonl 2>&1 | head -c 12000", - "rg -M 500 pat sessions.jsonl 2>&1 | tail -c 12000", - "rg pat src/ >.autoskillit/temp/search.out 2>&1", - "rg pat src/ 1>.autoskillit/temp/search.out 2>.autoskillit/temp/search.err", - "find / -quit 2>.autoskillit/temp/find.err", - "find / -quit 2>&1 | head -c 4000", - "rg -n pat single_file.py", - "git log --oneline -20", - ], -) -def test_allows_proven_bounded_cases(command, monkeypatch, tmp_path): - (tmp_path / "src").mkdir(exist_ok=True) - (tmp_path / ".autoskillit" / "temp").mkdir(parents=True, exist_ok=True) - assert _run_hook(_build_event(command), monkeypatch) == "" - - -@pytest.mark.parametrize( - "command", - [ - "rg pat src/ | head -c 4000", - "rg pat src/ && head -c 4000", - "rg pat src/ 2>err.txt", - "rg pat src/ 2>&1 >.autoskillit/temp/out", - "rg pat src/ 2>&1 | head -c 12001", - "rg pat src/ 2>&1 | head -c 0", - "rg -q pat src/", - "find / -quit", - "rg pat src/ 2>&1 | tee .autoskillit/temp/copy | head -c 4000", - "cat <(rg pat src/)", - "rg pat src/ >$OUTPUT 2>&1", - ], -) -def test_denies_false_bounds_and_unknown_shell_shapes(command, monkeypatch, tmp_path): - (tmp_path / "src").mkdir(exist_ok=True) - (tmp_path / ".autoskillit" / "temp").mkdir(parents=True, exist_ok=True) - assert _is_denied(_run_hook(_build_event(command), monkeypatch)) - - -def test_small_jsonl_exception_is_narrow(monkeypatch, tmp_path): - path = tmp_path / "small.jsonl" - path.write_text('{"value": "small"}\n') - - assert _run_hook(_build_event(f"cat {path.name}"), monkeypatch) == "" - assert _run_hook(_build_event(f"wc -l {path.name}"), monkeypatch) == "" - assert _is_denied(_run_hook(_build_event(f"jq -r .value {path.name}"), monkeypatch)) - assert _is_denied( - _run_hook(_build_event(f"cat {path.name} | awk '{{while (1) print}}'"), monkeypatch) - ) - - -@pytest.mark.parametrize("line_flag", ["-l", "--lines"]) -def test_wc_lines_allows_literal_in_project_jsonl_files(line_flag, monkeypatch, tmp_path): - first = tmp_path / "first.jsonl" - second = tmp_path / "second.jsonl" - plain = tmp_path / "plain.txt" - first.write_bytes(b"x" * 2_000) - second.write_bytes(b"y" * 3_000) - plain.write_text("plain\n") - - assert _run_hook(_build_event(f"wc {line_flag} {first.name}"), monkeypatch) == "" - assert _run_hook(_build_event(f"wc {line_flag} {plain.name}"), monkeypatch) == "" - assert ( - _run_hook( - _build_event(f"wc {line_flag} -- {first.name} {second.name}"), - monkeypatch, - ) - == "" - ) - - -def test_wc_lines_denies_nonliteral_or_unsafe_jsonl_operands(monkeypatch, tmp_path): - small = tmp_path / "small.jsonl" - small.write_text("{}\n") - plain = tmp_path / "plain.txt" - plain.write_text("plain\n") - outside = tmp_path.parent / f"{tmp_path.name}-outside.jsonl" - outside.write_text("{}\n") - symlink = tmp_path / "link.jsonl" - symlink.symlink_to(small) - oversized = tmp_path / "oversized.jsonl" - oversized.write_bytes(b"x" * 5_001) - aggregate_a = tmp_path / "aggregate-a.jsonl" - aggregate_b = tmp_path / "aggregate-b.jsonl" - aggregate_a.write_bytes(b"a" * 2_501) - aggregate_b.write_bytes(b"b" * 2_500) - - commands = [ - "wc -l missing.jsonl", - f"wc -l {outside}", - f"wc -l {symlink.name}", - "wc -l *.jsonl", - "wc -l -", - "wc -l", - "wc -l $TARGET.jsonl", - f"wc -l {small.name} {plain.name}", - f"wc -l --bytes {small.name}", - f"wc -c {small.name}", - f"wc -l {oversized.name}", - f"wc --lines {aggregate_a.name} {aggregate_b.name}", - ] - for command in commands: - assert _is_denied(_run_hook(_build_event(command), monkeypatch)), command - - -@pytest.mark.parametrize( - "command", - [ - "wc -l missing.jsonl 2>&1 | head -c 4000", - "wc --lines *.jsonl 1>.autoskillit/temp/wc.out 2>.autoskillit/temp/wc.err", - ], -) -def test_wc_lines_unsafe_operands_can_use_complete_byte_bound(command, monkeypatch, tmp_path): - (tmp_path / ".autoskillit" / "temp").mkdir(parents=True) - assert _run_hook(_build_event(command), monkeypatch) == "" - - -def test_large_jsonl_is_denied(monkeypatch, tmp_path): - path = tmp_path / "large.jsonl" - path.write_text('{"value": "' + ("x" * 5_100) + '"}\n') - assert _is_denied(_run_hook(_build_event(f"cat {path.name}"), monkeypatch)) - - -def test_real_large_jsonl_fixture_is_exercised_through_guard(monkeypatch, tmp_path): - source = Path(__file__).parents[1] / "fixtures" / "codex" / "large_embedded_payload_v1.jsonl" - target = tmp_path / source.name - shutil.copyfile(source, target) - assert target.stat().st_size > 5_000 - - for command in ( - f"rg -n payload {target.name}", - f"cat {target.name}", - f"jq -r .payload {target.name}", - ): - assert _is_denied(_run_hook(_build_event(command), monkeypatch)), command - assert ( - _run_hook( - _build_event(f"jq -r .payload {target.name} 2>&1 | head -c 4000"), - monkeypatch, - ) - == "" - ) - - -def test_symlink_jsonl_cannot_use_small_file_exception(monkeypatch, tmp_path): - target = tmp_path / "target.jsonl" - target.write_text("{}\n") - link = tmp_path / "link.jsonl" - link.symlink_to(target) - assert _is_denied(_run_hook(_build_event(f"cat {link.name}"), monkeypatch)) - - -def test_config_disable_and_overlay_priority(monkeypatch, tmp_path): - config_dir = tmp_path / ".autoskillit" / "temp" - config_dir.mkdir(parents=True) - base = config_dir / ".hook_config.json" - overlay = config_dir / ".hook_config_overlay.json" - base.write_text(json.dumps({"output_budget_policy": {"disabled": True}})) - - assert _run_hook(_build_event("rg pat src/"), monkeypatch) == "" - - overlay.write_text(json.dumps({"output_budget_policy": {"disabled": False}})) - assert _is_denied(_run_hook(_build_event("rg pat src/"), monkeypatch)) - - -def test_guard_fires_without_headless_gate(monkeypatch, tmp_path): - monkeypatch.delenv("AUTOSKILLIT_HEADLESS", raising=False) - (tmp_path / "src").mkdir() - assert _is_denied(_run_hook(_build_event("rg pat src/"), monkeypatch)) - - -def test_malformed_json_fails_open(monkeypatch): - assert _run_hook(None, monkeypatch, raw_stdin="not json {{{") == "" - - -def test_deny_reason_has_concrete_rewrite(monkeypatch, tmp_path): - from autoskillit.hooks.guards.output_budget_guard import ( # noqa: PLC0415 - OUTPUT_BUDGET_DENY_TRIGGER, - ) - - (tmp_path / "src").mkdir() - output = _run_hook(_build_event("rg pat src/"), monkeypatch) - reason = json.loads(output)["hookSpecificOutput"]["permissionDecisionReason"] - assert OUTPUT_BUDGET_DENY_TRIGGER in reason - assert "2>&1 | head -c 4000" in reason - assert ".autoskillit/temp/" in reason diff --git a/tests/hooks/test_policy_event.py b/tests/hooks/test_policy_event.py new file mode 100644 index 000000000..e5e802fad --- /dev/null +++ b/tests/hooks/test_policy_event.py @@ -0,0 +1,48 @@ +"""Tests for the typed policy-event formatter.""" + +from __future__ import annotations + +import pytest + +pytestmark = [pytest.mark.layer("hooks"), pytest.mark.small] + + +def test_render_provenance_prefix_contains_required_fields(): + from autoskillit.hooks._policy_event import PolicyEvent, render_provenance_prefix + + event = PolicyEvent( + hook_id="shell_capture_hook", + hook_version=1, + event="PreToolUse", + decision="deny", + reason_code="SHELL_OUTPUT_CAPTURED", + ) + prefix = render_provenance_prefix(event) + assert "AutoSkillit" in prefix + assert "shell_capture_hook" in prefix + assert "v1" in prefix + assert "deny" in prefix + assert "SHELL_OUTPUT_CAPTURED" in prefix + assert "permission decision" in prefix + assert "hook configured by this repository" in prefix + + +def test_render_is_single_line_and_stable(): + from autoskillit.hooks._policy_event import PolicyEvent, render_provenance_prefix + + event = PolicyEvent( + hook_id="shell_capture_hook", + hook_version=1, + event="PreToolUse", + decision="deny", + reason_code="SHELL_OUTPUT_CAPTURED", + ) + prefix = render_provenance_prefix(event) + assert "\n" not in prefix + assert prefix == ( + "[AutoSkillit hook shell_capture_hook v1" + " — PreToolUse permission decision: deny" + " (code=SHELL_OUTPUT_CAPTURED)." + " This is a real permission decision emitted by a hook" + " configured by this repository, not tool output.]" + ) diff --git a/tests/hooks/test_shell_capture_conformance.py b/tests/hooks/test_shell_capture_conformance.py new file mode 100644 index 000000000..cd74a7b8d --- /dev/null +++ b/tests/hooks/test_shell_capture_conformance.py @@ -0,0 +1,236 @@ +"""Hard semantic-conformance gate for the shell capture harness. + +Runs a corpus of shell commands both raw (``bash -c ``) and wrapped +(``bash -c ``), and asserts the harness is byte-exact and +exit-code-exact with the raw execution — the harness must never change what a +command does or what output the agent ultimately sees, only how much of that +output lands inline vs. in an artifact file. +""" + +from __future__ import annotations + +import shutil +import subprocess +from pathlib import Path + +import pytest + +from autoskillit.hooks.shell_capture_hook import _build_harness + +pytestmark = [pytest.mark.layer("hooks"), pytest.mark.medium] + +_INLINE_BYTES = 12_000 +_CAPTURE_SUBDIR = ".autoskillit/temp/shell_capture" +_TIMEOUT = 30 + +_NESTED_WRAP_INNER = "echo hi" + +_CORPUS = [ + ( + "pipe_wc", + "find . -path ./.autoskillit -prune -o -type f -print 2>&1 | wc -l | head -c 4000", + ), + ("ls_wc", "ls . 2>&1 | wc -l"), + ( + "heredoc_append", + "cat >> .autoskillit/temp/investigate/report.md <<'MARKER'\nsome content\nMARKER", + ), + ("multi_stmt", "cd /tmp && echo done # comment"), + ("exit_3", "exit 3"), + ("false_cmd", "false"), + ("mid_exit", "echo pre; exit 7; echo post"), + ("errexit", "set -e; false; echo unreachable"), + ("stderr_only", "echo err >&2"), + ("true_cmd", "true"), + ("self_bg", "{ sleep 0.2; echo late; } & echo started"), + ("large_output", "seq 1 200000"), + ("rg_sort", "rg pat . 2>&1 | sort | uniq -c | head -c 3000"), + ("jq_keys", "jq -c 'keys' x.jsonl 2>&1 | head -1 | head -c 1000"), + ("heredoc_no_newline", "cat <<'END'\nsome text\nEND"), + ("trailing_backslash", "echo one \\"), + ("self_signal", "echo pre; kill -TERM $$"), + ( + "unicode_heavy", + "python3 -c \"import sys; sys.stdout.buffer.write(b'\\xc3\\xa9' * 8000)\"", + ), + ("nested_wrap", None), # filled in below once _build_harness is available +] + + +def _make_project_dirs(tmp_path: Path) -> None: + (tmp_path / _CAPTURE_SUBDIR).mkdir(parents=True, exist_ok=True) + (tmp_path / ".autoskillit" / "temp" / "investigate").mkdir(parents=True, exist_ok=True) + (tmp_path / "x.jsonl").write_text('{"a":1}\n{"b":2}\n') + + +def _capture_dir(tmp_path: Path) -> Path: + return tmp_path / _CAPTURE_SUBDIR + + +def _artifact_files(tmp_path: Path) -> list[Path]: + return sorted(_capture_dir(tmp_path).glob("shell_*.log")) + + +def _run_raw(command: str, tmp_path: Path) -> subprocess.CompletedProcess[bytes]: + return subprocess.run( + ["bash", "-c", command], + capture_output=True, + cwd=str(tmp_path), + timeout=_TIMEOUT, + ) + + +def _run_raw_merged(command: str, tmp_path: Path) -> subprocess.CompletedProcess[bytes]: + return subprocess.run( + ["bash", "-c", command], + stdout=subprocess.PIPE, + stderr=subprocess.STDOUT, + cwd=str(tmp_path), + timeout=_TIMEOUT, + ) + + +def _run_wrapped(command: str, tmp_path: Path) -> subprocess.CompletedProcess[bytes]: + wrapped = _build_harness(command, str(tmp_path), _INLINE_BYTES) + return subprocess.run( + ["bash", "-c", wrapped], + capture_output=True, + cwd=str(tmp_path), + timeout=_TIMEOUT, + ) + + +# nested_wrap wraps a harness-of-"echo hi" and feeds it back through the +# harness a second time, proving the sentinel/idempotency path is also +# byte-safe under double-wrapping. +_CORPUS[-1] = ("nested_wrap", _build_harness(_NESTED_WRAP_INNER, "/tmp", _INLINE_BYTES)) + + +@pytest.mark.parametrize("label,command", _CORPUS, ids=[row[0] for row in _CORPUS]) +def test_capture_conformance(label: str, command: str, tmp_path: Path) -> None: + if label == "rg_sort" and shutil.which("rg") is None: + pytest.skip("rg not available") + if label == "jq_keys" and shutil.which("jq") is None: + pytest.skip("jq not available") + + _make_project_dirs(tmp_path) + + raw = _run_raw(command, tmp_path) + raw_combined = raw.stdout + raw.stderr + + if label == "heredoc_append": + # raw run already appended once; reset the target file so the + # wrapped run's append produces byte-identical content to compare. + report = tmp_path / ".autoskillit" / "temp" / "investigate" / "report.md" + report.unlink(missing_ok=True) + + wrapped = _run_wrapped(command, tmp_path) + + assert wrapped.returncode == raw.returncode, ( + f"[{label}] exit code mismatch: raw={raw.returncode} wrapped={wrapped.returncode}\n" + f"raw stderr={raw.stderr!r}\nwrapped stderr={wrapped.stderr!r}" + ) + + artifacts = _artifact_files(tmp_path) + + if label == "true_cmd": + assert raw_combined == b"" + assert wrapped.stdout == b"" + assert raw.returncode == 0 + assert not artifacts + return + + if label == "self_bg": + assert b"started" in wrapped.stdout + assert b"late" in wrapped.stdout + assert wrapped.returncode == 0 + return + + if label == "heredoc_append": + report = tmp_path / ".autoskillit" / "temp" / "investigate" / "report.md" + assert report.exists() + assert "some content" in report.read_text() + return + + if label == "nested_wrap": + assert wrapped.returncode == 0 + assert b"hi" in wrapped.stdout + return + + if label == "mid_exit": + assert raw.returncode == 7 + assert wrapped.returncode == 7 + + if label == "errexit": + assert raw.returncode == 1 + assert wrapped.returncode == 1 + + if label == "self_signal": + # subprocess.run reports signal-terminated processes as -signum + # (SIGTERM -> -15) rather than the 128+signum shells report via $?. + assert raw.returncode == -15 + assert wrapped.returncode == -15 + if artifacts: + assert b"pre" in artifacts[0].read_bytes() + return + + if label == "trailing_backslash": + return + + if label == "unicode_heavy": + assert artifacts, f"[{label}] expected an artifact for large unicode output" + assert len(artifacts) == 1, f"[{label}] expected exactly one artifact, found {artifacts}" + artifact_bytes = artifacts[0].read_bytes() + assert artifact_bytes == raw_combined, ( + f"[{label}] artifact content mismatch with raw combined output" + ) + artifact_bytes.decode("utf-8") + return + + if len(raw_combined) <= _INLINE_BYTES: + assert wrapped.stdout == raw_combined, ( + f"[{label}] inline output mismatch.\nraw={raw_combined!r}\nwrapped={wrapped.stdout!r}" + ) + assert not artifacts, f"[{label}] expected no artifact for small output, found {artifacts}" + else: + assert artifacts, f"[{label}] expected an artifact for large output, found none" + assert len(artifacts) == 1, f"[{label}] expected exactly one artifact, found {artifacts}" + artifact_bytes = artifacts[0].read_bytes() + assert artifact_bytes == raw_combined, ( + f"[{label}] artifact content mismatch with raw combined output" + ) + + +def test_interleaved_stdout_stderr_ordering(tmp_path: Path) -> None: + """Verify harness preserves interleaved stdout+stderr ordering via 2>&1.""" + command = "echo out1; echo err1 >&2; echo out2; echo err2 >&2; echo out3" + _make_project_dirs(tmp_path) + + raw_merged = _run_raw_merged(command, tmp_path) + wrapped = _run_wrapped(command, tmp_path) + + assert wrapped.returncode == raw_merged.returncode == 0 + + artifacts = _artifact_files(tmp_path) + if artifacts: + actual = artifacts[0].read_bytes() + else: + actual = wrapped.stdout + + assert actual == raw_merged.stdout, ( + f"Interleaved ordering mismatch.\n raw_merged={raw_merged.stdout!r}\n actual={actual!r}" + ) + + +def test_capture_dir_uncreatable_fail_stops(tmp_path: Path) -> None: + blocking_dir = tmp_path / ".autoskillit" / "temp" + blocking_dir.mkdir(parents=True) + (blocking_dir / "shell_capture").write_text("not a directory") + + command = "echo should_not_run" + wrapped = _run_wrapped(command, tmp_path) + + assert wrapped.returncode == 1 + combined = (wrapped.stdout + wrapped.stderr).decode(errors="replace") + assert "capture_failed" in combined.lower() or "CAPTURE_FAILED" in combined + assert "should_not_run" not in combined diff --git a/tests/hooks/test_shell_capture_hook.py b/tests/hooks/test_shell_capture_hook.py new file mode 100644 index 000000000..02c43c1c4 --- /dev/null +++ b/tests/hooks/test_shell_capture_hook.py @@ -0,0 +1,139 @@ +"""Tests for the Codex native shell capture PreToolUse hook.""" + +from __future__ import annotations + +import io +import json +from contextlib import redirect_stdout + +import pytest + +pytestmark = [pytest.mark.layer("hooks"), pytest.mark.small] + +_SENTINEL = "# autoskillit-shell-capture v1" + + +def _build_event(command, cwd: str = "/abs/project") -> dict: + return {"cwd": cwd, "tool_input": {"command": command}} + + +def _run_hook(event_data, monkeypatch, *, env_backend: str | None = None) -> str: + from autoskillit.hooks.shell_capture_hook import main # noqa: PLC0415 + + if env_backend is not None: + monkeypatch.setenv("AUTOSKILLIT_AGENT_BACKEND", env_backend) + else: + monkeypatch.delenv("AUTOSKILLIT_AGENT_BACKEND", raising=False) + + stdin_text = event_data if isinstance(event_data, str) else json.dumps(event_data) + monkeypatch.setattr("sys.stdin", io.StringIO(stdin_text)) + + output = io.StringIO() + with pytest.raises(SystemExit), redirect_stdout(output): + main() + return output.getvalue() + + +def _updated_command(output: str) -> str: + payload = json.loads(output) + return payload["hookSpecificOutput"]["updatedInput"]["command"] + + +def test_silent_off_codex(monkeypatch): + event = _build_event("echo hello") + + monkeypatch.delenv("AUTOSKILLIT_AGENT_BACKEND", raising=False) + assert _run_hook(event, monkeypatch) == "" + + assert _run_hook(event, monkeypatch, env_backend="claude_code") == "" + + +def test_rewrites_on_codex_env(monkeypatch): + output = _run_hook(_build_event("echo hello"), monkeypatch, env_backend="codex") + + payload = json.loads(output) + assert payload["hookSpecificOutput"]["permissionDecision"] == "allow" + command = payload["hookSpecificOutput"]["updatedInput"]["command"] + assert _SENTINEL in command + assert "echo hello" in command + + +def test_rewrites_on_turn_id_payload(monkeypatch): + event = _build_event("echo hello") + event["turn_id"] = "turn-1" + + output = _run_hook(event, monkeypatch) + + payload = json.loads(output) + assert payload["hookSpecificOutput"]["permissionDecision"] == "allow" + command = payload["hookSpecificOutput"]["updatedInput"]["command"] + assert _SENTINEL in command + assert "echo hello" in command + + +def test_always_wraps_sentinel_prefixed_command(monkeypatch): + already_wrapped = f"{_SENTINEL}\necho hi" + output = _run_hook(_build_event(already_wrapped), monkeypatch, env_backend="codex") + + command = _updated_command(output) + assert command.count(_SENTINEL) == 2 + assert already_wrapped in command + + +def test_newline_normalization_applied(monkeypatch): + output = _run_hook(_build_event("echo test"), monkeypatch, env_backend="codex") + + command = _updated_command(output) + assert "echo test\n)" in command + + +def test_disabled_policy_no_rewrite(monkeypatch, tmp_path): + config_dir = tmp_path / ".autoskillit" / "temp" + config_dir.mkdir(parents=True) + (config_dir / ".hook_config.json").write_text( + json.dumps({"output_budget_policy": {"disabled": True}}) + ) + + event = _build_event("echo hello", cwd=str(tmp_path)) + assert _run_hook(event, monkeypatch, env_backend="codex") == "" + + +def test_malformed_stdin_fails_open(monkeypatch): + assert _run_hook("not json", monkeypatch, env_backend="codex") == "" + + +def test_missing_cwd_fails_open(monkeypatch): + event = {"tool_input": {"command": "echo hello"}} + assert _run_hook(event, monkeypatch, env_backend="codex") == "" + + relative_event = _build_event("echo hello", cwd="relative/path") + assert _run_hook(relative_event, monkeypatch, env_backend="codex") == "" + + +def test_non_string_command_fails_open(monkeypatch): + event = {"cwd": "/abs/project", "tool_input": {"command": 5}} + assert _run_hook(event, monkeypatch, env_backend="codex") == "" + + +def test_marker_provenance_and_shell_safety(monkeypatch): + output = _run_hook(_build_event("echo hello"), monkeypatch, env_backend="codex") + command = _updated_command(output) + + assert "AutoSkillit" in command + assert "shell_capture_hook" in command + + import re + + marker_matches = re.findall(r"printf '\\n%s\\n' (.+)", command) + assert marker_matches, "expected a printf-embedded capture marker in the harness" + marker = marker_matches[0] + + assert "`" not in marker + + for var in ("$__as_sz", "$__as_f", "$__as_sha"): + assert f'"{var}"' in marker, f"expected expansion-safe {var} in marker" + + stripped = marker + for var in ("$__as_sz", "$__as_f", "$__as_sha"): + stripped = stripped.replace(f'"{var}"', "") + assert "$" not in stripped diff --git a/tests/infra/test_release_sanity.py b/tests/infra/test_release_sanity.py index eb9231561..35e524647 100644 --- a/tests/infra/test_release_sanity.py +++ b/tests/infra/test_release_sanity.py @@ -8,9 +8,7 @@ pytestmark = [pytest.mark.layer("infra"), pytest.mark.medium] REPO_ROOT = Path(__file__).parent.parent.parent -_PERSONAL_HOME_PATH_FIXTURES: dict[Path, frozenset[int]] = { - Path("tests/hooks/test_output_budget_guard.py"): frozenset({17, 18}), -} +_PERSONAL_HOME_PATH_FIXTURES: dict[Path, frozenset[int]] = {} def test_no_personal_home_paths_in_test_files(): diff --git a/tests/infra/test_schema_version_convention.py b/tests/infra/test_schema_version_convention.py index 46e6cb4b5..079299ab3 100644 --- a/tests/infra/test_schema_version_convention.py +++ b/tests/infra/test_schema_version_convention.py @@ -119,10 +119,10 @@ def _is_yaml_dump(node: ast.expr) -> bool: # _lifespan.py — hooks.json self-heal on startup drift (co-owned with Claude plugin system) ("src/autoskillit/server/_lifespan.py", 90), # tools_kitchen.py — hook config, quota guard, git_ops_policy, ingredient locks overlay - ("src/autoskillit/server/tools/tools_kitchen.py", 274), - ("src/autoskillit/server/tools/tools_kitchen.py", 293), - ("src/autoskillit/server/tools/tools_kitchen.py", 327), - ("src/autoskillit/server/tools/tools_kitchen.py", 1303), + ("src/autoskillit/server/tools/tools_kitchen.py", 272), + ("src/autoskillit/server/tools/tools_kitchen.py", 291), + ("src/autoskillit/server/tools/tools_kitchen.py", 325), + ("src/autoskillit/server/tools/tools_kitchen.py", 1301), # tools_pipeline_tracker.py — tracker_data dict ("src/autoskillit/server/tools/tools_pipeline_tracker.py", 166), # tools_status.py — mcp_data dict diff --git a/tests/server/test_tools_execution_spill.py b/tests/server/test_tools_execution_spill.py index ed8ac06dc..9c5001233 100644 --- a/tests/server/test_tools_execution_spill.py +++ b/tests/server/test_tools_execution_spill.py @@ -2,13 +2,17 @@ from __future__ import annotations +import errno import json import uuid +from pathlib import Path from types import SimpleNamespace from unittest.mock import AsyncMock, patch import pytest +from autoskillit.core import SubprocessResult, TerminationReason +from autoskillit.execution.process import CaptureReadError, CaptureSetupError, summarize_capture from autoskillit.server._response_budget import RESPONSE_SPILL_METADATA_KEY from autoskillit.server.tools.tools_execution import run_cmd, run_python, run_skill from tests.conftest import _make_result @@ -16,12 +20,46 @@ pytestmark = [pytest.mark.layer("server"), pytest.mark.small] +def _write_capture_result( + capture_dir: Path, + *, + returncode: int = 0, + stdout: str = "", + stderr: str = "", +) -> SubprocessResult: + """Write stdout/stderr content to real files under capture_dir and return the result. + + Mirrors what the real ``_run_subprocess_captured`` -> ``run_managed_async`` path + does: stdout/stderr strings are empty; the content lives in files referenced by + stdout_path/stderr_path. + """ + capture_dir.mkdir(parents=True, exist_ok=True) + stdout_path = capture_dir / f"proc_stdout_{uuid.uuid4().hex[:8]}.tmp" + stderr_path = capture_dir / f"proc_stderr_{uuid.uuid4().hex[:8]}.tmp" + stdout_path.write_text(stdout, encoding="utf-8") + stderr_path.write_text(stderr, encoding="utf-8") + return SubprocessResult( + returncode=returncode, + stdout="", + stderr="", + termination=TerminationReason.NATURAL_EXIT, + pid=12345, + stdout_path=stdout_path, + stderr_path=stderr_path, + ) + + @pytest.mark.anyio async def test_run_cmd_spills_large_stdout_under_calling_project(tool_ctx_kitchen_open, tmp_path): stdout = "head-sentinel\n" + ("x" * 10_000) + "\ntail-sentinel" + capture_dir = tmp_path / ".autoskillit" / "temp" / "run_cmd" + + async def _fake_captured(cmd, *, cwd, timeout, env=None, capture_dir=capture_dir): + return _write_capture_result(capture_dir, stdout=stdout) + with patch( - "autoskillit.server.tools.tools_execution._run_subprocess", - new=AsyncMock(return_value=(0, stdout, "")), + "autoskillit.server.tools.tools_execution._run_subprocess_captured", + new=AsyncMock(side_effect=_fake_captured), ): data = json.loads(await run_cmd("bounded-command", str(tmp_path))) @@ -43,10 +81,16 @@ async def test_run_cmd_resolves_absolute_cwd_only_for_spill_anchor( project_link = tmp_path / "project-link" project_link.symlink_to(project, target_is_directory=True) stdout = "x" * 10_000 - subprocess = AsyncMock(return_value=(0, stdout, "")) + capture_dir = project / ".autoskillit" / "temp" / "run_cmd" + + subprocess = AsyncMock( + side_effect=lambda cmd, *, cwd, timeout, env=None, capture_dir=capture_dir: ( + _write_capture_result(capture_dir, stdout=stdout) + ) + ) with patch( - "autoskillit.server.tools.tools_execution._run_subprocess", + "autoskillit.server.tools.tools_execution._run_subprocess_captured", new=subprocess, ): data = json.loads(await run_cmd("bounded-command", str(project_link))) @@ -61,9 +105,14 @@ async def test_run_cmd_resolves_absolute_cwd_only_for_spill_anchor( @pytest.mark.parametrize("cwd", ["", "relative-project"]) async def test_run_cmd_empty_or_relative_cwd_uses_injected_temp_dir(tool_ctx_kitchen_open, cwd): stdout = "x" * 10_000 + capture_dir = tool_ctx_kitchen_open.temp_dir / "run_cmd" + + async def _fake_captured(cmd, *, cwd, timeout, env=None, capture_dir=capture_dir): + return _write_capture_result(capture_dir, stdout=stdout) + with patch( - "autoskillit.server.tools.tools_execution._run_subprocess", - new=AsyncMock(return_value=(0, stdout, "")), + "autoskillit.server.tools.tools_execution._run_subprocess_captured", + new=AsyncMock(side_effect=_fake_captured), ): data = json.loads(await run_cmd("bounded-command", cwd)) @@ -72,9 +121,14 @@ async def test_run_cmd_empty_or_relative_cwd_uses_injected_temp_dir(tool_ctx_kit @pytest.mark.anyio async def test_run_cmd_small_output_shape_is_unchanged(tool_ctx_kitchen_open, tmp_path): + capture_dir = tmp_path / ".autoskillit" / "temp" / "run_cmd" + + async def _fake_captured(cmd, *, cwd, timeout, env=None, capture_dir=capture_dir): + return _write_capture_result(capture_dir, stdout="small") + with patch( - "autoskillit.server.tools.tools_execution._run_subprocess", - new=AsyncMock(return_value=(0, "small", "")), + "autoskillit.server.tools.tools_execution._run_subprocess_captured", + new=AsyncMock(side_effect=_fake_captured), ): raw = await run_cmd("bounded-command", str(tmp_path)) @@ -248,3 +302,197 @@ async def test_decorated_dispatch_preserves_routing_scalars_artifact_and_small_i await tools_fleet_dispatch.dispatch_food_truck(recipe="probe", task="small") == small_envelope ) + + +@pytest.mark.anyio +async def test_run_cmd_capture_read_failure_is_fail_stop( + tool_ctx_kitchen_open, tmp_path, monkeypatch +): + capture_dir = tmp_path / ".autoskillit" / "temp" / "run_cmd" + + async def _fake_captured(cmd, *, cwd, timeout, env=None, capture_dir=capture_dir): + return _write_capture_result(capture_dir, returncode=3, stdout="some output") + + def _raise_read_error(path, spec, *, complete=True): + raise CaptureReadError(errno.EIO, "test read error") + + monkeypatch.setattr( + "autoskillit.server.tools._execution_helpers.summarize_capture", + _raise_read_error, + ) + with patch( + "autoskillit.server.tools.tools_execution._run_subprocess_captured", + new=AsyncMock(side_effect=_fake_captured), + ): + data = json.loads(await run_cmd("bounded-command", str(tmp_path))) + + assert data["success"] is False + assert data["error"].startswith("capture_failed") + assert data["exit_code"] == 3 + + +@pytest.mark.anyio +async def test_run_cmd_capture_setup_failure_is_fail_stop(tool_ctx_kitchen_open, tmp_path): + # Simulates the real create_temp_io() failure mode: the capture directory path + # exists but is a regular file, so mkdir(parents=True, exist_ok=True) raises + # OSError, which run_managed_async wraps into CaptureSetupError before any + # subprocess is spawned. + subprocess = AsyncMock( + side_effect=CaptureSetupError("Cannot create capture directory: not a directory") + ) + with patch( + "autoskillit.server.tools.tools_execution._run_subprocess_captured", + new=subprocess, + ): + data = json.loads(await run_cmd("bounded-command", str(tmp_path))) + + assert data["exit_code"] == -1 + assert data["success"] is False + assert tool_ctx_kitchen_open.runner.call_args_list == [] + + +@pytest.mark.anyio +async def test_run_cmd_timeout_promotes_partial_capture(tool_ctx_kitchen_open, tmp_path): + stdout = "partial-head\n" + ("x" * 10_000) + "\npartial-tail" + capture_dir = tmp_path / ".autoskillit" / "temp" / "run_cmd" + + async def _fake_captured(cmd, *, cwd, timeout, env=None, capture_dir=capture_dir): + result = _write_capture_result(capture_dir, returncode=-1, stdout=stdout) + return SubprocessResult( + returncode=result.returncode, + stdout="", + stderr="", + termination=TerminationReason.TIMED_OUT, + pid=result.pid, + stdout_path=result.stdout_path, + stderr_path=result.stderr_path, + ) + + with patch( + "autoskillit.server.tools.tools_execution._run_subprocess_captured", + new=AsyncMock(side_effect=_fake_captured), + ): + data = json.loads(await run_cmd("bounded-command", str(tmp_path))) + + assert data["success"] is False + assert data["exit_code"] == -1 + assert "timed out" in data["error"] + assert "partial-head" in data["stdout"] + assert "partial-tail" in data["stdout"] + assert "complete=false" in data["stdout"] + + +@pytest.mark.anyio +async def test_run_cmd_never_materializes_full_output( + tool_ctx_kitchen_open, tmp_path, monkeypatch +): + def _raise_read_temp_output(stdout_path, stderr_path): + raise RuntimeError("read_temp_output must not be called on the capture path") + + monkeypatch.setattr( + "autoskillit.execution.process._process_io.read_temp_output", + _raise_read_temp_output, + ) + stdout = "head-sentinel\n" + ("x" * 50_000) + "\ntail-sentinel" + capture_dir = tmp_path / ".autoskillit" / "temp" / "run_cmd" + + async def _fake_captured(cmd, *, cwd, timeout, env=None, capture_dir=capture_dir): + return _write_capture_result(capture_dir, stdout=stdout) + + with patch( + "autoskillit.server.tools.tools_execution._run_subprocess_captured", + new=AsyncMock(side_effect=_fake_captured), + ): + data = json.loads(await run_cmd("bounded-command", str(tmp_path))) + + assert data["success"] is True + assert data["exit_code"] == 0 + assert "head-sentinel" in data["stdout"] + assert "tail-sentinel" in data["stdout"] + + +@pytest.mark.anyio +async def test_run_cmd_partial_capture_error_leaves_no_orphan( + tool_ctx_kitchen_open, tmp_path, monkeypatch +): + capture_dir = tmp_path / ".autoskillit" / "temp" / "run_cmd" + + async def _fake_captured(cmd, *, cwd, timeout, env=None, capture_dir=capture_dir): + return _write_capture_result( + capture_dir, returncode=0, stdout="small", stderr="also small" + ) + + real_summarize = summarize_capture + + def _fail_stderr_only(path, spec, *, complete=True): + if "stderr" in path.name: + raise CaptureReadError(errno.EIO, "test stderr read error") + return real_summarize(path, spec, complete=complete) + + monkeypatch.setattr( + "autoskillit.server.tools._execution_helpers.summarize_capture", + _fail_stderr_only, + ) + with patch( + "autoskillit.server.tools.tools_execution._run_subprocess_captured", + new=AsyncMock(side_effect=_fake_captured), + ): + data = json.loads(await run_cmd("bounded-command", str(tmp_path))) + + assert data["success"] is False + assert data["error"].startswith("capture_failed") + # stdout succeeded and was inline-sized -> its capture file must be unlinked, not orphaned. + remaining_stdout = list(capture_dir.glob("proc_stdout_*.tmp")) + assert remaining_stdout == [] + # stderr failed summarize_capture() -> its raw file must also be unlinked, not orphaned. + remaining_stderr = list(capture_dir.glob("proc_stderr_*.tmp")) + assert remaining_stderr == [] + + +@pytest.mark.anyio +async def test_run_cmd_signal_death_marks_incomplete(tool_ctx_kitchen_open, tmp_path): + stdout = "before-signal\n" + ("x" * 10_000) + "\nnever-reached" + capture_dir = tmp_path / ".autoskillit" / "temp" / "run_cmd" + + async def _fake_captured(cmd, *, cwd, timeout, env=None, capture_dir=capture_dir): + result = _write_capture_result(capture_dir, returncode=137, stdout=stdout) + return SubprocessResult( + returncode=137, + stdout="", + stderr="", + termination=TerminationReason.SIGNAL_DEATH, + pid=result.pid, + stdout_path=result.stdout_path, + stderr_path=result.stderr_path, + ) + + with patch( + "autoskillit.server.tools.tools_execution._run_subprocess_captured", + new=AsyncMock(side_effect=_fake_captured), + ): + data = json.loads(await run_cmd("bounded-command", str(tmp_path))) + + assert data["exit_code"] == 137 + assert data["success"] is False + assert "signal" in data["error"].lower() + assert "complete=false" in data["stdout"] + + +def test_summarize_capture_never_loads_full_content(tmp_path): + """Verify summarize_capture returns bounded slices without materializing the full file.""" + from autoskillit.core import SpillSpec + + large_file = tmp_path / "big_output.tmp" + content = b"HEAD-MARKER\n" + (b"x" * 200_000) + b"\nTAIL-MARKER\n" + large_file.write_bytes(content) + + spec = SpillSpec(inline_max_chars=1000, head_chars=500, tail_chars=500) + result = summarize_capture(large_file, spec, complete=True) + + assert result.inline_text is None + assert result.total_bytes == len(content) + assert len(result.head) <= spec.head_chars + assert len(result.tail) <= spec.tail_chars + assert "HEAD-MARKER" in result.head + assert "TAIL-MARKER" in result.tail + assert len(result.sha256) == 64 diff --git a/tests/server/test_tools_kitchen_gate_hook_config.py b/tests/server/test_tools_kitchen_gate_hook_config.py index 7f53cca85..d60fd3b63 100644 --- a/tests/server/test_tools_kitchen_gate_hook_config.py +++ b/tests/server/test_tools_kitchen_gate_hook_config.py @@ -151,7 +151,6 @@ async def test_open_kitchen_bridges_output_budget_policy( mock_ctx.config.quota_guard = QuotaGuardConfig() mock_ctx.config.output_budget = OutputBudgetConfig( guard_enabled=guard_enabled, - small_file_max_bytes=4321, shell_max_inline_bytes=8765, ) @@ -167,7 +166,6 @@ async def test_open_kitchen_bridges_output_budget_policy( data = json.loads(tmp_path.joinpath(*_HOOK_CONFIG_PATH_COMPONENTS).read_text()) assert data["output_budget_policy"] == { "disabled": expected_disabled, - "small_file_max_bytes": 4321, "shell_max_inline_bytes": 8765, } diff --git a/tests/server/test_tools_run_cmd_unit.py b/tests/server/test_tools_run_cmd_unit.py index 693a3c821..f720498d2 100644 --- a/tests/server/test_tools_run_cmd_unit.py +++ b/tests/server/test_tools_run_cmd_unit.py @@ -133,7 +133,7 @@ async def test_run_cmd_with_step_name_records_non_session_step( mock_recorder.record_non_session_step.assert_called_once_with( step_name="setup", tool="run_cmd", - result_summary={"exit_code": 0, "stdout_head": "task output"}, + result_summary={"exit_code": 0, "stdout_head": "(capture mode)"}, ) @pytest.mark.anyio diff --git a/tests/skills_extended/test_audit_friction_output_budget.py b/tests/skills_extended/test_audit_friction_output_budget.py deleted file mode 100644 index 62e2f970d..000000000 --- a/tests/skills_extended/test_audit_friction_output_budget.py +++ /dev/null @@ -1,127 +0,0 @@ -"""Source-parity coverage for audit-friction's bounded JSONL command battery.""" - -from __future__ import annotations - -import json -import os -import re -import subprocess -import sys -from pathlib import Path - -import pytest - -pytestmark = pytest.mark.medium - -PROJECT_ROOT = Path(__file__).resolve().parents[2] -SKILL_PATH = ( - PROJECT_ROOT / "src" / "autoskillit" / "skills_extended" / "audit-friction" / "SKILL.md" -) -GUARD_PATH = PROJECT_ROOT / "src" / "autoskillit" / "hooks" / "guards" / "output_budget_guard.py" - - -def _extract_battery_commands() -> tuple[str, ...]: - source = SKILL_PATH.read_text(encoding="utf-8") - section = source.split("Use this keyword battery against each file:", 1)[1] - bash_block = section.split("```bash", 1)[1].split("```", 1)[0] - return tuple( - line.strip() - for line in bash_block.splitlines() - if line.strip() and not line.lstrip().startswith("#") - ) - - -BATTERY_COMMANDS = _extract_battery_commands() - - -def test_real_command_battery_is_structurally_byte_bounded() -> None: - assert len(BATTERY_COMMANDS) == 8 - for command in BATTERY_COMMANDS: - assert not re.search(r"(^|\s)grep(?:\s|$)", command) - assert command.endswith("head -c 12000") - - stages = command.split(" | ") - assert stages[-1] == "head -c 12000" - assert all(stage.endswith("2>&1") for stage in stages[:-1]) - - jsonl_reader = stages[0] - assert jsonl_reader.startswith("rg ") - assert re.search(r"(?:^|\s)-M\s+500(?:\s|$)", jsonl_reader) - - -@pytest.mark.parametrize("command", BATTERY_COMMANDS) -def test_real_command_battery_passes_output_budget_guard( - command: str, - tmp_path: Path, -) -> None: - log_path = tmp_path / "session.jsonl" - log_path.write_text(json.dumps({"payload": "x" * 6000}) + "\n", encoding="utf-8") - rendered = ( - command.replace("FILE", str(log_path)) - .replace("TOOL_NAME", "Read") - .replace("CONFIRMING_PATTERN", "payload") - ) - payload = { - "tool_name": "Bash", - "tool_input": {"command": rendered}, - "cwd": str(tmp_path), - } - isolated_home = tmp_path / "home" - isolated_home.mkdir() - env = {key: value for key, value in os.environ.items() if not key.startswith("AUTOSKILLIT_")} - env.update( - { - "AUTOSKILLIT_CWD": str(tmp_path), - "HOME": str(isolated_home), - "XDG_CONFIG_HOME": str(isolated_home / ".config"), - } - ) - - result = subprocess.run( # noqa: S603 - [sys.executable, str(GUARD_PATH)], - input=json.dumps(payload), - capture_output=True, - text=True, - cwd=tmp_path, - env=env, - timeout=5, - ) - - assert result.returncode == 0, result.stderr - if result.stdout.strip(): - hook_output = json.loads(result.stdout)["hookSpecificOutput"] - assert hook_output.get("permissionDecision") != "deny", ( - f"real audit-friction command was denied: {rendered}\n{result.stdout}" - ) - - -def test_guard_denies_known_hazardous_command(tmp_path: Path) -> None: - """Guard actually classifies input (not silently bailing on all commands).""" - payload = { - "tool_name": "Bash", - "tool_input": {"command": "grep -r TODO ."}, - "cwd": str(tmp_path), - } - isolated_home = tmp_path / "home" - isolated_home.mkdir() - env = {key: value for key, value in os.environ.items() if not key.startswith("AUTOSKILLIT_")} - env.update( - { - "AUTOSKILLIT_CWD": str(tmp_path), - "HOME": str(isolated_home), - "XDG_CONFIG_HOME": str(isolated_home / ".config"), - } - ) - result = subprocess.run( # noqa: S603 - [sys.executable, str(GUARD_PATH)], - input=json.dumps(payload), - capture_output=True, - text=True, - cwd=tmp_path, - env=env, - timeout=5, - ) - assert result.returncode == 0, result.stderr - assert result.stdout.strip(), "guard produced no output for a hazardous command" - hook_output = json.loads(result.stdout)["hookSpecificOutput"] - assert hook_output.get("permissionDecision") == "deny"