From 79fc1e2a0f9b6817110f392f9cd2e6677011ad6b Mon Sep 17 00:00:00 2001 From: Trecek Date: Wed, 22 Jul 2026 20:17:33 -0700 Subject: [PATCH 01/32] feat: add descriptor-anchored capture authority --- src/autoskillit/hooks/AGENTS.md | 1 + src/autoskillit/hooks/_capture_artifacts.py | 687 ++++++++++++++++++++ tests/hooks/AGENTS.md | 1 + tests/hooks/test_capture_artifacts.py | 266 ++++++++ 4 files changed, 955 insertions(+) create mode 100644 src/autoskillit/hooks/_capture_artifacts.py create mode 100644 tests/hooks/test_capture_artifacts.py diff --git a/src/autoskillit/hooks/AGENTS.md b/src/autoskillit/hooks/AGENTS.md index ef56bb216..d0d7d1a25 100644 --- a/src/autoskillit/hooks/AGENTS.md +++ b/src/autoskillit/hooks/AGENTS.md @@ -24,6 +24,7 @@ Sub-packages: guards/ (see guards/AGENTS.md), formatters/ (see formatters/AGENTS | `_hook_utils.py` | Shared stdlib-only utilities for hook scripts (e.g., `find_project_root`, `STEP_SUFFIX_RE`) | | `_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) | +| `_capture_artifacts.py` | Stdlib-only descriptor-anchored shell-capture authority, runner, and cleanup classifier | | `_capture_cleanup.py` | Stdlib-only stale-capture-file sweep helpers (used by `session_start_hook.py`) | | `shell_capture_hook.py` | `PreToolUse`: input-rewrite hook for Codex shell capture — wraps commands in a lossless capture harness (#4286 / ADR-0006) | diff --git a/src/autoskillit/hooks/_capture_artifacts.py b/src/autoskillit/hooks/_capture_artifacts.py new file mode 100644 index 000000000..c2dae6f6e --- /dev/null +++ b/src/autoskillit/hooks/_capture_artifacts.py @@ -0,0 +1,687 @@ +"""Descriptor-anchored shell-capture artifacts and lifecycle helpers. + +This module is stdlib-only and is executable under Python isolated mode. It +owns the trust boundary for capture policy reads, artifact creation, replay, +and stale-candidate classification. +""" + +from __future__ import annotations + +import base64 +import binascii +import errno +import hashlib +import json +import os +import re +import shutil +import stat +import subprocess +import sys +import time +from dataclasses import dataclass +from pathlib import Path + +_HOOKS_DIR = str(Path(__file__).resolve().parent) +if _HOOKS_DIR not in sys.path: + sys.path.insert(0, _HOOKS_DIR) + +from _hook_settings import ( # type: ignore[import-not-found] # noqa: E402 + HOOK_CONFIG_FILENAME, + HOOK_CONFIG_OVERLAY_FILENAME, + merge_hook_configs, +) +from _policy_event import ( # type: ignore[import-not-found] # noqa: E402 + PolicyEvent, + render_capture_marker, +) + +__all__ = [ + "CAPTURE_PATH_COMPONENTS", + "CaptureArtifact", + "CapturePolicy", + "CaptureRoot", + "CaptureSetupError", + "ProjectAnchor", + "_CAPTURE_FILENAME_RE", + "create_capture_artifact", + "current_artifact_path_if_bound", + "open_capture_root", + "open_project_anchor", + "read_capture_policy", + "run_capture", + "sweep_stale_captures", +] + +CAPTURE_PATH_COMPONENTS = (".autoskillit", "temp", "shell_capture") +_CAPTURE_FILENAME_RE = re.compile(r"^shell_[0-9a-f]{16}\.log$") +_CAPTURE_ID_RE = re.compile(r"^[0-9a-f]{16}$") + +_DEFAULT_INLINE_BYTES = 12_000 +_MAX_INLINE_BYTES = 1_000_000 +_MAX_POLICY_FILE_BYTES = 64 * 1024 +_MAX_COMMAND_BYTES = 64 * 1024 +_MAX_ENCODED_COMMAND_BYTES = ((_MAX_COMMAND_BYTES + 2) // 3) * 4 +_DRAIN_CHUNK_BYTES = 64 * 1024 + +_DIRECTORY_FLAGS = ( + os.O_RDONLY + | getattr(os, "O_DIRECTORY", 0) + | getattr(os, "O_CLOEXEC", 0) + | getattr(os, "O_NOFOLLOW", 0) +) +_ARTIFACT_FLAGS = ( + os.O_RDWR | os.O_CREAT | os.O_EXCL | getattr(os, "O_CLOEXEC", 0) | getattr(os, "O_NOFOLLOW", 0) +) +_READ_FLAGS = os.O_RDONLY | getattr(os, "O_CLOEXEC", 0) | getattr(os, "O_NOFOLLOW", 0) + + +class CaptureSetupError(RuntimeError): + """Raised when the descriptor-anchored capture authority cannot be established.""" + + +@dataclass(frozen=True, slots=True) +class FileIdentity: + device: int + inode: int + + @classmethod + def from_stat(cls, value: os.stat_result) -> FileIdentity: + return cls(device=value.st_dev, inode=value.st_ino) + + +@dataclass(slots=True) +class ProjectAnchor: + """Opened project directory and its post-open physical-path hint.""" + + fd: int + identity: FileIdentity + physical_path: Path + + def close(self) -> None: + if self.fd >= 0: + os.close(self.fd) + self.fd = -1 + + +@dataclass(slots=True) +class CaptureRoot: + """Opened capture-root chain retained for the capture lifetime.""" + + autoskillit_fd: int + temp_fd: int + fd: int + autoskillit_identity: FileIdentity + temp_identity: FileIdentity + identity: FileIdentity + + def close(self) -> None: + for field_name in ("fd", "temp_fd", "autoskillit_fd"): + fd = getattr(self, field_name) + if fd >= 0: + os.close(fd) + setattr(self, field_name, -1) + + +@dataclass(slots=True) +class CaptureArtifact: + """Exclusive capture artifact retained by descriptor.""" + + fd: int + name: str + identity: FileIdentity + + def close(self) -> None: + if self.fd >= 0: + os.close(self.fd) + self.fd = -1 + + +@dataclass(frozen=True, slots=True) +class CapturePolicy: + disabled: bool = False + inline_bytes: int = _DEFAULT_INLINE_BYTES + + +@dataclass(frozen=True, slots=True) +class _DrainResult: + total_bytes: int + sha256: str + inline: bytes + head: bytes + tail: bytes + write_error: OSError | None + + +def _require_capabilities() -> None: + required_dir_fd = (os.open, os.mkdir) + if ( + getattr(os, "O_DIRECTORY", 0) == 0 + or getattr(os, "O_NOFOLLOW", 0) == 0 + or not hasattr(os, "fchdir") + or any(function not in os.supports_dir_fd for function in required_dir_fd) + ): + raise CaptureSetupError("required descriptor-relative filesystem primitives unavailable") + + +def _identity(fd: int) -> FileIdentity: + return FileIdentity.from_stat(os.fstat(fd)) + + +def _same_identity(fd: int, expected: FileIdentity) -> bool: + return _identity(fd) == expected + + +def _open_directory_component(parent_fd: int, name: str, *, create: bool) -> int: + try: + fd = os.open(name, _DIRECTORY_FLAGS, dir_fd=parent_fd) + except FileNotFoundError: + if not create: + raise CaptureSetupError(f"missing capture path component: {name}") from None + try: + os.mkdir(name, mode=0o700, dir_fd=parent_fd) + except FileExistsError: + pass + except OSError as exc: + raise CaptureSetupError(f"cannot create capture path component: {name}") from exc + try: + fd = os.open(name, _DIRECTORY_FLAGS, dir_fd=parent_fd) + except OSError as exc: + raise CaptureSetupError(f"cannot open created capture component: {name}") from exc + except OSError as exc: + raise CaptureSetupError(f"unsafe capture path component: {name}") from exc + + try: + if not stat.S_ISDIR(os.fstat(fd).st_mode): + raise CaptureSetupError(f"capture path component is not a directory: {name}") + except BaseException: + os.close(fd) + raise + return fd + + +def open_project_anchor(cwd: str) -> ProjectAnchor: + """Open the supplied cwd first; a symlink in the supplied spelling is allowed.""" + + _require_capabilities() + if not isinstance(cwd, str) or not cwd or not os.path.isabs(cwd) or "\x00" in cwd: + raise CaptureSetupError("cwd must be a non-empty absolute path") + try: + fd = os.open(cwd, _DIRECTORY_FLAGS & ~getattr(os, "O_NOFOLLOW", 0)) + except OSError as exc: + raise CaptureSetupError("cannot open project anchor") from exc + try: + anchor_stat = os.fstat(fd) + if not stat.S_ISDIR(anchor_stat.st_mode): + raise CaptureSetupError("project anchor is not a directory") + physical_path = Path(os.path.realpath(cwd)) + return ProjectAnchor( + fd=fd, + identity=FileIdentity.from_stat(anchor_stat), + physical_path=physical_path, + ) + except BaseException: + os.close(fd) + raise + + +def open_capture_root(anchor: ProjectAnchor, *, create: bool) -> CaptureRoot: + """Open the capture-root chain relative to ``anchor`` without following symlinks.""" + + opened: list[int] = [] + try: + autoskillit_fd = _open_directory_component( + anchor.fd, CAPTURE_PATH_COMPONENTS[0], create=create + ) + opened.append(autoskillit_fd) + temp_fd = _open_directory_component( + autoskillit_fd, CAPTURE_PATH_COMPONENTS[1], create=create + ) + opened.append(temp_fd) + capture_fd = _open_directory_component(temp_fd, CAPTURE_PATH_COMPONENTS[2], create=create) + opened.append(capture_fd) + return CaptureRoot( + autoskillit_fd=autoskillit_fd, + temp_fd=temp_fd, + fd=capture_fd, + autoskillit_identity=_identity(autoskillit_fd), + temp_identity=_identity(temp_fd), + identity=_identity(capture_fd), + ) + except BaseException: + for fd in reversed(opened): + os.close(fd) + raise + + +def create_capture_artifact(root: CaptureRoot, capture_id: str) -> CaptureArtifact: + """Create the final artifact exclusively beneath an already-open capture root.""" + + if not _CAPTURE_ID_RE.fullmatch(capture_id): + raise CaptureSetupError("invalid capture id") + name = f"shell_{capture_id}.log" + try: + fd = os.open(name, _ARTIFACT_FLAGS, mode=0o600, dir_fd=root.fd) + except OSError as exc: + raise CaptureSetupError("cannot create exclusive capture artifact") from exc + try: + value = os.fstat(fd) + if not stat.S_ISREG(value.st_mode) or value.st_nlink != 1 or value.st_mode & stat.S_IWOTH: + raise CaptureSetupError("unsafe capture artifact") + return CaptureArtifact(fd=fd, name=name, identity=FileIdentity.from_stat(value)) + except BaseException: + os.close(fd) + raise + + +def _read_bounded_file_at(directory_fd: int, name: str) -> dict: + try: + fd = os.open(name, _READ_FLAGS, dir_fd=directory_fd) + except FileNotFoundError: + return {} + except OSError: + return {} + try: + value = os.fstat(fd) + if not stat.S_ISREG(value.st_mode): + return {} + data = bytearray() + while len(data) <= _MAX_POLICY_FILE_BYTES: + chunk = os.read(fd, min(8192, _MAX_POLICY_FILE_BYTES + 1 - len(data))) + if not chunk: + break + data.extend(chunk) + if len(data) > _MAX_POLICY_FILE_BYTES: + return {} + parsed = json.loads(data) + return parsed if isinstance(parsed, dict) else {} + except (OSError, UnicodeDecodeError, json.JSONDecodeError, TypeError): + return {} + finally: + os.close(fd) + + +def _policy_inline_bytes(value: object) -> int: + if not isinstance(value, int) or isinstance(value, bool) or value <= 0: + return _DEFAULT_INLINE_BYTES + return min(value, _MAX_INLINE_BYTES) + + +def read_capture_policy(anchor: ProjectAnchor) -> CapturePolicy: + """Read output policy only through verified project/temp directory descriptors.""" + + autoskillit_fd = -1 + temp_fd = -1 + try: + autoskillit_fd = _open_directory_component( + anchor.fd, CAPTURE_PATH_COMPONENTS[0], create=False + ) + temp_fd = _open_directory_component( + autoskillit_fd, CAPTURE_PATH_COMPONENTS[1], create=False + ) + except CaptureSetupError: + return CapturePolicy() + + try: + base = _read_bounded_file_at(temp_fd, HOOK_CONFIG_FILENAME) + overlay = _read_bounded_file_at(temp_fd, HOOK_CONFIG_OVERLAY_FILENAME) + merged = merge_hook_configs(base, overlay) + section = merged.get("output_budget_policy", {}) + if not isinstance(section, dict): + section = {} + return CapturePolicy( + disabled=section.get("disabled") is True, + inline_bytes=_policy_inline_bytes(section.get("shell_max_inline_bytes")), + ) + finally: + os.close(temp_fd) + os.close(autoskillit_fd) + + +def _open_and_match_directory(parent_fd: int, name: str, expected: FileIdentity) -> int: + try: + fd = _open_directory_component(parent_fd, name, create=False) + except CaptureSetupError: + return -1 + if not _same_identity(fd, expected): + os.close(fd) + return -1 + return fd + + +def current_artifact_path_if_bound( + anchor: ProjectAnchor, + root: CaptureRoot, + artifact: CaptureArtifact, +) -> str | None: + """Return a path only if the current pathname chain still binds to all opened fds.""" + + opened: list[int] = [] + try: + try: + project_fd = os.open( + anchor.physical_path, + _DIRECTORY_FLAGS & ~getattr(os, "O_NOFOLLOW", 0), + ) + except OSError: + return None + opened.append(project_fd) + if not _same_identity(project_fd, anchor.identity): + return None + + autoskillit_fd = _open_and_match_directory( + project_fd, CAPTURE_PATH_COMPONENTS[0], root.autoskillit_identity + ) + if autoskillit_fd < 0: + return None + opened.append(autoskillit_fd) + + temp_fd = _open_and_match_directory( + autoskillit_fd, CAPTURE_PATH_COMPONENTS[1], root.temp_identity + ) + if temp_fd < 0: + return None + opened.append(temp_fd) + + capture_fd = _open_and_match_directory(temp_fd, CAPTURE_PATH_COMPONENTS[2], root.identity) + if capture_fd < 0: + return None + opened.append(capture_fd) + + try: + current_artifact_fd = os.open(artifact.name, _READ_FLAGS, dir_fd=capture_fd) + except OSError: + return None + opened.append(current_artifact_fd) + if not _same_identity(current_artifact_fd, artifact.identity): + return None + return str(anchor.physical_path.joinpath(*CAPTURE_PATH_COMPONENTS, artifact.name)) + finally: + for fd in reversed(opened): + os.close(fd) + + +def _wrap_user_command(command: str) -> str: + separator = "" if command.endswith("\n") else "\n" + return f"(\ntrap '__as_user_ec=$?; wait; exit \"$__as_user_ec\"' EXIT\n{command}{separator})" + + +def _spawn_bash( + anchor: ProjectAnchor, + bash_path: str, + command: str, + *, + capture_output: bool, +) -> subprocess.Popen[bytes]: + try: + original_cwd_fd = os.open(".", _DIRECTORY_FLAGS & ~getattr(os, "O_NOFOLLOW", 0)) + except OSError as exc: + raise CaptureSetupError("cannot preserve runner cwd") from exc + + process: subprocess.Popen[bytes] | None = None + restore_error: OSError | None = None + try: + os.fchdir(anchor.fd) + process = subprocess.Popen( + [bash_path, "-c", _wrap_user_command(command), "autoskillit-capture"], + stdout=subprocess.PIPE if capture_output else None, + stderr=subprocess.STDOUT if capture_output else None, + close_fds=True, + ) + except OSError as exc: + raise CaptureSetupError("cannot spawn capture shell") from exc + finally: + try: + os.fchdir(original_cwd_fd) + except OSError as exc: + restore_error = exc + os.close(original_cwd_fd) + + if restore_error is not None: + if process is not None: + process.terminate() + process.wait() + raise CaptureSetupError("cannot restore runner cwd") from restore_error + if process is None: + raise CaptureSetupError("capture shell did not start") + return process + + +def _write_all(fd: int, data: bytes) -> None: + view = memoryview(data) + while view: + written = os.write(fd, view) + if written <= 0: + raise OSError(errno.EIO, "capture artifact write made no progress") + view = view[written:] + + +def _drain_capture( + process: subprocess.Popen[bytes], + artifact: CaptureArtifact, + inline_bytes: int, +) -> _DrainResult: + stream = process.stdout + if stream is None: + raise CaptureSetupError("capture pipe unavailable") + + head_limit = (2 * inline_bytes) // 3 + tail_limit = inline_bytes - head_limit + total = 0 + digest = hashlib.sha256() + inline = bytearray() + head = bytearray() + tail = bytearray() + write_error: OSError | None = None + + while True: + chunk = stream.read(_DRAIN_CHUNK_BYTES) + if not chunk: + break + total += len(chunk) + digest.update(chunk) + if write_error is None: + try: + _write_all(artifact.fd, chunk) + except OSError as exc: + write_error = exc + if len(inline) <= inline_bytes: + remaining = inline_bytes + 1 - len(inline) + inline.extend(chunk[:remaining]) + if len(head) < head_limit: + head.extend(chunk[: head_limit - len(head)]) + if tail_limit: + tail.extend(chunk) + if len(tail) > tail_limit: + del tail[:-tail_limit] + + return _DrainResult( + total_bytes=total, + sha256=digest.hexdigest(), + inline=bytes(inline), + head=bytes(head), + tail=bytes(tail), + write_error=write_error, + ) + + +def _normalized_returncode(returncode: int) -> int: + return 128 + (-returncode) if returncode < 0 else returncode + + +def _capture_event(reason_code: str, decision: str) -> PolicyEvent: + return PolicyEvent( + hook_id="shell_capture_hook", + hook_version=1, + event="PreToolUse", + decision=decision, + reason_code=reason_code, + ) + + +def _emit_failure(detail: str) -> None: + prefix = render_capture_marker(_capture_event("CAPTURE_FAILED", "deny")) + safe_detail = " ".join(detail.split())[:240] + sys.stderr.write(f"{prefix} {safe_detail}]\n") + + +def _emit_capture( + result: _DrainResult, + artifact_path: str | None, + inline_bytes: int, +) -> None: + if result.total_bytes <= inline_bytes: + sys.stdout.buffer.write(result.inline) + sys.stdout.buffer.flush() + return + + prefix = render_capture_marker(_capture_event("SHELL_OUTPUT_CAPTURED", "input rewrite")) + path = artifact_path if artifact_path is not None else "unavailable" + marker = ( + f"\n{prefix} full output {result.total_bytes} bytes -> {path} " + f"sha256={result.sha256} complete=true]\n" + ).encode() + sys.stdout.buffer.write(result.head) + sys.stdout.buffer.write(marker) + sys.stdout.buffer.write(result.tail) + sys.stdout.buffer.flush() + + +def _resolve_bash() -> str: + bash_path = shutil.which("bash") + if bash_path is None: + raise CaptureSetupError("bash executable unavailable") + resolved = os.path.realpath(bash_path) + if not os.path.isabs(resolved): + raise CaptureSetupError("bash executable path is not absolute") + return resolved + + +def run_capture(command: str, cwd: str, capture_id: str) -> int: + """Run ``command`` from a descriptor-anchored project and capture its output.""" + + command_bytes = command.encode("utf-8") + if ( + not _CAPTURE_ID_RE.fullmatch(capture_id) + or "\x00" in command + or len(command_bytes) > _MAX_COMMAND_BYTES + ): + raise CaptureSetupError("invalid capture request") + + anchor = open_project_anchor(cwd) + root: CaptureRoot | None = None + artifact: CaptureArtifact | None = None + try: + policy = read_capture_policy(anchor) + bash_path = _resolve_bash() + if policy.disabled: + process = _spawn_bash(anchor, bash_path, command, capture_output=False) + return _normalized_returncode(process.wait()) + + root = open_capture_root(anchor, create=True) + artifact = create_capture_artifact(root, capture_id) + process = _spawn_bash(anchor, bash_path, command, capture_output=True) + result = _drain_capture(process, artifact, policy.inline_bytes) + returncode = _normalized_returncode(process.wait()) + if result.write_error is not None: + _emit_failure("capture artifact write failed") + return returncode + try: + artifact_path = current_artifact_path_if_bound(anchor, root, artifact) + except OSError: + _emit_failure("capture marker verification failed") + return returncode + _emit_capture(result, artifact_path, policy.inline_bytes) + return returncode + finally: + if artifact is not None: + artifact.close() + if root is not None: + root.close() + anchor.close() + + +def sweep_stale_captures( + project_root: str | Path, + *, + max_age_seconds: int = 3600, +) -> int: + """Classify stale captures without path-only deletion. + + The portable stdlib has no identity-conditioned unlink primitive. Validated + stale files are therefore retained, and the deleted count is always zero. + """ + + anchor: ProjectAnchor | None = None + root: CaptureRoot | None = None + try: + anchor = open_project_anchor(os.fspath(project_root)) + root = open_capture_root(anchor, create=False) + threshold = time.time() - max_age_seconds + try: + names = os.listdir(root.fd) + except OSError: + return 0 + for name in names: + if not _CAPTURE_FILENAME_RE.fullmatch(name): + continue + try: + fd = os.open(name, _READ_FLAGS, dir_fd=root.fd) + except OSError: + continue + try: + value = os.fstat(fd) + if ( + not stat.S_ISREG(value.st_mode) + or value.st_nlink != 1 + or value.st_mode & stat.S_IWOTH + or value.st_mtime > threshold + ): + continue + # Retention is the safe disposition without expected-inode unlink. + finally: + os.close(fd) + return 0 + except (CaptureSetupError, OSError): + return 0 + finally: + if root is not None: + root.close() + if anchor is not None: + anchor.close() + + +def _decode_command(value: str) -> str: + if len(value) > _MAX_ENCODED_COMMAND_BYTES: + raise CaptureSetupError("encoded command exceeds limit") + try: + raw = base64.b64decode(value, validate=True) + command = raw.decode("utf-8") + except (binascii.Error, UnicodeDecodeError, ValueError) as exc: + raise CaptureSetupError("invalid command transport") from exc + if len(raw) > _MAX_COMMAND_BYTES or "\x00" in command: + raise CaptureSetupError("decoded command exceeds limit") + return command + + +def _main(argv: list[str] | None = None) -> int: + args = sys.argv[1:] if argv is None else argv + if len(args) == 2 and args[0] == "reject": + _emit_failure("capture request rejected before command execution") + return 1 + if len(args) != 4 or args[0] != "run": + _emit_failure("invalid capture runner invocation") + return 1 + try: + command = _decode_command(args[1]) + return run_capture(command, args[2], args[3]) + except CaptureSetupError as exc: + _emit_failure(str(exc)) + return 1 + except (OSError, subprocess.SubprocessError): + _emit_failure("capture runner failed") + return 1 + + +if __name__ == "__main__": + sys.exit(_main()) diff --git a/tests/hooks/AGENTS.md b/tests/hooks/AGENTS.md index 19cb1a0d5..fcb860738 100644 --- a/tests/hooks/AGENTS.md +++ b/tests/hooks/AGENTS.md @@ -38,6 +38,7 @@ Hook script behavior, registration, and bridge tests. | `test_command_classification_git.py` | Tests for the git command classification primitives (is_git_command, extract_git_subcommand_and_flags) | | `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_capture_artifacts.py` | Descriptor-anchored shell-capture authority, policy, collision, binding, and cleanup tests | | `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 | diff --git a/tests/hooks/test_capture_artifacts.py b/tests/hooks/test_capture_artifacts.py new file mode 100644 index 000000000..f7fe2efe2 --- /dev/null +++ b/tests/hooks/test_capture_artifacts.py @@ -0,0 +1,266 @@ +"""Tests for descriptor-anchored shell-capture authority.""" + +from __future__ import annotations + +import json +import os +import subprocess +import time +from pathlib import Path + +import pytest + +import autoskillit.hooks._capture_artifacts as capture_artifacts +from autoskillit.hooks._capture_artifacts import ( + CAPTURE_PATH_COMPONENTS, + CapturePolicy, + CaptureSetupError, + create_capture_artifact, + current_artifact_path_if_bound, + open_capture_root, + open_project_anchor, + read_capture_policy, + run_capture, + sweep_stale_captures, +) + +pytestmark = [pytest.mark.layer("hooks"), pytest.mark.medium] + +_CAPTURE_ID = "0123456789abcdef" + + +def _capture_dir(project: Path) -> Path: + return project.joinpath(*CAPTURE_PATH_COMPONENTS) + + +def _open_authority(project: Path): + anchor = open_project_anchor(str(project)) + try: + root = open_capture_root(anchor, create=True) + except BaseException: + anchor.close() + raise + return anchor, root + + +def test_project_anchor_accepts_symlink_cwd(tmp_path: Path) -> None: + project = tmp_path / "project" + project.mkdir() + supplied_cwd = tmp_path / "project-link" + supplied_cwd.symlink_to(project, target_is_directory=True) + + anchor, root = _open_authority(supplied_cwd) + artifact = create_capture_artifact(root, _CAPTURE_ID) + try: + assert anchor.physical_path == project.resolve() + assert _capture_dir(project).is_dir() + assert current_artifact_path_if_bound(anchor, root, artifact) == str( + _capture_dir(project) / artifact.name + ) + finally: + artifact.close() + root.close() + anchor.close() + + +@pytest.mark.parametrize("component", CAPTURE_PATH_COMPONENTS) +def test_capture_root_rejects_symlinked_components(component: str, tmp_path: Path) -> None: + project = tmp_path / "project" + project.mkdir() + external = tmp_path / "external" + external.mkdir() + + parent = project + for name in CAPTURE_PATH_COMPONENTS: + candidate = parent / name + if name == component: + candidate.symlink_to(external, target_is_directory=True) + break + candidate.mkdir() + parent = candidate + + anchor = open_project_anchor(str(project)) + try: + with pytest.raises(CaptureSetupError): + open_capture_root(anchor, create=True) + finally: + anchor.close() + + +def test_symlinked_policy_root_is_not_trusted(tmp_path: Path) -> None: + project = tmp_path / "project" + project.mkdir() + autoskillit_dir = project / CAPTURE_PATH_COMPONENTS[0] + autoskillit_dir.mkdir() + external_temp = tmp_path / "external-temp" + external_temp.mkdir() + (external_temp / ".hook_config.json").write_text( + json.dumps({"output_budget_policy": {"disabled": True}}) + ) + (autoskillit_dir / CAPTURE_PATH_COMPONENTS[1]).symlink_to( + external_temp, target_is_directory=True + ) + + anchor = open_project_anchor(str(project)) + try: + assert read_capture_policy(anchor) == CapturePolicy() + with pytest.raises(CaptureSetupError): + open_capture_root(anchor, create=True) + finally: + anchor.close() + + +def test_verified_policy_merges_overlay_and_bounds_inline_bytes(tmp_path: Path) -> None: + project = tmp_path / "project" + temp_dir = project.joinpath(*CAPTURE_PATH_COMPONENTS[:2]) + temp_dir.mkdir(parents=True) + (temp_dir / ".hook_config.json").write_text( + json.dumps( + { + "output_budget_policy": { + "disabled": False, + "shell_max_inline_bytes": 31, + } + } + ) + ) + (temp_dir / ".hook_config_overlay.json").write_text( + json.dumps({"output_budget_policy": {"disabled": True}}) + ) + + anchor = open_project_anchor(str(project)) + try: + assert read_capture_policy(anchor) == CapturePolicy(disabled=True, inline_bytes=31) + finally: + anchor.close() + + +@pytest.mark.parametrize("collision", ["symlink", "hardlink", "regular"]) +def test_artifact_creation_rejects_existing_entries(collision: str, tmp_path: Path) -> None: + project = tmp_path / "project" + project.mkdir() + anchor, root = _open_authority(project) + artifact_path = _capture_dir(project) / f"shell_{_CAPTURE_ID}.log" + external = tmp_path / "external-secret" + external.write_bytes(b"must-survive") + try: + if collision == "symlink": + artifact_path.symlink_to(external) + elif collision == "hardlink": + try: + os.link(external, artifact_path) + except OSError: + pytest.skip("hardlinks unavailable") + else: + artifact_path.write_bytes(b"existing") + + with pytest.raises(CaptureSetupError): + create_capture_artifact(root, _CAPTURE_ID) + + assert external.read_bytes() == b"must-survive" + if collision == "regular": + assert artifact_path.read_bytes() == b"existing" + finally: + root.close() + anchor.close() + + +def test_marker_path_requires_current_directory_binding(tmp_path: Path) -> None: + project = tmp_path / "project" + project.mkdir() + anchor, root = _open_authority(project) + artifact = create_capture_artifact(root, _CAPTURE_ID) + capture_dir = _capture_dir(project) + displaced = capture_dir.with_name("shell_capture-displaced") + try: + capture_dir.rename(displaced) + capture_dir.mkdir() + assert current_artifact_path_if_bound(anchor, root, artifact) is None + finally: + artifact.close() + root.close() + anchor.close() + + +def test_stale_capture_is_safely_retained_without_identity_unlink(tmp_path: Path) -> None: + project = tmp_path / "project" + capture_dir = _capture_dir(project) + capture_dir.mkdir(parents=True) + stale = capture_dir / f"shell_{_CAPTURE_ID}.log" + stale.write_bytes(b"retained") + old = time.time() - 7200 + os.utime(stale, (old, old)) + + assert sweep_stale_captures(project, max_age_seconds=3600) == 0 + assert stale.read_bytes() == b"retained" + + +def test_cleanup_rejects_symlinked_capture_root(tmp_path: Path) -> None: + project = tmp_path / "project" + temp_dir = project.joinpath(*CAPTURE_PATH_COMPONENTS[:2]) + temp_dir.mkdir(parents=True) + external = tmp_path / "external" + external.mkdir() + stale = external / f"shell_{_CAPTURE_ID}.log" + stale.write_bytes(b"must-survive") + old = time.time() - 7200 + os.utime(stale, (old, old)) + (temp_dir / CAPTURE_PATH_COMPONENTS[2]).symlink_to(external, target_is_directory=True) + + assert sweep_stale_captures(project, max_age_seconds=0) == 0 + assert stale.read_bytes() == b"must-survive" + + +def test_setup_failure_prevents_user_command(tmp_path: Path) -> None: + project = tmp_path / "project" + temp_dir = project.joinpath(*CAPTURE_PATH_COMPONENTS[:2]) + temp_dir.mkdir(parents=True) + (temp_dir / CAPTURE_PATH_COMPONENTS[2]).write_text("blocking file") + + with pytest.raises(CaptureSetupError): + run_capture("printf ran > command_ran", str(project), _CAPTURE_ID) + assert not (project / "command_ran").exists() + + +def test_verified_disabled_policy_runs_without_capture( + tmp_path: Path, capfd: pytest.CaptureFixture[str] +) -> None: + project = tmp_path / "project" + temp_dir = project.joinpath(*CAPTURE_PATH_COMPONENTS[:2]) + temp_dir.mkdir(parents=True) + (temp_dir / ".hook_config.json").write_text( + json.dumps({"output_budget_policy": {"disabled": True}}) + ) + + assert run_capture("printf policy-disabled", str(project), _CAPTURE_ID) == 0 + captured = capfd.readouterr() + assert captured.out == "policy-disabled" + assert not _capture_dir(project).exists() + + +def test_spawn_failure_closes_created_artifact_fd( + tmp_path: Path, monkeypatch: pytest.MonkeyPatch +) -> None: + project = tmp_path / "project" + project.mkdir() + observed_fds: list[int] = [] + real_create = capture_artifacts.create_capture_artifact + + def record_artifact(root, capture_id): + artifact = real_create(root, capture_id) + observed_fds.append(artifact.fd) + return artifact + + def fail_spawn(*args, **kwargs): + raise OSError("fault injection") + + monkeypatch.setattr(capture_artifacts, "create_capture_artifact", record_artifact) + monkeypatch.setattr(subprocess, "Popen", fail_spawn) + + with pytest.raises(CaptureSetupError): + run_capture("printf never", str(project), _CAPTURE_ID) + + assert observed_fds + for fd in observed_fds: + with pytest.raises(OSError): + os.fstat(fd) From 94ea1c553ff25059395fa16f07fd0f47ae216f83 Mon Sep 17 00:00:00 2001 From: Trecek Date: Wed, 22 Jul 2026 20:37:09 -0700 Subject: [PATCH 02/32] fix: anchor shell capture to verified descriptors --- docs/decisions/0005-output-budget-protocol.md | 6 +- docs/decisions/0006-output-containment.md | 27 +- docs/safety/hooks.md | 32 +- src/autoskillit/core/path_containment.py | 10 +- src/autoskillit/hooks/AGENTS.md | 3 +- src/autoskillit/hooks/_capture_artifacts.py | 98 +++-- src/autoskillit/hooks/_capture_cleanup.py | 101 ------ src/autoskillit/hooks/_hook_settings.py | 2 +- src/autoskillit/hooks/_policy_event.py | 5 +- src/autoskillit/hooks/session_start_hook.py | 25 +- src/autoskillit/hooks/shell_capture_hook.py | 129 ++----- tests/arch/AGENTS.md | 1 + tests/arch/test_python_no_hardcoded_temp.py | 1 - tests/arch/test_shell_capture_trust_anchor.py | 79 ++++ tests/arch/test_subpackage_isolation.py | 2 +- tests/core/test_path_containment.py | 10 + .../backends/test_cli_conformance_probes.py | 80 +++- tests/hooks/test_capture_artifacts.py | 152 ++++++++ tests/hooks/test_session_start_hook.py | 51 ++- tests/hooks/test_shell_capture_conformance.py | 341 ++++++++++++------ tests/hooks/test_shell_capture_hook.py | 112 ++++-- 21 files changed, 846 insertions(+), 421 deletions(-) delete mode 100644 src/autoskillit/hooks/_capture_cleanup.py create mode 100644 tests/arch/test_shell_capture_trust_anchor.py diff --git a/docs/decisions/0005-output-budget-protocol.md b/docs/decisions/0005-output-budget-protocol.md index 4ddaed9ef..2ebb9118f 100644 --- a/docs/decisions/0005-output-budget-protocol.md +++ b/docs/decisions/0005-output-budget-protocol.md @@ -69,8 +69,10 @@ the selected decision without applying a second static shaping pass. 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. +retained); larger outputs are captured losslessly to the same descriptor-owned artifact +with a bounded head/tail slice and provenance marker inlined. SessionStart classifies +stale artifacts but conservatively retains them on portable Python because pathname +unlink cannot condition deletion on the validated inode. `small_file_max_bytes` was removed — it existed solely for the classifier's literal-small-JSONL exception, which is no longer needed. diff --git a/docs/decisions/0006-output-containment.md b/docs/decisions/0006-output-containment.md index 8331c5590..003803057 100644 --- a/docs/decisions/0006-output-containment.md +++ b/docs/decisions/0006-output-containment.md @@ -26,8 +26,11 @@ output-boundary bounding on measured bytes: 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 + command in a minimal isolated runner invocation. The runner opens `cwd` first, + establishes descriptor-relative authority for policy and capture components, + creates the artifact exclusively without following symlinks, and drains child + output through its owned fd. The bounded inline slice includes a provenance + marker whose path is present only after marker-time identity verification. The ordinary outer-result limit remains the backstop for hook-failure paths. The separately configured `CODEX_HISTORY_RETENTION_TOKEN_LIMIT` governs later history. @@ -66,10 +69,9 @@ hook can be retired in favor of that mechanism. 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. +2. Fatal self-signals (e.g. `kill -TERM $$`) are reported as the shell-compatible + `128 + signal` status by the runner. Output drained before termination remains + available inline and in the retained artifact. SIGKILL remains untrappable. 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 @@ -79,6 +81,16 @@ hook can be retired in favor of that mechanism. 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. +7. A supplied symlink spelling of `cwd` is accepted only by opening it first as + the `ProjectAnchor`; `.autoskillit`, `temp`, and `shell_capture` symlinks are + rejected. Physical path strings are display hints, not filesystem authority. +8. Same-user code inside the command can rename a verified directory entry after + it is opened. Output, hashing, and replay remain fd-bound; if the live pathname + chain no longer matches, the marker reports its path as `unavailable`. +9. Portable Linux/macOS Python exposes descriptor-relative unlink but no + expected-inode conditional unlink. SessionStart therefore retains stale + candidates rather than making a security claim across a validation/deletion + race. Artifact quota and lifecycle reclamation remain follow-up work. ## Consequences @@ -89,4 +101,5 @@ hook can be retired in favor of that mechanism. 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`. + the descriptor-anchored project capture root. A pathname is reported only + while it still binds to the opened project, directories, and artifact. diff --git a/docs/safety/hooks.md b/docs/safety/hooks.md index 81f5b3561..5f0ce6f6f 100644 --- a/docs/safety/hooks.md +++ b/docs/safety/hooks.md @@ -88,25 +88,33 @@ in `.hook_config.json` for future recipes that legitimately need these operation **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 retained; reclaimed by SessionStart sweep), 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`. +minimal isolated-Python runner. The runner opens the supplied `cwd` as a +`ProjectAnchor` directory descriptor before deriving a physical path, then opens or +creates `.autoskillit`, `temp`, and `shell_capture` relative to retained directory +descriptors without following symlinks. It reads output policy through the verified +`temp` descriptor and creates `shell_.log` exclusively with no-follow +semantics. The child sends combined stdout+stderr through a pipe; the runner writes, +measures, hashes, and replays bytes through owned descriptors. Only a bounded inline +slice enters context: full content when small, otherwise head + provenance marker +(bytes, sha256, verified path or `unavailable`) + tail. Set +`output_budget.guard_enabled: false` to disable capture after verified policy loading; +the inline threshold is controlled by `output_budget.shell_max_inline_bytes`. #### Capture Artifact Lifecycle | Phase | Behavior | |-------|----------| -| Creation | Harness creates `/.autoskillit/temp/shell_capture/shell_.log` via `mkdir -p` + redirect | +| Creation | Runner creates the artifact relative to a verified capture-directory fd with `O_CREAT | O_EXCL | O_NOFOLLOW` and mode `0600` | | Retention | Artifact is always retained after command execution (both inline and spill branches) | -| Ownership | The hook subprocess; no downstream consumer reads these files | -| Cleanup — session lifecycle | `session_start_hook.py` sweeps stale captures (older than 1 hour, measured against wall-clock time) at every SessionStart | +| Ownership | The isolated runner retains all directory/artifact fds through measurement, replay, and marker-path identity verification | +| Cleanup — session lifecycle | `session_start_hook.py` calls the same descriptor-anchored helper to classify stale captures. Portable Python cannot unlink by expected inode, so candidates are conservatively retained rather than deleted through a race-prone pathname | | Naming contract | `shell_[0-9a-f]{16}.log` — files not matching this pattern are never deleted | -| Safety | Sweep validates the filename allowlist and rejects symlinks via `lstat`-backed checks | -| Failure mode | Sweep errors are swallowed (fail-open); cleanup failure never blocks command execution | +| Safety | Capture components reject symlinks; artifacts reject collisions, symlinks, hardlinks, and world-writable entries; marker paths are emitted only while every current binding matches the opened identities | +| Failure mode | Capture setup failure stops before the user command. SessionStart cleanup errors remain fail-open and never block reminder injection | + +Codex hook generation excludes the interactive-only SessionStart hook, so Codex +artifacts have no automatic deletion guarantee. Lifecycle/quota reclamation must not +weaken the descriptor and identity contract when it is added. ### `generated_file_write_guard.py` **Guarded tools:** `Write`, `Edit` diff --git a/src/autoskillit/core/path_containment.py b/src/autoskillit/core/path_containment.py index 8ff66fdd9..01d5009dc 100644 --- a/src/autoskillit/core/path_containment.py +++ b/src/autoskillit/core/path_containment.py @@ -1,8 +1,10 @@ """Path containment guards for closure-mode artifact validation (IL-0, stdlib-only). -Resolves a path against an allowed root, rejecting symlinks, hardlinks, -traversal escapes, oversized files, and world-writable files. Also provides -a metadata-stability check for TOCTOU detection between pre- and post-read stats. +Resolves a child path against an already-trusted allowed root, rejecting child +symlinks, hardlinks, traversal escapes, oversized files, and world-writable +files. Root authority is a caller precondition; this module does not establish +whether ``allowed_root`` itself or its ancestors are hostile. Also provides a +metadata-stability check for TOCTOU detection between pre- and post-read stats. """ from __future__ import annotations @@ -24,6 +26,8 @@ def resolve_contained_path( *, max_size_bytes: int = 50_000_000, ) -> Path: + """Validate a child path beneath an allowed root whose authority is pre-established.""" + original = Path(path) orig_st = original.lstat() if stat.S_ISLNK(orig_st.st_mode): diff --git a/src/autoskillit/hooks/AGENTS.md b/src/autoskillit/hooks/AGENTS.md index d0d7d1a25..de741db55 100644 --- a/src/autoskillit/hooks/AGENTS.md +++ b/src/autoskillit/hooks/AGENTS.md @@ -24,8 +24,7 @@ Sub-packages: guards/ (see guards/AGENTS.md), formatters/ (see formatters/AGENTS | `_hook_utils.py` | Shared stdlib-only utilities for hook scripts (e.g., `find_project_root`, `STEP_SUFFIX_RE`) | | `_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) | -| `_capture_artifacts.py` | Stdlib-only descriptor-anchored shell-capture authority, runner, and cleanup classifier | -| `_capture_cleanup.py` | Stdlib-only stale-capture-file sweep helpers (used by `session_start_hook.py`) | +| `_capture_artifacts.py` | Stdlib-only descriptor-anchored shell-capture authority, runner, and cleanup classifier used by `session_start_hook.py` | | `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/_capture_artifacts.py b/src/autoskillit/hooks/_capture_artifacts.py index c2dae6f6e..bbae67b79 100644 --- a/src/autoskillit/hooks/_capture_artifacts.py +++ b/src/autoskillit/hooks/_capture_artifacts.py @@ -153,6 +153,22 @@ class _DrainResult: write_error: OSError | None +@dataclass(slots=True) +class _CleanupCandidate: + name: str + fd: int + device: int + inode: int + mode: int + nlink: int + mtime: float + + def close(self) -> None: + if self.fd >= 0: + os.close(self.fd) + self.fd = -1 + + def _require_capabilities() -> None: required_dir_fd = (os.open, os.mkdir) if ( @@ -283,7 +299,7 @@ def _read_bounded_file_at(directory_fd: int, name: str) -> dict: return {} try: value = os.fstat(fd) - if not stat.S_ISREG(value.st_mode): + if not stat.S_ISREG(value.st_mode) or value.st_nlink != 1 or value.st_mode & stat.S_IWOTH: return {} data = bytearray() while len(data) <= _MAX_POLICY_FILE_BYTES: @@ -393,7 +409,13 @@ def current_artifact_path_if_bound( except OSError: return None opened.append(current_artifact_fd) - if not _same_identity(current_artifact_fd, artifact.identity): + current_value = os.fstat(current_artifact_fd) + if ( + FileIdentity.from_stat(current_value) != artifact.identity + or not stat.S_ISREG(current_value.st_mode) + or current_value.st_nlink != 1 + or current_value.st_mode & stat.S_IWOTH + ): return None return str(anchor.physical_path.joinpath(*CAPTURE_PATH_COMPONENTS, artifact.name)) finally: @@ -560,7 +582,10 @@ def _resolve_bash() -> str: def run_capture(command: str, cwd: str, capture_id: str) -> int: """Run ``command`` from a descriptor-anchored project and capture its output.""" - command_bytes = command.encode("utf-8") + try: + command_bytes = command.encode("utf-8") + except UnicodeEncodeError as exc: + raise CaptureSetupError("invalid command encoding") from exc if ( not _CAPTURE_ID_RE.fullmatch(capture_id) or "\x00" in command @@ -581,7 +606,16 @@ def run_capture(command: str, cwd: str, capture_id: str) -> int: root = open_capture_root(anchor, create=True) artifact = create_capture_artifact(root, capture_id) process = _spawn_bash(anchor, bash_path, command, capture_output=True) - result = _drain_capture(process, artifact, policy.inline_bytes) + try: + result = _drain_capture(process, artifact, policy.inline_bytes) + except (CaptureSetupError, OSError): + if process.stdout is not None: + process.stdout.close() + if process.poll() is None: + process.terminate() + returncode = _normalized_returncode(process.wait()) + _emit_failure("capture pipe drain failed") + return returncode returncode = _normalized_returncode(process.wait()) if result.write_error is not None: _emit_failure("capture artifact write failed") @@ -601,6 +635,42 @@ def run_capture(command: str, cwd: str, capture_id: str) -> int: anchor.close() +def _open_stale_candidate( + root: CaptureRoot, + name: str, + *, + mtime_threshold: float, +) -> _CleanupCandidate | None: + if not _CAPTURE_FILENAME_RE.fullmatch(name): + return None + try: + fd = os.open(name, _READ_FLAGS, dir_fd=root.fd) + except OSError: + return None + try: + value = os.fstat(fd) + except OSError: + os.close(fd) + return None + if ( + not stat.S_ISREG(value.st_mode) + or value.st_nlink != 1 + or value.st_mode & stat.S_IWOTH + or value.st_mtime > mtime_threshold + ): + os.close(fd) + return None + return _CleanupCandidate( + name=name, + fd=fd, + device=value.st_dev, + inode=value.st_ino, + mode=value.st_mode, + nlink=value.st_nlink, + mtime=value.st_mtime, + ) + + def sweep_stale_captures( project_root: str | Path, *, @@ -623,24 +693,10 @@ def sweep_stale_captures( except OSError: return 0 for name in names: - if not _CAPTURE_FILENAME_RE.fullmatch(name): - continue - try: - fd = os.open(name, _READ_FLAGS, dir_fd=root.fd) - except OSError: - continue - try: - value = os.fstat(fd) - if ( - not stat.S_ISREG(value.st_mode) - or value.st_nlink != 1 - or value.st_mode & stat.S_IWOTH - or value.st_mtime > threshold - ): - continue + candidate = _open_stale_candidate(root, name, mtime_threshold=threshold) + if candidate is not None: # Retention is the safe disposition without expected-inode unlink. - finally: - os.close(fd) + candidate.close() return 0 except (CaptureSetupError, OSError): return 0 diff --git a/src/autoskillit/hooks/_capture_cleanup.py b/src/autoskillit/hooks/_capture_cleanup.py deleted file mode 100644 index d54eb5d26..000000000 --- a/src/autoskillit/hooks/_capture_cleanup.py +++ /dev/null @@ -1,101 +0,0 @@ -"""Stdlib-only stale-capture-file sweep helpers (used by ``session_start_hook.py``). - -Reaps capture artifacts written by ``shell_capture_hook.py`` after their -capture-session lifetime has elapsed. Mirrors the TTL-sweep pattern in -``session_start_hook.py`` (kitchen_state, pipeline_tracker) with containment -checks inlined from ``core.path_containment.resolve_contained_path`` — -the stdlib-only hook import boundary prevents importing from ``autoskillit.*``. - -The 1-hour age threshold (``max_age_seconds=3600`` default) is the liveness -guard: a capture file currently being written by an in-flight harness has an -``mtime`` within seconds of now and will never be swept. POSIX ``unlink`` on -an open fd is safe (the inode persists until fd close), so even in the -pathological case of a sweep racing with an active reader, no data -corruption occurs — only premature path removal, which the age threshold -makes effectively impossible. -""" - -from __future__ import annotations - -import os -import re -import stat -import time -from pathlib import Path - -__all__ = ["_CAPTURE_FILENAME_RE", "_is_safe_capture_file", "sweep_stale_captures"] - -# Strict allowlist matching the 16-hex-char uid produced by -# ``shell_capture_hook._build_harness`` (``uuid4().hex[:16]``). -_CAPTURE_FILENAME_RE = re.compile(r"^shell_[0-9a-f]{16}\.log$") - - -def _is_safe_capture_file(path: Path, capture_dir: Path) -> bool: - """Return True iff ``path`` is a safe, in-place, regular capture file. - - Rejects: - - Names that don't match the strict ``shell_<16hex>.log`` pattern - - Symlinks (``stat.S_ISLNK``) - - Paths whose resolved target escapes ``capture_dir`` - - Hardlinks (``st_nlink > 1``) - - World-writable files (``st_mode & 0o002``) - """ - if not _CAPTURE_FILENAME_RE.match(path.name): - return False - try: - st = path.lstat() - except OSError: - return False - if stat.S_ISLNK(st.st_mode): - return False - if st.st_nlink > 1: - return False - if st.st_mode & 0o002: - return False - try: - capture_dir_resolved = Path(capture_dir).resolve(strict=True) - resolved = path.resolve(strict=True) - except (OSError, RuntimeError): - return False - if not resolved.is_relative_to(capture_dir_resolved): - return False - return True - - -def sweep_stale_captures( - capture_dir: Path | str, - *, - max_age_seconds: int = 3600, -) -> int: - """Delete capture files in ``capture_dir`` older than ``max_age_seconds``. - - Returns the count of deleted files. Fail-open: any per-file exception - is swallowed and the iteration continues, mirroring ``session_start_hook.py`` - TTL-sweep behavior. - """ - capture_dir_path = Path(capture_dir) - if not capture_dir_path.is_dir(): - return 0 - try: - capture_dir_path.resolve(strict=True) - except OSError: - return 0 - mtime_threshold = time.time() - max_age_seconds - - deleted = 0 - try: - entries = list(capture_dir_path.iterdir()) - except OSError: - return 0 - for entry in entries: - try: - if not _is_safe_capture_file(entry, capture_dir_path): - continue - st = entry.stat() - if st.st_mtime > mtime_threshold: - continue - os.unlink(entry) - deleted += 1 - except (FileNotFoundError, OSError): - continue - return deleted diff --git a/src/autoskillit/hooks/_hook_settings.py b/src/autoskillit/hooks/_hook_settings.py index 48f690539..9ebdeeca3 100644 --- a/src/autoskillit/hooks/_hook_settings.py +++ b/src/autoskillit/hooks/_hook_settings.py @@ -45,7 +45,7 @@ {"cache_path", "cache_max_age", "buffer_seconds", "disabled"} ) -# The exact keys the shell capture hook reads from +# The exact keys the descriptor-anchored shell capture runner 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( diff --git a/src/autoskillit/hooks/_policy_event.py b/src/autoskillit/hooks/_policy_event.py index 0cec5996b..44c599548 100644 --- a/src/autoskillit/hooks/_policy_event.py +++ b/src/autoskillit/hooks/_policy_event.py @@ -33,11 +33,10 @@ def render_provenance_prefix(event: PolicyEvent) -> str: def render_capture_marker(event: PolicyEvent) -> str: - """Render a capture marker prefix safe for embedding in double-quoted shell strings. + """Render a capture marker prefix safe for direct bounded-output emission. The returned string is guaranteed free of ``"``, backticks, and ``$`` - (it is embedded inside a double-quoted ``printf`` argument in the - shell capture harness). + so callers may also transport it through a shell boundary if needed. """ return ( f"[AutoSkillit hook {event.hook_id} v{event.hook_version}" diff --git a/src/autoskillit/hooks/session_start_hook.py b/src/autoskillit/hooks/session_start_hook.py index 1c5355722..ec3df16ae 100644 --- a/src/autoskillit/hooks/session_start_hook.py +++ b/src/autoskillit/hooks/session_start_hook.py @@ -12,11 +12,18 @@ import json import os -import re import sys from datetime import UTC, datetime from pathlib import Path +_HOOKS_DIR = str(Path(__file__).resolve().parent) +if _HOOKS_DIR not in sys.path: + sys.path.insert(0, _HOOKS_DIR) + +from _capture_artifacts import ( # type: ignore[import-not-found] # noqa: E402 + sweep_stale_captures, +) + def main() -> None: if os.environ.get("AUTOSKILLIT_HEADLESS") == "1": @@ -81,21 +88,7 @@ def main() -> None: pass try: - _capture_dir = Path.cwd() / ".autoskillit" / "temp" / "shell_capture" - if _capture_dir.is_dir(): - _CAPTURE_RE = re.compile(r"^shell_[0-9a-f]{16}\.log$") - _now = datetime.now(UTC) - for _cp in _capture_dir.iterdir(): - try: - if not _CAPTURE_RE.match(_cp.name): - continue - if _cp.is_symlink(): - continue - _c_age = _now.timestamp() - _cp.stat().st_mtime - if _c_age >= 3600: - _cp.unlink() - except Exception: - pass + sweep_stale_captures(Path.cwd()) except Exception: pass diff --git a/src/autoskillit/hooks/shell_capture_hook.py b/src/autoskillit/hooks/shell_capture_hook.py index 5c51ea7db..cb766c497 100644 --- a/src/autoskillit/hooks/shell_capture_hook.py +++ b/src/autoskillit/hooks/shell_capture_hook.py @@ -1,9 +1,9 @@ #!/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. +Rewrites every native shell command on Codex into a minimal isolated-Python +runner invocation. The runner establishes the project/capture trust anchor, +executes the command, and emits only a bounded inline slice. Codex-only (#4286 / ADR-0006). Claude Code sessions are unaffected. stdlib-only; no autoskillit imports. @@ -11,113 +11,48 @@ from __future__ import annotations +import base64 import json import os +import re 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" +_CAPTURE_ID_RE = re.compile(r"^[0-9a-f]{16}$") +_MAX_COMMAND_BYTES = 64 * 1024 +_RUNNER_PATH = Path(__file__).resolve().with_name("_capture_artifacts.py") 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 _build_harness(command: str, cwd: str, capture_id: str) -> str: + """Build a shell-safe isolated runner invocation without embedding ``command``.""" - -def _read_policy(cwd: str) -> tuple[bool, int]: + if not _CAPTURE_ID_RE.fullmatch(capture_id): + raise ValueError("capture_id must contain exactly 16 lowercase hex characters") 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[:16] - 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" -else -{sha_line} - head -c {head} "$__as_f" -{marker_line} - tail -c {tail} "$__as_f" -fi -exit $__as_ec -""" + command_bytes = command.encode("utf-8") + except UnicodeEncodeError: + command_bytes = b"" + if "\x00" in command or not command_bytes or len(command_bytes) > _MAX_COMMAND_BYTES: + argv = [sys.executable, "-I", str(_RUNNER_PATH), "reject", capture_id] + else: + encoded = base64.b64encode(command_bytes).decode("ascii") + argv = [ + sys.executable, + "-I", + str(_RUNNER_PATH), + "run", + encoded, + cwd, + capture_id, + ] + return f"{_HARNESS_SENTINEL}\n{shlex.join(argv)}" def main() -> None: @@ -136,16 +71,12 @@ def main() -> None: 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) + harness = _build_harness(command, cwd, uuid4().hex[:16]) payload = json.dumps( { "hookSpecificOutput": { diff --git a/tests/arch/AGENTS.md b/tests/arch/AGENTS.md index aa21ceb77..52d21c2c2 100644 --- a/tests/arch/AGENTS.md +++ b/tests/arch/AGENTS.md @@ -78,6 +78,7 @@ AST enforcement, sub-package layer contracts, and architectural invariant tests. | `test_pty_coherence.py` | Dispatch-type-aware PTY allocation guards: AST enforcement that dispatch_food_truck passes pty_override=False and _attempt_contract_nudge accepts pty_override | | `test_pyright_suppression_allowlist.py` | REQ-PYRIGHT-001: pyright/type-ignore suppression allowlist + count budget | | `test_python_no_hardcoded_temp.py` | Architectural invariant: no literal `.autoskillit/temp` outside the whitelist | +| `test_shell_capture_trust_anchor.py` | Guards canonical descriptor-anchored capture/cleanup wiring and explicit project-temp cleanup debt | | `test_quota_capability_isolation.py` | AST guard: quota modules must not reference BackendCapabilities fields | | `test_recipe_diagram_freshness.py` | Parametrized diagram freshness enforcement: bundled recipes must have non-stale diagrams; missing diagrams are xfail(strict=True) with shrink-enforcement meta-test | | `test_recipe_contract_freshness.py` | Parametrized JSON card freshness enforcement: contract cards must have non-stale .json companions with content parity | diff --git a/tests/arch/test_python_no_hardcoded_temp.py b/tests/arch/test_python_no_hardcoded_temp.py index d31bc1ab3..247373bda 100644 --- a/tests/arch/test_python_no_hardcoded_temp.py +++ b/tests/arch/test_python_no_hardcoded_temp.py @@ -67,7 +67,6 @@ "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/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_shell_capture_trust_anchor.py b/tests/arch/test_shell_capture_trust_anchor.py new file mode 100644 index 000000000..a96d24d4d --- /dev/null +++ b/tests/arch/test_shell_capture_trust_anchor.py @@ -0,0 +1,79 @@ +"""Architecture guards for descriptor-anchored shell capture.""" + +from __future__ import annotations + +from pathlib import Path + +import pytest + +pytestmark = [pytest.mark.layer("arch"), pytest.mark.small] + +_REPO_ROOT = Path(__file__).resolve().parents[2] +_SRC = _REPO_ROOT / "src" / "autoskillit" + +_PROJECT_TEMP_CLEANUP_DEBT = { + "core/io.py": { + "owner": "core IO", + "reason": "atomic temp writes still use mkdir/mkstemp/os.replace by pathname", + "tracking_issue": "#4319", + }, + "workspace/worktree.py": { + "owner": "workspace worktree lifecycle", + "reason": "worktree sidecars are still removed with pathname-based rmtree", + "tracking_issue": "#4319", + }, + "workspace/clone_registry.py": { + "owner": "workspace clone registry", + "reason": "registry-provided clone paths still reach removal callbacks", + "tracking_issue": "#4319", + }, + "workspace/clone.py": { + "owner": "workspace clone lifecycle", + "reason": "clone cleanup still uses pathname-based unlink/rmtree", + "tracking_issue": "#4319", + }, +} + + +def _source(relative: str) -> str: + return (_SRC / relative).read_text(encoding="utf-8") + + +def test_session_start_calls_canonical_capture_cleanup() -> None: + source = _source("hooks/session_start_hook.py") + assert "from _capture_artifacts import" in source + assert "sweep_stale_captures(Path.cwd())" in source + assert "shell_capture" not in source + + +def test_shell_capture_code_has_no_pathname_harness_or_cleanup() -> None: + sources = { + relative: _source(relative) + for relative in ( + "hooks/shell_capture_hook.py", + "hooks/_capture_artifacts.py", + ) + } + combined = "\n".join(sources.values()) + for forbidden in ( + "mkdir -p", + '> "$__as_f"', + ".unlink(", + "os.unlink(", + "shutil.rmtree(", + ): + assert forbidden not in combined, f"pathname capture operation reintroduced: {forbidden}" + + +def test_project_temp_cleanup_debt_is_narrow_and_owned() -> None: + assert set(_PROJECT_TEMP_CLEANUP_DEBT) == { + "core/io.py", + "workspace/worktree.py", + "workspace/clone_registry.py", + "workspace/clone.py", + } + for relative, debt in _PROJECT_TEMP_CLEANUP_DEBT.items(): + assert (_SRC / relative).is_file() + assert debt["owner"] + assert debt["reason"] + assert debt["tracking_issue"] == "#4319" diff --git a/tests/arch/test_subpackage_isolation.py b/tests/arch/test_subpackage_isolation.py index d8f4782ee..6f7aeac97 100644 --- a/tests/arch/test_subpackage_isolation.py +++ b/tests/arch/test_subpackage_isolation.py @@ -890,7 +890,7 @@ def test_no_subpackage_exceeds_10_files() -> None: "core": 25, # +_delivery_bounds (resolve_general_output_token_limit) "core/types": 33, # +recipe_delivery typed budget/provenance contracts "cli": 21, - "hooks": 18, # +recipe_confirmed_post_hook, +quota_guard_state_post_hook, +_policy_event, +shell_capture_hook (#4286), +_capture_cleanup.py # noqa: E501 + "hooks": 18, # +recipe_confirmed_post_hook, +quota_guard_state_post_hook, +_policy_event, +shell_capture_hook (#4286), +_capture_artifacts.py # noqa: E501 "pipeline": 12, "fleet": 23, # +_issue_url_helpers.py # noqa: E501 "recipe/rules": 55, # +commit_guard_regression_route +rules_model +rules_gitignored_deliverable +rules_issue_scope_threading +rules_inventory_gate_bilateral +rules_verdict_context +rules_contract_recovery # noqa: E501 diff --git a/tests/core/test_path_containment.py b/tests/core/test_path_containment.py index dadcc5456..4cb37a404 100644 --- a/tests/core/test_path_containment.py +++ b/tests/core/test_path_containment.py @@ -75,6 +75,16 @@ def test_blocks_hardlink(self, tmp_path) -> None: with pytest.raises(ContainmentError, match="[Hh]ardlink"): resolve_contained_path(hardlinked, allowed) + def test_symlinked_allowed_root_is_a_caller_authority_precondition(self, tmp_path) -> None: + trusted_target = tmp_path / "trusted-target" + trusted_target.mkdir() + child = trusted_target / "child.txt" + child.write_text("data") + allowed_spelling = tmp_path / "allowed-root" + allowed_spelling.symlink_to(trusted_target, target_is_directory=True) + + assert resolve_contained_path(allowed_spelling / child.name, allowed_spelling) == child + class TestCheckMetadataStable: def test_accepts_unchanged(self, tmp_path) -> None: diff --git a/tests/execution/backends/test_cli_conformance_probes.py b/tests/execution/backends/test_cli_conformance_probes.py index 9987fdfd8..a52d602e1 100644 --- a/tests/execution/backends/test_cli_conformance_probes.py +++ b/tests/execution/backends/test_cli_conformance_probes.py @@ -8,9 +8,12 @@ from __future__ import annotations +import base64 +import binascii import json import os import re +import shlex import shutil import subprocess import tomllib @@ -328,8 +331,7 @@ def _assert(output: _CodexProbeOutput) -> None: _OUTPUT_BUDGET_CANARY_PROMPT = ( "This is a hook conformance probe. Use the shell tool exactly once to run " f"`{_OUTPUT_BUDGET_CANARY_COMMAND}` without changing or bounding it. " - "After the tool is denied, repeat the complete denial reason verbatim and stop. " - "Do not run any other tool." + "After the tool completes, stop without running any other tool." ) _POLICY_DENIAL_MARKERS = ( "blocked by policy", @@ -337,7 +339,7 @@ def _assert(output: _CodexProbeOutput) -> None: "policy_violation", "rejected:", ) -_CAPTURE_ARTIFACT_PATH_RE = re.compile(r"\.autoskillit/temp/shell_capture/shell_[0-9a-f]{16}\.log") +_CAPTURE_ARTIFACT_NAME_RE = re.compile(r"^shell_([0-9a-f]{16})\.log$") _skip_unless_codex_output_budget_smoke = pytest.mark.skipif( not os.environ.get("CODEX_SMOKE_TEST") @@ -377,6 +379,7 @@ def _assert(output: _CodexProbeOutput) -> None: class _DenyRoundTripOutput(NamedTuple): transcript: str cli_version: str + artifact_names: tuple[str, ...] = () class _GeneratedChildProbeOutput(NamedTuple): @@ -641,9 +644,39 @@ def _run_shell_capture_probe(backend: str, tmp_path: Path) -> _DenyRoundTripOutp return _DenyRoundTripOutput( transcript=transcript, cli_version=_cli_version(command[0], env), + artifact_names=tuple( + path.name + for path in sorted( + (workspace / ".autoskillit" / "temp" / "shell_capture").glob("shell_*.log") + ) + ), ) +def _parse_capture_runner(command: str) -> tuple[str, str] | None: + try: + argv = shlex.split(command.splitlines()[-1]) + runner_index = next( + index for index, value in enumerate(argv) if value.endswith("_capture_artifacts.py") + ) + if argv[runner_index - 1] != "-I" or argv[runner_index + 1] != "run": + return None + encoded = argv[runner_index + 2] + capture_id = argv[runner_index + 4] + if re.fullmatch(r"[0-9a-f]{16}", capture_id) is None: + return None + decoded = base64.b64decode(encoded, validate=True).decode() + return decoded, capture_id + except ( + StopIteration, + IndexError, + ValueError, + binascii.Error, + UnicodeDecodeError, + ): + return None + + def _assert_shell_capture_round_trip(output: _DenyRoundTripOutput) -> None: denial_reason = _policy_denial_reason(output.transcript) assert denial_reason is None, ( @@ -653,10 +686,6 @@ def _assert_shell_capture_round_trip(output: _DenyRoundTripOutput) -> None: assert "autoskillit-shell-capture" in output.transcript, ( "Harness sentinel missing — hook may not have fired" ) - assert _OUTPUT_BUDGET_CANARY_COMMAND in output.transcript, ( - "Canary command text missing from transcript" - ) - completed_commands: list[str] = [] for line in output.transcript.splitlines(): try: @@ -674,18 +703,34 @@ def _assert_shell_capture_round_trip(output: _DenyRoundTripOutput) -> None: assert completed_commands, ( "No completed command_execution event found — the rewritten command did not execute" ) - assert any( - _OUTPUT_BUDGET_CANARY_COMMAND in command - and "autoskillit-shell-capture" in command - and _CAPTURE_ARTIFACT_PATH_RE.search(command) + parsed = [ + runner for command in completed_commands - ), "No completed rewritten command referenced the shell-capture artifact path" + if "autoskillit-shell-capture" in command + if (runner := _parse_capture_runner(command)) is not None + ] + assert parsed, "No completed rewritten command invoked the isolated shell-capture runner" + assert any(command == _OUTPUT_BUDGET_CANARY_COMMAND for command, _ in parsed), ( + "The completed runner invocation did not transport the canary command" + ) + artifact_ids = { + match.group(1) + for name in output.artifact_names + if (match := _CAPTURE_ARTIFACT_NAME_RE.fullmatch(name)) is not None + } + runner_ids = {capture_id for _, capture_id in parsed} + assert artifact_ids & runner_ids, ( + "No emitted shell-capture artifact matched the completed runner capture id" + ) def test_shell_capture_assertion_requires_completed_rewritten_command() -> None: - capture_path = "/tmp/workspace/.autoskillit/temp/shell_capture/shell_0123456789abcdef.log" + capture_id = "0123456789abcdef" + encoded = base64.b64encode(_OUTPUT_BUDGET_CANARY_COMMAND.encode()).decode() rewritten_command = ( - f"# autoskillit-shell-capture v1\n__as_f={capture_path}\n{_OUTPUT_BUDGET_CANARY_COMMAND}\n" + "# autoskillit-shell-capture v1\n" + f"/usr/bin/python3 -I /opt/autoskillit/_capture_artifacts.py run {encoded} " + f"/tmp/workspace {capture_id}" ) def _output(*, status: str, command: str = rewritten_command) -> _DenyRoundTripOutput: @@ -700,6 +745,7 @@ def _output(*, status: str, command: str = rewritten_command) -> _DenyRoundTripO return _DenyRoundTripOutput( transcript=json.dumps(event), cli_version="codex-cli test", + artifact_names=(f"shell_{capture_id}.log",), ) _assert_shell_capture_round_trip(_output(status="completed")) @@ -708,10 +754,10 @@ def _output(*, status: str, command: str = rewritten_command) -> _DenyRoundTripO with pytest.raises(AssertionError, match="No completed command_execution"): _assert_shell_capture_round_trip(_output(status=noncompleted_status)) - command_without_capture_path = rewritten_command.replace(capture_path, "/tmp/missing.log") - with pytest.raises(AssertionError, match="shell-capture artifact path"): + command_without_runner = rewritten_command.replace("_capture_artifacts.py", "other.py") + with pytest.raises(AssertionError, match="isolated shell-capture runner"): _assert_shell_capture_round_trip( - _output(status="completed", command=command_without_capture_path) + _output(status="completed", command=command_without_runner) ) diff --git a/tests/hooks/test_capture_artifacts.py b/tests/hooks/test_capture_artifacts.py index f7fe2efe2..3eaa3b3e0 100644 --- a/tests/hooks/test_capture_artifacts.py +++ b/tests/hooks/test_capture_artifacts.py @@ -110,6 +110,21 @@ def test_symlinked_policy_root_is_not_trusted(tmp_path: Path) -> None: anchor.close() +def test_symlinked_policy_leaf_is_not_trusted(tmp_path: Path) -> None: + project = tmp_path / "project" + temp_dir = project.joinpath(*CAPTURE_PATH_COMPONENTS[:2]) + temp_dir.mkdir(parents=True) + external_config = tmp_path / "external-config.json" + external_config.write_text(json.dumps({"output_budget_policy": {"disabled": True}})) + (temp_dir / ".hook_config.json").symlink_to(external_config) + + anchor = open_project_anchor(str(project)) + try: + assert read_capture_policy(anchor) == CapturePolicy() + finally: + anchor.close() + + def test_verified_policy_merges_overlay_and_bounds_inline_bytes(tmp_path: Path) -> None: project = tmp_path / "project" temp_dir = project.joinpath(*CAPTURE_PATH_COMPONENTS[:2]) @@ -195,6 +210,66 @@ def test_stale_capture_is_safely_retained_without_identity_unlink(tmp_path: Path assert stale.read_bytes() == b"retained" +def test_cleanup_candidate_classification_rejects_unsafe_entries(tmp_path: Path) -> None: + project = tmp_path / "project" + project.mkdir() + anchor, root = _open_authority(project) + capture_dir = _capture_dir(project) + valid = capture_dir / "shell_1111111111111111.log" + valid.write_text("valid") + world_writable = capture_dir / "shell_2222222222222222.log" + world_writable.write_text("unsafe") + os.chmod(world_writable, 0o666) + hardlink_source = tmp_path / "hardlink-source" + hardlink_source.write_text("unsafe") + hardlink = capture_dir / "shell_3333333333333333.log" + try: + os.link(hardlink_source, hardlink) + except OSError: + root.close() + anchor.close() + pytest.skip("hardlinks unavailable") + + try: + threshold = time.time() - 100 + assert ( + capture_artifacts._open_stale_candidate( + root, + valid.name, + mtime_threshold=threshold, + ) + is None + ) + os.utime(valid, (threshold - 100, threshold - 100)) + candidate = capture_artifacts._open_stale_candidate( + root, + valid.name, + mtime_threshold=threshold, + ) + assert candidate is not None + assert candidate.inode == valid.stat().st_ino + candidate.close() + assert ( + capture_artifacts._open_stale_candidate( + root, + world_writable.name, + mtime_threshold=time.time() + 1, + ) + is None + ) + assert ( + capture_artifacts._open_stale_candidate( + root, + hardlink.name, + mtime_threshold=time.time() + 1, + ) + is None + ) + finally: + root.close() + anchor.close() + + def test_cleanup_rejects_symlinked_capture_root(tmp_path: Path) -> None: project = tmp_path / "project" temp_dir = project.joinpath(*CAPTURE_PATH_COMPONENTS[:2]) @@ -211,6 +286,40 @@ def test_cleanup_rejects_symlinked_capture_root(tmp_path: Path) -> None: assert stale.read_bytes() == b"must-survive" +def test_cleanup_retains_replacement_raced_after_validation( + tmp_path: Path, + monkeypatch: pytest.MonkeyPatch, +) -> None: + project = tmp_path / "project" + capture_dir = _capture_dir(project) + capture_dir.mkdir(parents=True) + stale = capture_dir / f"shell_{_CAPTURE_ID}.log" + stale.write_bytes(b"validated") + os.utime(stale, (0, 0)) + displaced = capture_dir / "validated-inode" + real_open = capture_artifacts._open_stale_candidate + swapped = False + + def swap_after_validation(root, name, *, mtime_threshold): + nonlocal swapped + candidate = real_open(root, name, mtime_threshold=mtime_threshold) + if candidate is not None and not swapped: + stale.rename(displaced) + stale.write_bytes(b"replacement") + swapped = True + return candidate + + monkeypatch.setattr( + capture_artifacts, + "_open_stale_candidate", + swap_after_validation, + ) + + assert sweep_stale_captures(project, max_age_seconds=0) == 0 + assert stale.read_bytes() == b"replacement" + assert displaced.read_bytes() == b"validated" + + def test_setup_failure_prevents_user_command(tmp_path: Path) -> None: project = tmp_path / "project" temp_dir = project.joinpath(*CAPTURE_PATH_COMPONENTS[:2]) @@ -264,3 +373,46 @@ def fail_spawn(*args, **kwargs): for fd in observed_fds: with pytest.raises(OSError): os.fstat(fd) + + +def test_artifact_write_failure_emits_failure_marker( + tmp_path: Path, + monkeypatch: pytest.MonkeyPatch, + capfd: pytest.CaptureFixture[str], +) -> None: + project = tmp_path / "project" + project.mkdir() + + def fail_write(fd, data): + raise OSError("fault injection") + + monkeypatch.setattr(capture_artifacts, "_write_all", fail_write) + + assert run_capture("printf ran > command_ran; printf output", str(project), _CAPTURE_ID) == 0 + captured = capfd.readouterr() + assert (project / "command_ran").read_text() == "ran" + assert "CAPTURE_FAILED" in captured.err + assert "SHELL_OUTPUT_CAPTURED" not in captured.out + captured.err + + +def test_marker_verification_failure_emits_failure_marker( + tmp_path: Path, + monkeypatch: pytest.MonkeyPatch, + capfd: pytest.CaptureFixture[str], +) -> None: + project = tmp_path / "project" + project.mkdir() + + def fail_verification(anchor, root, artifact): + raise OSError("fault injection") + + monkeypatch.setattr( + capture_artifacts, + "current_artifact_path_if_bound", + fail_verification, + ) + + assert run_capture("printf output", str(project), _CAPTURE_ID) == 0 + captured = capfd.readouterr() + assert "CAPTURE_FAILED" in captured.err + assert "SHELL_OUTPUT_CAPTURED" not in captured.out + captured.err diff --git a/tests/hooks/test_session_start_hook.py b/tests/hooks/test_session_start_hook.py index 60a4b5210..45cac5698 100644 --- a/tests/hooks/test_session_start_hook.py +++ b/tests/hooks/test_session_start_hook.py @@ -20,13 +20,19 @@ SCRIPT = Path(__file__).resolve().parents[2] / "src/autoskillit/hooks/session_start_hook.py" -def _run(stdin_data: str, env: dict | None = None) -> tuple[int, str]: +def _run( + stdin_data: str, + env: dict | None = None, + *, + cwd: Path | None = None, +) -> tuple[int, str]: result = subprocess.run( [sys.executable, str(SCRIPT)], input=stdin_data, capture_output=True, text=True, env=env, + cwd=cwd, ) return result.returncode, result.stdout @@ -71,3 +77,46 @@ def test_session_start_hook_registry_scope() -> None: if h.event_type == "SessionStart" and "session_start_hook.py" in h.scripts ) assert hook_def.session_scope == "interactive_only" + + +def test_session_start_cleanup_does_not_follow_symlinked_capture_root( + tmp_path: Path, +) -> None: + project = tmp_path / "project" + temp_dir = project / ".autoskillit" / "temp" + temp_dir.mkdir(parents=True) + external = tmp_path / "external" + external.mkdir() + stale = external / "shell_0123456789abcdef.log" + stale.write_bytes(b"must-survive") + os.utime(stale, (0, 0)) + (temp_dir / "shell_capture").symlink_to(external, target_is_directory=True) + transcript = tmp_path / "empty-transcript.jsonl" + transcript.write_text("") + payload = json.dumps({"session_id": "abc", "transcript_path": str(transcript)}) + env = {k: v for k, v in os.environ.items() if k != "AUTOSKILLIT_HEADLESS"} + env["AUTOSKILLIT_STATE_DIR"] = str(tmp_path / "empty-state") + + rc, _ = _run(payload, env=env, cwd=project) + + assert rc == 0 + assert stale.read_bytes() == b"must-survive" + + +def test_session_start_cleanup_failure_keeps_resume_reminder( + tmp_path: Path, +) -> None: + project = tmp_path / "project" + temp_dir = project / ".autoskillit" / "temp" + temp_dir.mkdir(parents=True) + (temp_dir / "shell_capture").write_text("blocking regular file") + transcript = tmp_path / "resumed.jsonl" + transcript.write_text('{"type":"say","text":"hello"}\n') + payload = json.dumps({"session_id": "abc", "transcript_path": str(transcript)}) + env = {k: v for k, v in os.environ.items() if k != "AUTOSKILLIT_HEADLESS"} + env["AUTOSKILLIT_STATE_DIR"] = str(tmp_path / "empty-state") + + rc, out = _run(payload, env=env, cwd=project) + + assert rc == 0 + assert "additionalContext" in json.loads(out) diff --git a/tests/hooks/test_shell_capture_conformance.py b/tests/hooks/test_shell_capture_conformance.py index f0ab8ef1d..e5eea63a1 100644 --- a/tests/hooks/test_shell_capture_conformance.py +++ b/tests/hooks/test_shell_capture_conformance.py @@ -9,20 +9,23 @@ from __future__ import annotations +import io +import json import os -import re +import shlex import shutil import subprocess import time -from collections.abc import Callable +from contextlib import redirect_stdout from pathlib import Path +from types import SimpleNamespace from uuid import uuid4 import pytest -from autoskillit.hooks._capture_cleanup import ( +import autoskillit.hooks.shell_capture_hook as shell_capture_hook +from autoskillit.hooks._capture_artifacts import ( _CAPTURE_FILENAME_RE, - _is_safe_capture_file, sweep_stale_captures, ) from autoskillit.hooks.shell_capture_hook import _build_harness @@ -73,7 +76,7 @@ "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 + ("nested_wrap", None), ] @@ -111,7 +114,7 @@ def _run_raw_merged(command: str, tmp_path: Path) -> subprocess.CompletedProcess def _run_wrapped(command: str, tmp_path: Path) -> subprocess.CompletedProcess[bytes]: - wrapped = _build_harness(command, str(tmp_path), _INLINE_BYTES) + wrapped = _build_harness(command, str(tmp_path), uuid4().hex[:16]) return subprocess.run( ["bash", "-c", wrapped], capture_output=True, @@ -120,10 +123,15 @@ def _run_wrapped(command: str, tmp_path: Path) -> subprocess.CompletedProcess[by ) -# 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)) +def _main_generated_wrapper(command: str, cwd: Path, monkeypatch: pytest.MonkeyPatch) -> str: + event = {"cwd": str(cwd), "tool_input": {"command": command}} + monkeypatch.setenv("AUTOSKILLIT_AGENT_BACKEND", "codex") + monkeypatch.setattr("sys.stdin", io.StringIO(json.dumps(event))) + output = io.StringIO() + with pytest.raises(SystemExit), redirect_stdout(output): + shell_capture_hook.main() + payload = json.loads(output.getvalue()) + return payload["hookSpecificOutput"]["updatedInput"]["command"] @pytest.mark.parametrize("label,command", _CORPUS, ids=[row[0] for row in _CORPUS]) @@ -134,6 +142,8 @@ def test_capture_conformance(label: str, command: str, tmp_path: Path) -> None: pytest.skip("jq not available") _make_project_dirs(tmp_path) + if label == "nested_wrap": + command = _build_harness(_NESTED_WRAP_INNER, str(tmp_path), uuid4().hex[:16]) raw = _run_raw(command, tmp_path) raw_combined = raw.stdout + raw.stderr @@ -143,6 +153,9 @@ def test_capture_conformance(label: str, command: str, tmp_path: Path) -> None: # wrapped run's append produces byte-identical content to compare. report = tmp_path / ".autoskillit" / "temp" / "investigate" / "report.md" report.unlink(missing_ok=True) + if label == "nested_wrap": + for artifact in _artifact_files(tmp_path): + artifact.unlink() wrapped = _run_wrapped(command, tmp_path) @@ -187,10 +200,10 @@ def test_capture_conformance(label: str, command: str, tmp_path: Path) -> None: 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 $?. + # The isolated runner deliberately translates a child signal to the + # shell-compatible 128+signal status. assert raw.returncode == -15 - assert wrapped.returncode == -15 + assert wrapped.returncode == 143 if artifacts: assert b"pre" in artifacts[0].read_bytes() return @@ -260,6 +273,199 @@ def test_capture_dir_uncreatable_fail_stops(tmp_path: Path) -> None: assert "should_not_run" not in combined +@pytest.mark.parametrize("component", [".autoskillit", "temp", "shell_capture"]) +def test_main_generated_wrapper_rejects_symlinked_capture_components( + component: str, + tmp_path: Path, + monkeypatch: pytest.MonkeyPatch, +) -> None: + project = tmp_path / "project" + project.mkdir() + external = tmp_path / "external" + external.mkdir() + secret = external / "secret" + secret.write_text("must-not-be-read") + + parent = project + for name in (".autoskillit", "temp", "shell_capture"): + candidate = parent / name + if name == component: + candidate.symlink_to(external, target_is_directory=True) + break + candidate.mkdir() + parent = candidate + if component == "temp": + (external / ".hook_config.json").write_text( + json.dumps({"output_budget_policy": {"disabled": True}}) + ) + + wrapper = _main_generated_wrapper( + "printf ran > command_ran", + project, + monkeypatch, + ) + completed = subprocess.run( + ["bash", "-c", wrapper], + cwd=project, + capture_output=True, + text=True, + check=False, + ) + + assert completed.returncode == 1 + assert "CAPTURE_FAILED" in completed.stdout + completed.stderr + assert not (project / "command_ran").exists() + assert not list(external.glob("shell_*.log")) + assert secret.read_text() == "must-not-be-read" + assert "must-not-be-read" not in completed.stdout + completed.stderr + + +def test_main_generated_wrapper_accepts_symlinked_cwd( + tmp_path: Path, + monkeypatch: pytest.MonkeyPatch, +) -> None: + project = tmp_path / "project" + project.mkdir() + supplied_cwd = tmp_path / "project-link" + supplied_cwd.symlink_to(project, target_is_directory=True) + + wrapper = _main_generated_wrapper("printf anchored", supplied_cwd, monkeypatch) + completed = subprocess.run( + ["bash", "-c", wrapper], + cwd=supplied_cwd, + capture_output=True, + check=False, + ) + + assert completed.returncode == 0 + assert completed.stdout == b"anchored" + assert len(_artifact_files(project)) == 1 + + +@pytest.mark.parametrize("collision", ["symlink", "hardlink", "regular"]) +def test_main_generated_wrapper_rejects_final_artifact_collisions( + collision: str, + tmp_path: Path, + monkeypatch: pytest.MonkeyPatch, +) -> None: + project = tmp_path / "project" + _make_project_dirs(project) + capture_id = "a1b2c3d4e5f60718" + monkeypatch.setattr( + shell_capture_hook, + "uuid4", + lambda: SimpleNamespace(hex=capture_id + "0" * 16), + ) + artifact = _capture_dir(project) / f"shell_{capture_id}.log" + external = tmp_path / "external-secret" + external.write_bytes(b"must-survive") + if collision == "symlink": + artifact.symlink_to(external) + elif collision == "hardlink": + try: + os.link(external, artifact) + except OSError: + pytest.skip("hardlinks unavailable") + else: + artifact.write_bytes(b"existing") + + wrapper = _main_generated_wrapper( + "printf ran > command_ran", + project, + monkeypatch, + ) + completed = subprocess.run( + ["bash", "-c", wrapper], + cwd=project, + capture_output=True, + check=False, + ) + + assert completed.returncode == 1 + combined = (completed.stdout + completed.stderr).decode() + assert "CAPTURE_FAILED" in combined + assert "must-survive" not in combined + assert not (project / "command_ran").exists() + assert external.read_bytes() == b"must-survive" + if collision == "regular": + assert artifact.read_bytes() == b"existing" + + +def test_capture_directory_replacement_uses_open_fds_and_hides_path( + tmp_path: Path, + monkeypatch: pytest.MonkeyPatch, +) -> None: + project = tmp_path / "project" + _make_project_dirs(project) + config = project / ".autoskillit" / "temp" / ".hook_config.json" + config.write_text(json.dumps({"output_budget_policy": {"shell_max_inline_bytes": 8}})) + external = tmp_path / "replacement-target" + external.mkdir() + command = ( + "mv .autoskillit/temp/shell_capture " + ".autoskillit/temp/shell_capture-original; " + f"ln -s {shlex.quote(str(external))} .autoskillit/temp/shell_capture; " + "printf 0123456789abcdef" + ) + + wrapper = _main_generated_wrapper(command, project, monkeypatch) + completed = subprocess.run( + ["bash", "-c", wrapper], + cwd=project, + capture_output=True, + check=False, + ) + + assert completed.returncode == 0 + assert b"-> unavailable " in completed.stdout + assert b"SHELL_OUTPUT_CAPTURED" in completed.stdout + displaced = project / ".autoskillit" / "temp" / "shell_capture-original" + artifacts = sorted(displaced.glob("shell_*.log")) + assert len(artifacts) == 1 + assert artifacts[0].read_bytes() == b"0123456789abcdef" + assert not list(external.iterdir()) + + +def test_capture_artifact_replacement_uses_open_fd_and_hides_path( + tmp_path: Path, + monkeypatch: pytest.MonkeyPatch, +) -> None: + project = tmp_path / "project" + _make_project_dirs(project) + config = project / ".autoskillit" / "temp" / ".hook_config.json" + config.write_text(json.dumps({"output_budget_policy": {"shell_max_inline_bytes": 8}})) + capture_id = "1029384756abcdef" + monkeypatch.setattr( + shell_capture_hook, + "uuid4", + lambda: SimpleNamespace(hex=capture_id + "0" * 16), + ) + capture_dir = _capture_dir(project) + artifact = capture_dir / f"shell_{capture_id}.log" + displaced = capture_dir / "opened-artifact.log" + external = tmp_path / "external-target" + external.write_bytes(b"must-survive") + command = ( + f"mv {shlex.quote(str(artifact))} {shlex.quote(str(displaced))}; " + f"ln -s {shlex.quote(str(external))} {shlex.quote(str(artifact))}; " + "printf fedcba9876543210" + ) + + wrapper = _main_generated_wrapper(command, project, monkeypatch) + completed = subprocess.run( + ["bash", "-c", wrapper], + cwd=project, + capture_output=True, + check=False, + ) + + assert completed.returncode == 0 + assert b"-> unavailable " in completed.stdout + assert displaced.read_bytes() == b"fedcba9876543210" + assert external.read_bytes() == b"must-survive" + assert artifact.is_symlink() + + @pytest.mark.parametrize( "cmd", ["echo hello", "ls -la", "cat /dev/null", "python3 -c 'print(1)'", ""], @@ -271,7 +477,7 @@ def test_harness_contains_no_destructive_verbs(cmd: str) -> None: hook-injected scaffolding. Destructive verbs (rm, unlink, etc.) are forbidden by Codex's built-in policy. This test ensures the harness never introduces them. """ - harness = _build_harness(cmd, "/tmp/test", _INLINE_BYTES) + harness = _build_harness(cmd, "/tmp/test", uuid4().hex[:16]) for raw_line in harness.splitlines(): line = raw_line.strip() if not line or line.startswith("#"): @@ -287,8 +493,8 @@ def test_harness_contains_no_destructive_verbs(cmd: str) -> None: ) -def test_small_output_artifact_cleaned_by_sweep(tmp_path: Path) -> None: - """Small-output captures are retained on disk but reaped by the Python-side sweep.""" +def test_small_output_artifact_safely_retained_by_sweep(tmp_path: Path) -> None: + """Portable cleanup retains stale captures without identity-conditioned unlink.""" _make_project_dirs(tmp_path) command = "echo hello_small" wrapped = _run_wrapped(command, tmp_path) @@ -298,11 +504,10 @@ def test_small_output_artifact_cleaned_by_sweep(tmp_path: Path) -> None: assert len(artifacts) == 1 target = artifacts[0] - # Force mtime into the past so max_age_seconds=0 sweeps it. os.utime(target, (0, 0)) - deleted = sweep_stale_captures(_capture_dir(tmp_path), max_age_seconds=0) - assert deleted == 1 - assert not target.exists() + deleted = sweep_stale_captures(tmp_path, max_age_seconds=0) + assert deleted == 0 + assert target.exists() def test_sweep_rejects_symlinks_and_traversals(tmp_path: Path) -> None: @@ -316,7 +521,7 @@ def test_sweep_rejects_symlinks_and_traversals(tmp_path: Path) -> None: symlink.symlink_to(outside) assert symlink.is_symlink() - deleted = sweep_stale_captures(capture, max_age_seconds=0) + deleted = sweep_stale_captures(tmp_path, max_age_seconds=0) assert deleted == 0 assert outside.exists(), "outside target file must be untouched by sweep" assert outside.read_text() == "must-survive" @@ -337,9 +542,9 @@ def test_sweep_filename_allowlist(tmp_path: Path) -> None: invalid_ext = capture / "evil.sh" invalid_ext.write_text("x") - deleted = sweep_stale_captures(capture, max_age_seconds=0) - assert deleted == 1 - assert not valid.exists() + deleted = sweep_stale_captures(tmp_path, max_age_seconds=0) + assert deleted == 0 + assert valid.exists() assert short_uid.exists(), "old 8-char uid format files must not be deleted" assert invalid_ext.exists(), "files outside the shell_*.log allowlist must not be deleted" @@ -357,89 +562,13 @@ def test_sweep_stale_captures_uses_wall_clock_not_directory_mtime(tmp_path: Path os.utime(stale, (backdated, backdated)) os.utime(capture, (backdated, backdated)) - deleted = sweep_stale_captures(capture, max_age_seconds=100) - assert deleted == 1 - assert not stale.exists(), ( - "file older than max_age_seconds must be swept even when the capture " - "directory's own mtime is equally stale" - ) - - -def _cc_case_valid(capture: Path) -> Path: - good = capture / f"shell_{uuid4().hex[:16]}.log" - good.write_text("ok") - return good - - -def _cc_case_symlink(capture: Path) -> Path: - target = capture / f"shell_{uuid4().hex[:16]}.log" - target.write_text("ok") - sym = capture / f"shell_{uuid4().hex[:16]}.log" - sym.symlink_to(target) - return sym - - -def _cc_case_traversal(capture: Path) -> Path: - traversal = capture / "../../escape.log" - traversal.write_text("x") - return traversal - - -def _cc_case_hardlink(capture: Path) -> Path: - source = capture.parent / "hardlink_source.log" - source.write_text("data") - hardlinked = capture / f"shell_{uuid4().hex[:16]}.log" - try: - os.link(source, hardlinked) - except OSError: - pytest.skip("hardlink not supported in this environment") - return hardlinked - - -def _cc_case_world_writable(capture: Path) -> Path: - path = capture / f"shell_{uuid4().hex[:16]}.log" - path.write_text("x") - os.chmod(path, 0o666) - return path - - -@pytest.mark.parametrize( - "case_builder,expect_safe", - [ - (_cc_case_valid, True), - (_cc_case_symlink, False), - (_cc_case_traversal, False), - (_cc_case_hardlink, False), - (_cc_case_world_writable, False), - ], - ids=["valid", "symlink", "traversal", "hardlink", "world_writable"], -) -def test_capture_cleanup_containment_parity( - case_builder: Callable[[Path], Path], expect_safe: bool, tmp_path: Path -) -> None: - """_is_safe_capture_file rejects the same attack vectors as core.path_containment.""" - from autoskillit.core.path_containment import ContainmentError, resolve_contained_path - - _make_project_dirs(tmp_path) - capture = _capture_dir(tmp_path) - path = case_builder(capture) - - assert _is_safe_capture_file(path, capture) is expect_safe - if expect_safe: - try: - resolve_contained_path(path, capture) - except ContainmentError as exc: - pytest.fail(f"core containment rejected a valid capture file: {exc}") - else: - with pytest.raises(ContainmentError): - resolve_contained_path(path, capture) + deleted = sweep_stale_captures(tmp_path, max_age_seconds=100) + assert deleted == 0 + assert stale.exists() def test_capture_filename_regex_consistency() -> None: - """Hook module and sweep module must agree on the shell_*.log regex.""" + """The canonical helper accepts only the current capture filename contract.""" sample = f"shell_{uuid4().hex[:16]}.log" - assert _CAPTURE_FILENAME_RE.match(sample) is not None - - # The inline literal in session_start_hook.py must accept the same filename. - inline_pattern = r"^shell_[0-9a-f]{16}\.log$" - assert re.match(inline_pattern, sample) is not None + assert _CAPTURE_FILENAME_RE.fullmatch(sample) is not None + assert _CAPTURE_FILENAME_RE.fullmatch("shell_deadbeef.log") is None diff --git a/tests/hooks/test_shell_capture_hook.py b/tests/hooks/test_shell_capture_hook.py index 02c43c1c4..db3c085a3 100644 --- a/tests/hooks/test_shell_capture_hook.py +++ b/tests/hooks/test_shell_capture_hook.py @@ -2,13 +2,18 @@ from __future__ import annotations +import base64 import io import json +import re +import shlex +import subprocess from contextlib import redirect_stdout +from pathlib import Path import pytest -pytestmark = [pytest.mark.layer("hooks"), pytest.mark.small] +pytestmark = [pytest.mark.layer("hooks"), pytest.mark.medium] _SENTINEL = "# autoskillit-shell-capture v1" @@ -39,6 +44,21 @@ def _updated_command(output: str) -> str: return payload["hookSpecificOutput"]["updatedInput"]["command"] +def _runner_argv(command: str) -> list[str]: + lines = command.splitlines() + assert lines[0] == _SENTINEL + assert len(lines) == 2 + return shlex.split(lines[1]) + + +def _transported_command(command: str) -> str: + argv = _runner_argv(command) + assert argv[1] == "-I" + assert Path(argv[2]).name == "_capture_artifacts.py" + assert argv[3] == "run" + return base64.b64decode(argv[4], validate=True).decode() + + def test_silent_off_codex(monkeypatch): event = _build_event("echo hello") @@ -55,7 +75,8 @@ def test_rewrites_on_codex_env(monkeypatch): assert payload["hookSpecificOutput"]["permissionDecision"] == "allow" command = payload["hookSpecificOutput"]["updatedInput"]["command"] assert _SENTINEL in command - assert "echo hello" in command + assert _transported_command(command) == "echo hello" + assert "echo hello" not in command def test_rewrites_on_turn_id_payload(monkeypatch): @@ -68,7 +89,7 @@ def test_rewrites_on_turn_id_payload(monkeypatch): assert payload["hookSpecificOutput"]["permissionDecision"] == "allow" command = payload["hookSpecificOutput"]["updatedInput"]["command"] assert _SENTINEL in command - assert "echo hello" in command + assert _transported_command(command) == "echo hello" def test_always_wraps_sentinel_prefixed_command(monkeypatch): @@ -76,18 +97,19 @@ def test_always_wraps_sentinel_prefixed_command(monkeypatch): 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 + assert command.count(_SENTINEL) == 1 + assert _transported_command(command) == already_wrapped -def test_newline_normalization_applied(monkeypatch): - output = _run_hook(_build_event("echo test"), monkeypatch, env_backend="codex") +@pytest.mark.parametrize("original", ["echo test", "echo test\n"]) +def test_command_transport_preserves_original(monkeypatch, original): + output = _run_hook(_build_event(original), monkeypatch, env_backend="codex") command = _updated_command(output) - assert "echo test\n)" in command + assert _transported_command(command) == original -def test_disabled_policy_no_rewrite(monkeypatch, tmp_path): +def test_verified_disabled_policy_is_runner_owned(monkeypatch, tmp_path): config_dir = tmp_path / ".autoskillit" / "temp" config_dir.mkdir(parents=True) (config_dir / ".hook_config.json").write_text( @@ -95,7 +117,20 @@ def test_disabled_policy_no_rewrite(monkeypatch, tmp_path): ) event = _build_event("echo hello", cwd=str(tmp_path)) - assert _run_hook(event, monkeypatch, env_backend="codex") == "" + output = _run_hook(event, monkeypatch, env_backend="codex") + command = _updated_command(output) + completed = subprocess.run( + ["bash", "-c", command], + cwd=tmp_path, + capture_output=True, + text=True, + check=False, + ) + + assert completed.returncode == 0 + assert completed.stdout == "hello\n" + assert "SHELL_OUTPUT_CAPTURED" not in completed.stdout + assert not (config_dir / "shell_capture").exists() def test_malformed_stdin_fails_open(monkeypatch): @@ -115,25 +150,46 @@ def test_non_string_command_fails_open(monkeypatch): 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) +@pytest.mark.parametrize("command", ["x" * (64 * 1024 + 1), "printf bad\x00command"]) +def test_invalid_command_transport_builds_nonexecuting_rejection(command): + from autoskillit.hooks.shell_capture_hook import _build_harness - assert "AutoSkillit" in command - assert "shell_capture_hook" in command + harness = _build_harness(command, "/abs/project", "0123456789abcdef") + argv = _runner_argv(harness) - import re + assert argv[3] == "reject" + assert command not in harness - 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 +def test_marker_provenance_is_emitted_by_runner(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": {"shell_max_inline_bytes": 8}}) + ) + output = _run_hook( + _build_event("printf 0123456789abcdef", cwd=str(tmp_path)), + monkeypatch, + env_backend="codex", + ) + command = _updated_command(output) + argv = _runner_argv(command) + + assert argv[1] == "-I" + assert Path(argv[2]).name == "_capture_artifacts.py" + assert re.fullmatch(r"[0-9a-f]{16}", argv[-1]) + assert "printf 0123456789abcdef" not in command + assert "AutoSkillit" not in command + assert "`" not in command + + completed = subprocess.run( + ["bash", "-c", command], + cwd=tmp_path, + capture_output=True, + text=True, + check=False, + ) + assert completed.returncode == 0 + assert "AutoSkillit hook shell_capture_hook" in completed.stdout + assert "SHELL_OUTPUT_CAPTURED" in completed.stdout + assert f"shell_{argv[-1]}.log" in completed.stdout From 326b94f54928b4c531aa6ec18459f7f433e11396 Mon Sep 17 00:00:00 2001 From: Trecek Date: Wed, 22 Jul 2026 22:56:28 -0700 Subject: [PATCH 03/32] fix: complete shell capture trust-anchor remediation --- src/autoskillit/hooks/_capture_artifacts.py | 30 +++- src/autoskillit/hooks/shell_capture_hook.py | 44 ++++- .../backends/test_cli_conformance_probes.py | 18 +- .../backends/test_hook_deny_efficacy_probe.py | 18 +- .../backends/test_hook_strength_matrix.py | 13 ++ tests/hooks/test_capture_artifacts.py | 163 ++++++++++++++++++ tests/hooks/test_shell_capture_conformance.py | 18 +- tests/hooks/test_shell_capture_hook.py | 29 ++++ 8 files changed, 315 insertions(+), 18 deletions(-) diff --git a/src/autoskillit/hooks/_capture_artifacts.py b/src/autoskillit/hooks/_capture_artifacts.py index bbae67b79..ebfe4bb9e 100644 --- a/src/autoskillit/hooks/_capture_artifacts.py +++ b/src/autoskillit/hooks/_capture_artifacts.py @@ -21,20 +21,26 @@ import time from dataclasses import dataclass from pathlib import Path +from typing import TYPE_CHECKING _HOOKS_DIR = str(Path(__file__).resolve().parent) if _HOOKS_DIR not in sys.path: sys.path.insert(0, _HOOKS_DIR) -from _hook_settings import ( # type: ignore[import-not-found] # noqa: E402 - HOOK_CONFIG_FILENAME, - HOOK_CONFIG_OVERLAY_FILENAME, - merge_hook_configs, -) -from _policy_event import ( # type: ignore[import-not-found] # noqa: E402 - PolicyEvent, - render_capture_marker, -) +if TYPE_CHECKING: + from autoskillit.hooks._hook_settings import ( + HOOK_CONFIG_FILENAME, + HOOK_CONFIG_OVERLAY_FILENAME, + merge_hook_configs, + ) + from autoskillit.hooks._policy_event import PolicyEvent, render_capture_marker +else: + from _hook_settings import ( + HOOK_CONFIG_FILENAME, + HOOK_CONFIG_OVERLAY_FILENAME, + merge_hook_configs, + ) + from _policy_event import PolicyEvent, render_capture_marker __all__ = [ "CAPTURE_PATH_COMPONENTS", @@ -596,6 +602,7 @@ def run_capture(command: str, cwd: str, capture_id: str) -> int: anchor = open_project_anchor(cwd) root: CaptureRoot | None = None artifact: CaptureArtifact | None = None + process: subprocess.Popen[bytes] | None = None try: policy = read_capture_policy(anchor) bash_path = _resolve_bash() @@ -628,6 +635,8 @@ def run_capture(command: str, cwd: str, capture_id: str) -> int: _emit_capture(result, artifact_path, policy.inline_bytes) return returncode finally: + if process is not None and process.stdout is not None: + process.stdout.close() if artifact is not None: artifact.close() if root is not None: @@ -723,6 +732,9 @@ def _decode_command(value: str) -> str: def _main(argv: list[str] | None = None) -> int: args = sys.argv[1:] if argv is None else argv if len(args) == 2 and args[0] == "reject": + if not _CAPTURE_ID_RE.fullmatch(args[1]): + _emit_failure("invalid capture id") + return 1 _emit_failure("capture request rejected before command execution") return 1 if len(args) != 4 or args[0] != "run": diff --git a/src/autoskillit/hooks/shell_capture_hook.py b/src/autoskillit/hooks/shell_capture_hook.py index cb766c497..b9128efca 100644 --- a/src/autoskillit/hooks/shell_capture_hook.py +++ b/src/autoskillit/hooks/shell_capture_hook.py @@ -16,6 +16,7 @@ import os import re import shlex +import struct import sys from pathlib import Path from uuid import uuid4 @@ -23,13 +24,44 @@ _HARNESS_SENTINEL = "# autoskillit-shell-capture v1" _CAPTURE_ID_RE = re.compile(r"^[0-9a-f]{16}$") _MAX_COMMAND_BYTES = 64 * 1024 -_RUNNER_PATH = Path(__file__).resolve().with_name("_capture_artifacts.py") +_ARG_MAX_FALLBACK_BYTES = 128 * 1024 +_ARG_MAX_HEADROOM_BYTES = 32 * 1024 +_RUNNER_BASENAME = "_capture_artifacts.py" def _is_codex_session(payload: dict) -> bool: return os.environ.get("AUTOSKILLIT_AGENT_BACKEND") == "codex" or "turn_id" in payload +def _system_arg_max() -> int: + try: + value = os.sysconf("SC_ARG_MAX") + except (AttributeError, OSError, ValueError): + return _ARG_MAX_FALLBACK_BYTES + return value if isinstance(value, int) and value > 0 else _ARG_MAX_FALLBACK_BYTES + + +def _exec_footprint(argv: list[str]) -> int: + argv_bytes = sum(len(os.fsencode(argument)) + 1 for argument in argv) + environment_bytes = sum( + len(os.fsencode(key)) + len(os.fsencode(value)) + 2 for key, value in os.environ.items() + ) + pointer_bytes = (len(argv) + len(os.environ) + 2) * struct.calcsize("P") + return argv_bytes + environment_bytes + pointer_bytes + + +def _fits_arg_max(argv: list[str]) -> bool: + return _exec_footprint(argv) + _ARG_MAX_HEADROOM_BYTES <= _system_arg_max() + + +def _render_harness(argv: list[str]) -> str: + return f"{_HARNESS_SENTINEL}\n{shlex.join(argv)}" + + +def _runner_path() -> Path: + return Path(__file__).resolve().with_name(_RUNNER_BASENAME) + + def _build_harness(command: str, cwd: str, capture_id: str) -> str: """Build a shell-safe isolated runner invocation without embedding ``command``.""" @@ -40,19 +72,23 @@ def _build_harness(command: str, cwd: str, capture_id: str) -> str: except UnicodeEncodeError: command_bytes = b"" if "\x00" in command or not command_bytes or len(command_bytes) > _MAX_COMMAND_BYTES: - argv = [sys.executable, "-I", str(_RUNNER_PATH), "reject", capture_id] + argv = [sys.executable, "-I", str(_runner_path()), "reject", capture_id] else: encoded = base64.b64encode(command_bytes).decode("ascii") argv = [ sys.executable, "-I", - str(_RUNNER_PATH), + str(_runner_path()), "run", encoded, cwd, capture_id, ] - return f"{_HARNESS_SENTINEL}\n{shlex.join(argv)}" + harness = _render_harness(argv) + outer_argv = ["bash", "-c", harness] + if not _fits_arg_max(argv) or not _fits_arg_max(outer_argv): + argv = [sys.executable, "-I", str(_runner_path()), "reject", capture_id] + return _render_harness(argv) def main() -> None: diff --git a/tests/execution/backends/test_cli_conformance_probes.py b/tests/execution/backends/test_cli_conformance_probes.py index a52d602e1..3865d0b56 100644 --- a/tests/execution/backends/test_cli_conformance_probes.py +++ b/tests/execution/backends/test_cli_conformance_probes.py @@ -600,6 +600,7 @@ def _run_shell_capture_probe(backend: str, tmp_path: Path) -> _DenyRoundTripOutp tmp_path.mkdir(parents=True, exist_ok=True) workspace = tmp_path / "workspace" workspace.mkdir() + (workspace / "output_budget_probe.txt").write_text(("output_budget_probe " * 1_000) + "\n") env, codex_home, _claude_config = _isolated_cli_env(tmp_path, workspace) if backend == "codex": @@ -686,6 +687,9 @@ def _assert_shell_capture_round_trip(output: _DenyRoundTripOutput) -> None: assert "autoskillit-shell-capture" in output.transcript, ( "Harness sentinel missing — hook may not have fired" ) + assert "SHELL_OUTPUT_CAPTURED" in output.transcript, ( + "Shell-capture marker missing — the live CLI path did not exercise bounded replay" + ) completed_commands: list[str] = [] for line in output.transcript.splitlines(): try: @@ -733,13 +737,23 @@ def test_shell_capture_assertion_requires_completed_rewritten_command() -> None: f"/tmp/workspace {capture_id}" ) - def _output(*, status: str, command: str = rewritten_command) -> _DenyRoundTripOutput: + def _output( + *, + status: str, + command: str = rewritten_command, + include_marker: bool = True, + ) -> _DenyRoundTripOutput: event = { "type": "item.completed", "item": { "type": "command_execution", "command": command, "status": status, + "aggregated_output": ( + "[AutoSkillit hook shell_capture_hook v1 (code=SHELL_OUTPUT_CAPTURED)]" + if include_marker + else "marker absent" + ), }, } return _DenyRoundTripOutput( @@ -749,6 +763,8 @@ def _output(*, status: str, command: str = rewritten_command) -> _DenyRoundTripO ) _assert_shell_capture_round_trip(_output(status="completed")) + with pytest.raises(AssertionError, match="Shell-capture marker missing"): + _assert_shell_capture_round_trip(_output(status="completed", include_marker=False)) for noncompleted_status in ("denied", "failed"): with pytest.raises(AssertionError, match="No completed command_execution"): diff --git a/tests/execution/backends/test_hook_deny_efficacy_probe.py b/tests/execution/backends/test_hook_deny_efficacy_probe.py index 2fef4e753..16c73de75 100644 --- a/tests/execution/backends/test_hook_deny_efficacy_probe.py +++ b/tests/execution/backends/test_hook_deny_efficacy_probe.py @@ -8,10 +8,12 @@ from __future__ import annotations +import base64 import copy import json import os import re +import shlex import subprocess import sys from pathlib import Path @@ -364,5 +366,19 @@ def test_shell_capture_hook_is_input_rewrite_and_excluded_from_deny_matrix( 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 + assert _OUTPUT_BUDGET_PROBE_COMMAND not in updated_command + + argv = shlex.split(updated_command.splitlines()[-1]) + runner_index = next( + index for index, value in enumerate(argv) if value.endswith("_capture_artifacts.py") + ) + assert argv[runner_index - 2] == sys.executable + assert argv[runner_index - 1] == "-I" + assert argv[runner_index + 1] == "run" + assert ( + base64.b64decode(argv[runner_index + 2], validate=True).decode() + == _OUTPUT_BUDGET_PROBE_COMMAND + ) + assert argv[runner_index + 3] == str(tmp_path) + assert re.fullmatch(r"[0-9a-f]{16}", argv[runner_index + 4]) diff --git a/tests/execution/backends/test_hook_strength_matrix.py b/tests/execution/backends/test_hook_strength_matrix.py index 7baf1cf1c..62d03e849 100644 --- a/tests/execution/backends/test_hook_strength_matrix.py +++ b/tests/execution/backends/test_hook_strength_matrix.py @@ -27,6 +27,19 @@ ) +@pytest.fixture(scope="module", autouse=True) +def _require_completed_probe_session(request: pytest.FixtureRequest) -> None: + probe_is_collected = any( + Path(str(item.path)).name == "test_hook_deny_efficacy_probe.py" + for item in request.session.items + ) + if probe_is_collected: + pytest.skip( + "strength matrix is serialized at session finish; " + "run matrix tests separately after the probe suite" + ) + + def _load_matrix() -> dict: """Load the serialized strength matrix, skipping if absent.""" if not _MATRIX_PATH.exists(): diff --git a/tests/hooks/test_capture_artifacts.py b/tests/hooks/test_capture_artifacts.py index 3eaa3b3e0..e844ec574 100644 --- a/tests/hooks/test_capture_artifacts.py +++ b/tests/hooks/test_capture_artifacts.py @@ -2,8 +2,10 @@ from __future__ import annotations +import base64 import json import os +import stat import subprocess import time from pathlib import Path @@ -87,6 +89,48 @@ def test_capture_root_rejects_symlinked_components(component: str, tmp_path: Pat anchor.close() +@pytest.mark.parametrize("missing_component", CAPTURE_PATH_COMPONENTS) +def test_capture_root_rejects_missing_components(missing_component: str, tmp_path: Path) -> None: + project = tmp_path / "project" + project.mkdir() + parent = project + for name in CAPTURE_PATH_COMPONENTS: + if name == missing_component: + break + parent = parent / name + parent.mkdir() + + anchor = open_project_anchor(str(project)) + try: + with pytest.raises(CaptureSetupError, match="missing capture path component"): + open_capture_root(anchor, create=False) + finally: + anchor.close() + + +@pytest.mark.parametrize("blocking_component", CAPTURE_PATH_COMPONENTS) +def test_capture_root_rejects_blocking_regular_file_components( + blocking_component: str, tmp_path: Path +) -> None: + project = tmp_path / "project" + project.mkdir() + parent = project + for name in CAPTURE_PATH_COMPONENTS: + candidate = parent / name + if name == blocking_component: + candidate.write_text("blocking file") + break + candidate.mkdir() + parent = candidate + + anchor = open_project_anchor(str(project)) + try: + with pytest.raises(CaptureSetupError, match="unsafe capture path component"): + open_capture_root(anchor, create=True) + finally: + anchor.close() + + def test_symlinked_policy_root_is_not_trusted(tmp_path: Path) -> None: project = tmp_path / "project" project.mkdir() @@ -331,6 +375,17 @@ def test_setup_failure_prevents_user_command(tmp_path: Path) -> None: assert not (project / "command_ran").exists() +@pytest.mark.parametrize("capture_id", ["", "0123456789abcde", "0123456789abcdeg"]) +def test_reject_mode_validates_capture_id( + capture_id: str, + capfd: pytest.CaptureFixture[str], +) -> None: + assert capture_artifacts._main(["reject", capture_id]) == 1 + captured = capfd.readouterr() + assert "invalid capture id" in captured.err + assert "capture request rejected before command execution" not in captured.err + + def test_verified_disabled_policy_runs_without_capture( tmp_path: Path, capfd: pytest.CaptureFixture[str] ) -> None: @@ -375,6 +430,114 @@ def fail_spawn(*args, **kwargs): os.fstat(fd) +def test_post_creation_identity_failure_closes_artifact_and_emits_failure( + tmp_path: Path, + monkeypatch: pytest.MonkeyPatch, + capfd: pytest.CaptureFixture[str], +) -> None: + project = tmp_path / "project" + project.mkdir() + artifact_fds: list[int] = [] + real_fstat = os.fstat + + def fail_artifact_identity(fd): + value = real_fstat(fd) + if stat.S_ISREG(value.st_mode): + artifact_fds.append(fd) + raise OSError("fault injection") + return value + + monkeypatch.setattr(capture_artifacts.os, "fstat", fail_artifact_identity) + encoded = base64.b64encode(b"printf ran > command_ran").decode() + + assert capture_artifacts._main(["run", encoded, str(project), _CAPTURE_ID]) == 1 + captured = capfd.readouterr() + assert "CAPTURE_FAILED" in captured.err + assert "SHELL_OUTPUT_CAPTURED" not in captured.out + captured.err + assert not (project / "command_ran").exists() + assert artifact_fds + for fd in artifact_fds: + with pytest.raises(OSError): + real_fstat(fd) + + +def test_capture_pipe_closes_on_success( + tmp_path: Path, + monkeypatch: pytest.MonkeyPatch, +) -> None: + project = tmp_path / "project" + project.mkdir() + processes: list[subprocess.Popen[bytes]] = [] + real_spawn = capture_artifacts._spawn_bash + + def record_spawn(*args, **kwargs): + process = real_spawn(*args, **kwargs) + processes.append(process) + return process + + monkeypatch.setattr(capture_artifacts, "_spawn_bash", record_spawn) + + assert run_capture("printf output", str(project), _CAPTURE_ID) == 0 + assert processes + assert processes[0].stdout is not None + assert processes[0].stdout.closed + + +def test_capture_stream_failure_closes_pipe_and_artifact( + tmp_path: Path, + monkeypatch: pytest.MonkeyPatch, + capfd: pytest.CaptureFixture[str], +) -> None: + project = tmp_path / "project" + project.mkdir() + observed_fds: list[int] = [] + real_create = capture_artifacts.create_capture_artifact + + class FailingStream: + closed = False + + def read(self, _size): + raise OSError("fault injection") + + def close(self): + self.closed = True + + class FailingProcess: + def __init__(self) -> None: + self.stdout = FailingStream() + self.terminated = False + + def poll(self): + return None + + def terminate(self): + self.terminated = True + + def wait(self): + return 0 + + process = FailingProcess() + + def record_artifact(root, capture_id): + artifact = real_create(root, capture_id) + observed_fds.append(artifact.fd) + return artifact + + monkeypatch.setattr(capture_artifacts, "create_capture_artifact", record_artifact) + monkeypatch.setattr(capture_artifacts, "_spawn_bash", lambda *_args, **_kwargs: process) + + assert run_capture("printf output", str(project), _CAPTURE_ID) == 0 + captured = capfd.readouterr() + assert "CAPTURE_FAILED" in captured.err + assert "SHELL_OUTPUT_CAPTURED" not in captured.out + captured.err + assert process.terminated + assert process.stdout.closed + assert observed_fds + for fd in observed_fds: + with pytest.raises(OSError): + os.fstat(fd) + + def test_artifact_write_failure_emits_failure_marker( tmp_path: Path, monkeypatch: pytest.MonkeyPatch, diff --git a/tests/hooks/test_shell_capture_conformance.py b/tests/hooks/test_shell_capture_conformance.py index e5eea63a1..aabb19205 100644 --- a/tests/hooks/test_shell_capture_conformance.py +++ b/tests/hooks/test_shell_capture_conformance.py @@ -9,6 +9,7 @@ from __future__ import annotations +import hashlib import io import json import os @@ -159,7 +160,8 @@ def test_capture_conformance(label: str, command: str, tmp_path: Path) -> None: wrapped = _run_wrapped(command, tmp_path) - assert wrapped.returncode == raw.returncode, ( + expected_wrapped_returncode = 128 + (-raw.returncode) if raw.returncode < 0 else raw.returncode + assert wrapped.returncode == expected_wrapped_returncode, ( f"[{label}] exit code mismatch: raw={raw.returncode} wrapped={wrapped.returncode}\n" f"raw stderr={raw.stderr!r}\nwrapped stderr={wrapped.stderr!r}" ) @@ -419,10 +421,15 @@ def test_capture_directory_replacement_uses_open_fds_and_hides_path( assert completed.returncode == 0 assert b"-> unavailable " in completed.stdout assert b"SHELL_OUTPUT_CAPTURED" in completed.stdout + expected = b"0123456789abcdef" + assert f"full output {len(expected)} bytes".encode() in completed.stdout + assert f"sha256={hashlib.sha256(expected).hexdigest()}".encode() in completed.stdout + assert completed.stdout.startswith(expected[:5]) + assert completed.stdout.endswith(expected[-3:]) displaced = project / ".autoskillit" / "temp" / "shell_capture-original" artifacts = sorted(displaced.glob("shell_*.log")) assert len(artifacts) == 1 - assert artifacts[0].read_bytes() == b"0123456789abcdef" + assert artifacts[0].read_bytes() == expected assert not list(external.iterdir()) @@ -461,7 +468,12 @@ def test_capture_artifact_replacement_uses_open_fd_and_hides_path( assert completed.returncode == 0 assert b"-> unavailable " in completed.stdout - assert displaced.read_bytes() == b"fedcba9876543210" + expected = b"fedcba9876543210" + assert f"full output {len(expected)} bytes".encode() in completed.stdout + assert f"sha256={hashlib.sha256(expected).hexdigest()}".encode() in completed.stdout + assert completed.stdout.startswith(expected[:5]) + assert completed.stdout.endswith(expected[-3:]) + assert displaced.read_bytes() == expected assert external.read_bytes() == b"must-survive" assert artifact.is_symlink() diff --git a/tests/hooks/test_shell_capture_hook.py b/tests/hooks/test_shell_capture_hook.py index db3c085a3..e02fb58ed 100644 --- a/tests/hooks/test_shell_capture_hook.py +++ b/tests/hooks/test_shell_capture_hook.py @@ -8,6 +8,7 @@ import re import shlex import subprocess +import sys from contextlib import redirect_stdout from pathlib import Path @@ -119,6 +120,16 @@ def test_verified_disabled_policy_is_runner_owned(monkeypatch, tmp_path): event = _build_event("echo hello", cwd=str(tmp_path)) output = _run_hook(event, monkeypatch, env_backend="codex") command = _updated_command(output) + argv = _runner_argv(command) + + assert argv[0] == sys.executable + assert argv[1] == "-I" + assert Path(argv[2]).name == "_capture_artifacts.py" + assert argv[3] == "run" + assert base64.b64decode(argv[4], validate=True).decode() == "echo hello" + assert argv[5] == str(tmp_path) + assert re.fullmatch(r"[0-9a-f]{16}", argv[6]) + completed = subprocess.run( ["bash", "-c", command], cwd=tmp_path, @@ -161,6 +172,24 @@ def test_invalid_command_transport_builds_nonexecuting_rejection(command): assert command not in harness +def test_arg_max_exhaustion_builds_nonexecuting_rejection( + monkeypatch: pytest.MonkeyPatch, +) -> None: + import autoskillit.hooks.shell_capture_hook as shell_capture_hook + + monkeypatch.setattr(shell_capture_hook.os, "sysconf", lambda _name: 1) + + harness = shell_capture_hook._build_harness( + "printf must-not-run", + "/abs/project", + "0123456789abcdef", + ) + argv = _runner_argv(harness) + + assert argv[3] == "reject" + assert "printf must-not-run" not in harness + + def test_marker_provenance_is_emitted_by_runner(monkeypatch, tmp_path): config_dir = tmp_path / ".autoskillit" / "temp" config_dir.mkdir(parents=True) From 4e82f5379d77a1453795a76b008f59ff2db514c9 Mon Sep 17 00:00:00 2001 From: Trecek Date: Thu, 23 Jul 2026 07:40:53 -0700 Subject: [PATCH 04/32] fix: harden shell capture runtime failures --- src/autoskillit/hooks/_capture_artifacts.py | 144 +++++++++++----- tests/hooks/test_capture_artifacts.py | 176 ++++++++++++++++++++ 2 files changed, 279 insertions(+), 41 deletions(-) diff --git a/src/autoskillit/hooks/_capture_artifacts.py b/src/autoskillit/hooks/_capture_artifacts.py index ebfe4bb9e..8ae4829bd 100644 --- a/src/autoskillit/hooks/_capture_artifacts.py +++ b/src/autoskillit/hooks/_capture_artifacts.py @@ -14,7 +14,6 @@ import json import os import re -import shutil import stat import subprocess import sys @@ -69,6 +68,18 @@ _MAX_COMMAND_BYTES = 64 * 1024 _MAX_ENCODED_COMMAND_BYTES = ((_MAX_COMMAND_BYTES + 2) // 3) * 4 _DRAIN_CHUNK_BYTES = 64 * 1024 +_CAPTURE_FAILURE_RETURN_CODE = 1 +_TRUSTED_BASH_CANDIDATES = ("/bin/bash", "/usr/bin/bash") +_EXECUTABLE_MODE_BITS = stat.S_IXUSR | stat.S_IXGRP | stat.S_IXOTH +_UNTRUSTED_WRITE_BITS = stat.S_IWGRP | stat.S_IWOTH +_CAPTURE_RUNTIME_ERRORS = ( + OSError, + subprocess.SubprocessError, + RuntimeError, + TypeError, + UnicodeError, + ValueError, +) _DIRECTORY_FLAGS = ( os.O_RDONLY @@ -553,36 +564,79 @@ def _emit_failure(detail: str) -> None: sys.stderr.write(f"{prefix} {safe_detail}]\n") +def _capture_failure_return(detail: str, returncode: int | None) -> int: + try: + _emit_failure(detail) + except _CAPTURE_RUNTIME_ERRORS: + return _CAPTURE_FAILURE_RETURN_CODE + return returncode if returncode is not None else _CAPTURE_FAILURE_RETURN_CODE + + def _emit_capture( result: _DrainResult, artifact_path: str | None, inline_bytes: int, ) -> None: if result.total_bytes <= inline_bytes: - sys.stdout.buffer.write(result.inline) - sys.stdout.buffer.flush() - return - - prefix = render_capture_marker(_capture_event("SHELL_OUTPUT_CAPTURED", "input rewrite")) - path = artifact_path if artifact_path is not None else "unavailable" - marker = ( - f"\n{prefix} full output {result.total_bytes} bytes -> {path} " - f"sha256={result.sha256} complete=true]\n" - ).encode() - sys.stdout.buffer.write(result.head) - sys.stdout.buffer.write(marker) - sys.stdout.buffer.write(result.tail) + payload = result.inline + else: + prefix = render_capture_marker(_capture_event("SHELL_OUTPUT_CAPTURED", "input rewrite")) + path = artifact_path if artifact_path is not None else "unavailable" + marker = ( + f"\n{prefix} full output {result.total_bytes} bytes -> {path} " + f"sha256={result.sha256} complete=true]\n" + ).encode() + payload = result.head + marker + result.tail + sys.stdout.buffer.write(payload) sys.stdout.buffer.flush() def _resolve_bash() -> str: - bash_path = shutil.which("bash") - if bash_path is None: - raise CaptureSetupError("bash executable unavailable") - resolved = os.path.realpath(bash_path) - if not os.path.isabs(resolved): - raise CaptureSetupError("bash executable path is not absolute") - return resolved + for candidate in _TRUSTED_BASH_CANDIDATES: + if not os.path.isabs(candidate): + continue + try: + fd = os.open(candidate, _READ_FLAGS) + except OSError: + continue + try: + value = os.fstat(fd) + if ( + stat.S_ISREG(value.st_mode) + and value.st_uid == 0 + and value.st_mode & _EXECUTABLE_MODE_BITS + and not value.st_mode & _UNTRUSTED_WRITE_BITS + ): + return candidate + except OSError: + pass + finally: + os.close(fd) + raise CaptureSetupError("trusted bash executable unavailable") + + +def _settle_failed_capture(process: subprocess.Popen[bytes]) -> int | None: + if process.stdout is not None: + try: + process.stdout.close() + except _CAPTURE_RUNTIME_ERRORS: + pass + try: + running = process.poll() is None + except _CAPTURE_RUNTIME_ERRORS: + running = True + if running: + try: + process.terminate() + except _CAPTURE_RUNTIME_ERRORS: + try: + process.kill() + except _CAPTURE_RUNTIME_ERRORS: + pass + try: + return _normalized_returncode(process.wait()) + except _CAPTURE_RUNTIME_ERRORS: + return None def run_capture(command: str, cwd: str, capture_id: str) -> int: @@ -613,35 +667,43 @@ def run_capture(command: str, cwd: str, capture_id: str) -> int: root = open_capture_root(anchor, create=True) artifact = create_capture_artifact(root, capture_id) process = _spawn_bash(anchor, bash_path, command, capture_output=True) + returncode: int | None = None + failure_stage = "capture pipe drain" try: result = _drain_capture(process, artifact, policy.inline_bytes) - except (CaptureSetupError, OSError): - if process.stdout is not None: - process.stdout.close() - if process.poll() is None: - process.terminate() + failure_stage = "capture process wait" returncode = _normalized_returncode(process.wait()) - _emit_failure("capture pipe drain failed") - return returncode - returncode = _normalized_returncode(process.wait()) - if result.write_error is not None: - _emit_failure("capture artifact write failed") - return returncode - try: + if result.write_error is not None: + return _capture_failure_return("capture artifact write failed", returncode) + failure_stage = "capture marker verification" artifact_path = current_artifact_path_if_bound(anchor, root, artifact) - except OSError: - _emit_failure("capture marker verification failed") + failure_stage = "capture replay emission" + _emit_capture(result, artifact_path, policy.inline_bytes) return returncode - _emit_capture(result, artifact_path, policy.inline_bytes) - return returncode + except _CAPTURE_RUNTIME_ERRORS: + if returncode is None: + returncode = _settle_failed_capture(process) + return _capture_failure_return(f"{failure_stage} failed", returncode) finally: if process is not None and process.stdout is not None: - process.stdout.close() + try: + process.stdout.close() + except _CAPTURE_RUNTIME_ERRORS: + pass if artifact is not None: - artifact.close() + try: + artifact.close() + except _CAPTURE_RUNTIME_ERRORS: + pass if root is not None: - root.close() - anchor.close() + try: + root.close() + except _CAPTURE_RUNTIME_ERRORS: + pass + try: + anchor.close() + except _CAPTURE_RUNTIME_ERRORS: + pass def _open_stale_candidate( diff --git a/tests/hooks/test_capture_artifacts.py b/tests/hooks/test_capture_artifacts.py index e844ec574..3a2a40914 100644 --- a/tests/hooks/test_capture_artifacts.py +++ b/tests/hooks/test_capture_artifacts.py @@ -45,6 +45,53 @@ def _open_authority(project: Path): return anchor, root +class _ReadableStream: + def __init__(self, value: bytes) -> None: + self._value = value + self.closed = False + + def read(self, _size: int) -> bytes: + value, self._value = self._value, b"" + return value + + def close(self) -> None: + self.closed = True + + +class _FakeCaptureProcess: + def __init__(self, value: bytes) -> None: + self.stdout = _ReadableStream(value) + self.terminated = False + self.killed = False + self.wait_calls = 0 + + def poll(self) -> None: + return None + + def terminate(self) -> None: + self.terminated = True + + def kill(self) -> None: + self.killed = True + + def wait(self) -> int: + self.wait_calls += 1 + return 0 + + +def _record_artifact_fds(monkeypatch: pytest.MonkeyPatch) -> list[int]: + observed_fds: list[int] = [] + real_create = capture_artifacts.create_capture_artifact + + def record_artifact(root, capture_id): + artifact = real_create(root, capture_id) + observed_fds.append(artifact.fd) + return artifact + + monkeypatch.setattr(capture_artifacts, "create_capture_artifact", record_artifact) + return observed_fds + + def test_project_anchor_accepts_symlink_cwd(tmp_path: Path) -> None: project = tmp_path / "project" project.mkdir() @@ -483,6 +530,52 @@ def record_spawn(*args, **kwargs): assert processes[0].stdout.closed +def test_bash_resolution_ignores_ambient_path( + tmp_path: Path, + monkeypatch: pytest.MonkeyPatch, + capfd: pytest.CaptureFixture[str], +) -> None: + project = tmp_path / "project" + project.mkdir() + attacker_bin = tmp_path / "attacker-bin" + attacker_bin.mkdir() + fake_bash = attacker_bin / "bash" + fake_bash.write_text("#!/bin/sh\nprintf attacker-controlled\nexit 99\n") + fake_bash.chmod(0o755) + monkeypatch.setenv("PATH", str(attacker_bin)) + + assert run_capture("printf trusted-bash", str(project), _CAPTURE_ID) == 0 + captured = capfd.readouterr() + assert captured.out == "trusted-bash" + assert "attacker-controlled" not in captured.out + captured.err + + +@pytest.mark.parametrize("candidate_kind", ["symlink", "world-writable"]) +def test_bash_resolution_rejects_untrusted_explicit_candidate( + candidate_kind: str, + tmp_path: Path, + monkeypatch: pytest.MonkeyPatch, +) -> None: + target = tmp_path / "candidate-target" + target.write_text("#!/bin/sh\nexit 0\n") + target.chmod(0o755) + candidate = target + if candidate_kind == "symlink": + candidate = tmp_path / "bash" + candidate.symlink_to(target) + else: + target.chmod(0o777) + + monkeypatch.setattr( + capture_artifacts, + "_TRUSTED_BASH_CANDIDATES", + (str(candidate),), + ) + + with pytest.raises(CaptureSetupError, match="trusted bash executable unavailable"): + capture_artifacts._resolve_bash() + + def test_capture_stream_failure_closes_pipe_and_artifact( tmp_path: Path, monkeypatch: pytest.MonkeyPatch, @@ -538,6 +631,89 @@ def record_artifact(root, capture_id): os.fstat(fd) +def test_digest_failure_emits_failure_and_closes_runtime_resources( + tmp_path: Path, + monkeypatch: pytest.MonkeyPatch, + capfd: pytest.CaptureFixture[str], +) -> None: + project = tmp_path / "project" + project.mkdir() + observed_fds = _record_artifact_fds(monkeypatch) + process = _FakeCaptureProcess(b"captured-output") + + class BrokenDigest: + def update(self, _chunk: bytes) -> None: + raise RuntimeError("fault injection") + + def hexdigest(self) -> str: + raise AssertionError("digest failure must stop before finalization") + + monkeypatch.setattr(capture_artifacts, "_spawn_bash", lambda *_args, **_kwargs: process) + monkeypatch.setattr(capture_artifacts.hashlib, "sha256", BrokenDigest) + + assert run_capture("printf output", str(project), _CAPTURE_ID) == 0 + captured = capfd.readouterr() + assert "CAPTURE_FAILED" in captured.err + assert "SHELL_OUTPUT_CAPTURED" not in captured.out + captured.err + assert process.terminated + assert process.stdout.closed + assert process.wait_calls == 1 + assert observed_fds + for fd in observed_fds: + with pytest.raises(OSError): + os.fstat(fd) + + +def test_success_marker_emission_failure_closes_resources_without_success( + tmp_path: Path, + monkeypatch: pytest.MonkeyPatch, + capfd: pytest.CaptureFixture[str], +) -> None: + project = tmp_path / "project" + project.mkdir() + observed_fds = _record_artifact_fds(monkeypatch) + process = _FakeCaptureProcess(b"captured-output") + + def fail_success_marker(*_args, **_kwargs) -> None: + raise RuntimeError("fault injection") + + monkeypatch.setattr(capture_artifacts, "_spawn_bash", lambda *_args, **_kwargs: process) + monkeypatch.setattr(capture_artifacts, "_emit_capture", fail_success_marker) + + assert run_capture("printf output", str(project), _CAPTURE_ID) == 0 + captured = capfd.readouterr() + assert "CAPTURE_FAILED" in captured.err + assert "SHELL_OUTPUT_CAPTURED" not in captured.out + captured.err + assert process.stdout.closed + assert process.wait_calls == 1 + assert _capture_dir(project).joinpath(f"shell_{_CAPTURE_ID}.log").read_bytes() == ( + b"captured-output" + ) + assert observed_fds + for fd in observed_fds: + with pytest.raises(OSError): + os.fstat(fd) + + +def test_failure_marker_emission_failure_returns_capture_failure_code( + tmp_path: Path, + monkeypatch: pytest.MonkeyPatch, +) -> None: + project = tmp_path / "project" + project.mkdir() + process = _FakeCaptureProcess(b"captured-output") + + def fail_marker(*_args, **_kwargs) -> None: + raise RuntimeError("fault injection") + + monkeypatch.setattr(capture_artifacts, "_spawn_bash", lambda *_args, **_kwargs: process) + monkeypatch.setattr(capture_artifacts, "_emit_capture", fail_marker) + monkeypatch.setattr(capture_artifacts, "_emit_failure", fail_marker) + + assert run_capture("printf output", str(project), _CAPTURE_ID) == 1 + assert process.stdout.closed + + def test_artifact_write_failure_emits_failure_marker( tmp_path: Path, monkeypatch: pytest.MonkeyPatch, From f3b5a05ef5eec9c9c91d0614036ea03dbdbf7aba Mon Sep 17 00:00:00 2001 From: Trecek Date: Thu, 23 Jul 2026 08:28:37 -0700 Subject: [PATCH 05/32] fix: address shell capture trust-anchor audit --- src/autoskillit/hooks/_capture_artifacts.py | 60 ++++- tests/arch/test_shell_capture_trust_anchor.py | 9 +- tests/hooks/test_capture_artifacts.py | 212 +++++++++++++++++- 3 files changed, 261 insertions(+), 20 deletions(-) diff --git a/src/autoskillit/hooks/_capture_artifacts.py b/src/autoskillit/hooks/_capture_artifacts.py index 8ae4829bd..4fa7ea29e 100644 --- a/src/autoskillit/hooks/_capture_artifacts.py +++ b/src/autoskillit/hooks/_capture_artifacts.py @@ -19,6 +19,7 @@ import sys import time from dataclasses import dataclass +from enum import Enum from pathlib import Path from typing import TYPE_CHECKING @@ -91,12 +92,22 @@ os.O_RDWR | os.O_CREAT | os.O_EXCL | getattr(os, "O_CLOEXEC", 0) | getattr(os, "O_NOFOLLOW", 0) ) _READ_FLAGS = os.O_RDONLY | getattr(os, "O_CLOEXEC", 0) | getattr(os, "O_NOFOLLOW", 0) +_STALE_READ_FLAGS = _READ_FLAGS | getattr(os, "O_NONBLOCK", 0) class CaptureSetupError(RuntimeError): """Raised when the descriptor-anchored capture authority cannot be established.""" +class _CleanupDeletionMode(Enum): + """Observed cleanup capability; neither stdlib mode permits identity-bound deletion.""" + + SAFE_RETENTION_WITHOUT_DIR_FD_UNLINK = "safe-retention-without-dir-fd-unlink" + SAFE_RETENTION_WITH_UNCONDITIONAL_DIR_FD_UNLINK = ( + "safe-retention-with-unconditional-dir-fd-unlink" + ) + + @dataclass(frozen=True, slots=True) class FileIdentity: device: int @@ -113,6 +124,7 @@ class ProjectAnchor: fd: int identity: FileIdentity + supplied_path: str physical_path: Path def close(self) -> None: @@ -186,15 +198,25 @@ def close(self) -> None: self.fd = -1 -def _require_capabilities() -> None: - required_dir_fd = (os.open, os.mkdir) +def _probe_cleanup_deletion_mode() -> _CleanupDeletionMode: + if os.unlink in getattr(os, "supports_dir_fd", ()): + return _CleanupDeletionMode.SAFE_RETENTION_WITH_UNCONDITIONAL_DIR_FD_UNLINK + return _CleanupDeletionMode.SAFE_RETENTION_WITHOUT_DIR_FD_UNLINK + + +def _require_capabilities() -> _CleanupDeletionMode: + required_dir_fd = (os.open, os.mkdir, os.stat) + required_flags = ("O_CLOEXEC", "O_CREAT", "O_DIRECTORY", "O_EXCL", "O_NOFOLLOW", "O_NONBLOCK") if ( - getattr(os, "O_DIRECTORY", 0) == 0 - or getattr(os, "O_NOFOLLOW", 0) == 0 + any(getattr(os, flag, 0) == 0 for flag in required_flags) or not hasattr(os, "fchdir") + or not hasattr(os, "fstat") or any(function not in os.supports_dir_fd for function in required_dir_fd) + or os.stat not in getattr(os, "supports_follow_symlinks", ()) + or os.listdir not in getattr(os, "supports_fd", ()) ): raise CaptureSetupError("required descriptor-relative filesystem primitives unavailable") + return _probe_cleanup_deletion_mode() def _identity(fd: int) -> FileIdentity: @@ -251,6 +273,7 @@ def open_project_anchor(cwd: str) -> ProjectAnchor: return ProjectAnchor( fd=fd, identity=FileIdentity.from_stat(anchor_stat), + supplied_path=cwd, physical_path=physical_path, ) except BaseException: @@ -392,9 +415,10 @@ def current_artifact_path_if_bound( opened: list[int] = [] try: try: + marker_physical_path = Path(os.path.realpath(anchor.supplied_path)) project_fd = os.open( - anchor.physical_path, - _DIRECTORY_FLAGS & ~getattr(os, "O_NOFOLLOW", 0), + marker_physical_path, + _DIRECTORY_FLAGS, ) except OSError: return None @@ -434,7 +458,7 @@ def current_artifact_path_if_bound( or current_value.st_mode & stat.S_IWOTH ): return None - return str(anchor.physical_path.joinpath(*CAPTURE_PATH_COMPONENTS, artifact.name)) + return str(marker_physical_path.joinpath(*CAPTURE_PATH_COMPONENTS, artifact.name)) finally: for fd in reversed(opened): os.close(fd) @@ -478,8 +502,7 @@ def _spawn_bash( if restore_error is not None: if process is not None: - process.terminate() - process.wait() + _settle_failed_capture(process) raise CaptureSetupError("cannot restore runner cwd") from restore_error if process is None: raise CaptureSetupError("capture shell did not start") @@ -715,7 +738,18 @@ def _open_stale_candidate( if not _CAPTURE_FILENAME_RE.fullmatch(name): return None try: - fd = os.open(name, _READ_FLAGS, dir_fd=root.fd) + observed = os.stat(name, dir_fd=root.fd, follow_symlinks=False) + except OSError: + return None + if ( + not stat.S_ISREG(observed.st_mode) + or observed.st_nlink != 1 + or observed.st_mode & stat.S_IWOTH + or observed.st_mtime > mtime_threshold + ): + return None + try: + fd = os.open(name, _STALE_READ_FLAGS, dir_fd=root.fd) except OSError: return None try: @@ -724,7 +758,8 @@ def _open_stale_candidate( os.close(fd) return None if ( - not stat.S_ISREG(value.st_mode) + FileIdentity.from_stat(value) != FileIdentity.from_stat(observed) + or not stat.S_ISREG(value.st_mode) or value.st_nlink != 1 or value.st_mode & stat.S_IWOTH or value.st_mtime > mtime_threshold @@ -756,6 +791,7 @@ def sweep_stale_captures( anchor: ProjectAnchor | None = None root: CaptureRoot | None = None try: + _cleanup_mode = _require_capabilities() anchor = open_project_anchor(os.fspath(project_root)) root = open_capture_root(anchor, create=False) threshold = time.time() - max_age_seconds @@ -766,7 +802,7 @@ def sweep_stale_captures( for name in names: candidate = _open_stale_candidate(root, name, mtime_threshold=threshold) if candidate is not None: - # Retention is the safe disposition without expected-inode unlink. + # Both probed stdlib modes retain: neither can bind unlink to this fd identity. candidate.close() return 0 except (CaptureSetupError, OSError): diff --git a/tests/arch/test_shell_capture_trust_anchor.py b/tests/arch/test_shell_capture_trust_anchor.py index a96d24d4d..67290d5e8 100644 --- a/tests/arch/test_shell_capture_trust_anchor.py +++ b/tests/arch/test_shell_capture_trust_anchor.py @@ -32,6 +32,11 @@ "reason": "clone cleanup still uses pathname-based unlink/rmtree", "tracking_issue": "#4319", }, + "scripts/recipe/create_worktree.sh": { + "owner": "workspace worktree lifecycle", + "reason": "recipe-side worktree sidecars are still written and removed by pathname", + "tracking_issue": "#4319", + }, } @@ -71,9 +76,11 @@ def test_project_temp_cleanup_debt_is_narrow_and_owned() -> None: "workspace/worktree.py", "workspace/clone_registry.py", "workspace/clone.py", + "scripts/recipe/create_worktree.sh", } for relative, debt in _PROJECT_TEMP_CLEANUP_DEBT.items(): - assert (_SRC / relative).is_file() + path = _REPO_ROOT / relative if relative.startswith("scripts/") else _SRC / relative + assert path.is_file() assert debt["owner"] assert debt["reason"] assert debt["tracking_issue"] == "#4319" diff --git a/tests/hooks/test_capture_artifacts.py b/tests/hooks/test_capture_artifacts.py index 3a2a40914..93470fbdc 100644 --- a/tests/hooks/test_capture_artifacts.py +++ b/tests/hooks/test_capture_artifacts.py @@ -7,6 +7,7 @@ import os import stat import subprocess +import sys import time from pathlib import Path @@ -112,6 +113,79 @@ def test_project_anchor_accepts_symlink_cwd(tmp_path: Path) -> None: anchor.close() +def test_symlinked_cwd_is_opened_before_path_derivation_and_descendants( + tmp_path: Path, + monkeypatch: pytest.MonkeyPatch, +) -> None: + project = tmp_path / "project" + project.mkdir() + supplied_cwd = tmp_path / "project-link" + supplied_cwd.symlink_to(project, target_is_directory=True) + events: list[tuple[str, int | None]] = [] + real_open = capture_artifacts.os.open + real_realpath = capture_artifacts.os.path.realpath + + def track_open(path, flags, mode=0o777, *, dir_fd=None): + fd = real_open(path, flags, mode, dir_fd=dir_fd) + if os.fspath(path) == str(supplied_cwd) and dir_fd is None: + events.append(("project", None)) + elif path in CAPTURE_PATH_COMPONENTS: + events.append((os.fspath(path), dir_fd)) + return fd + + def track_realpath(path): + events.append(("derive", None)) + return real_realpath(path) + + monkeypatch.setattr(capture_artifacts, "_require_capabilities", lambda: None) + monkeypatch.setattr(capture_artifacts.os, "open", track_open) + monkeypatch.setattr(capture_artifacts.os.path, "realpath", track_realpath) + + anchor, root = _open_authority(supplied_cwd) + try: + assert events[:2] == [("project", None), ("derive", None)] + assert events[2:] == [ + (CAPTURE_PATH_COMPONENTS[0], anchor.fd), + (CAPTURE_PATH_COMPONENTS[1], root.autoskillit_fd), + (CAPTURE_PATH_COMPONENTS[2], root.temp_fd), + ] + finally: + root.close() + anchor.close() + + +def test_capability_probe_requires_descriptor_relative_stat( + monkeypatch: pytest.MonkeyPatch, +) -> None: + supported = set(capture_artifacts.os.supports_dir_fd) + supported.discard(capture_artifacts.os.stat) + monkeypatch.setattr(capture_artifacts.os, "supports_dir_fd", supported) + + with pytest.raises(CaptureSetupError, match="filesystem primitives unavailable"): + capture_artifacts._require_capabilities() + + +def test_capability_probe_requires_exclusive_creation_flag( + monkeypatch: pytest.MonkeyPatch, +) -> None: + monkeypatch.setattr(capture_artifacts.os, "O_EXCL", 0) + + with pytest.raises(CaptureSetupError, match="filesystem primitives unavailable"): + capture_artifacts._require_capabilities() + + +def test_cleanup_capability_probe_selects_safe_retention_without_dir_fd_unlink( + monkeypatch: pytest.MonkeyPatch, +) -> None: + supported = set(capture_artifacts.os.supports_dir_fd) + supported.discard(capture_artifacts.os.unlink) + monkeypatch.setattr(capture_artifacts.os, "supports_dir_fd", supported) + + mode = capture_artifacts._probe_cleanup_deletion_mode() + + assert mode is capture_artifacts._CleanupDeletionMode.SAFE_RETENTION_WITHOUT_DIR_FD_UNLINK + + @pytest.mark.parametrize("component", CAPTURE_PATH_COMPONENTS) def test_capture_root_rejects_symlinked_components(component: str, tmp_path: Path) -> None: project = tmp_path / "project" @@ -288,6 +362,26 @@ def test_marker_path_requires_current_directory_binding(tmp_path: Path) -> None: anchor.close() +def test_marker_path_rederives_symlinked_project_path(tmp_path: Path) -> None: + project = tmp_path / "project" + project.mkdir() + replacement = tmp_path / "replacement" + replacement.mkdir() + supplied_cwd = tmp_path / "project-link" + supplied_cwd.symlink_to(project, target_is_directory=True) + anchor, root = _open_authority(supplied_cwd) + artifact = create_capture_artifact(root, _CAPTURE_ID) + try: + supplied_cwd.unlink() + supplied_cwd.symlink_to(replacement, target_is_directory=True) + + assert current_artifact_path_if_bound(anchor, root, artifact) is None + finally: + artifact.close() + root.close() + anchor.close() + + def test_stale_capture_is_safely_retained_without_identity_unlink(tmp_path: Path) -> None: project = tmp_path / "project" capture_dir = _capture_dir(project) @@ -301,6 +395,33 @@ def test_stale_capture_is_safely_retained_without_identity_unlink(tmp_path: Path assert stale.read_bytes() == b"retained" +def test_name_matching_fifo_does_not_block_stale_cleanup(tmp_path: Path) -> None: + project = tmp_path / "project" + capture_dir = _capture_dir(project) + capture_dir.mkdir(parents=True) + fifo = capture_dir / f"shell_{_CAPTURE_ID}.log" + try: + os.mkfifo(fifo) + except OSError as exc: + pytest.skip(f"FIFO unavailable: {exc}") + code = ( + "import sys\n" + "from autoskillit.hooks._capture_artifacts import sweep_stale_captures\n" + "raise SystemExit(sweep_stale_captures(sys.argv[1], max_age_seconds=0))\n" + ) + + completed = subprocess.run( + [sys.executable, "-c", code, str(project)], + capture_output=True, + text=True, + timeout=2, + check=False, + ) + + assert completed.returncode == 0, completed.stderr + assert fifo.is_fifo() + + def test_cleanup_candidate_classification_rejects_unsafe_entries(tmp_path: Path) -> None: project = tmp_path / "project" project.mkdir() @@ -411,14 +532,20 @@ def swap_after_validation(root, name, *, mtime_threshold): assert displaced.read_bytes() == b"validated" -def test_setup_failure_prevents_user_command(tmp_path: Path) -> None: +def test_setup_failure_prevents_user_command_and_emits_failure_marker( + tmp_path: Path, + capfd: pytest.CaptureFixture[str], +) -> None: project = tmp_path / "project" temp_dir = project.joinpath(*CAPTURE_PATH_COMPONENTS[:2]) temp_dir.mkdir(parents=True) (temp_dir / CAPTURE_PATH_COMPONENTS[2]).write_text("blocking file") + encoded = base64.b64encode(b"printf ran > command_ran").decode() - with pytest.raises(CaptureSetupError): - run_capture("printf ran > command_ran", str(project), _CAPTURE_ID) + assert capture_artifacts._main(["run", encoded, str(project), _CAPTURE_ID]) == 1 + captured = capfd.readouterr() + assert "CAPTURE_FAILED" in captured.err + assert "SHELL_OUTPUT_CAPTURED" not in captured.out + captured.err assert not (project / "command_ran").exists() @@ -450,13 +577,27 @@ def test_verified_disabled_policy_runs_without_capture( def test_spawn_failure_closes_created_artifact_fd( - tmp_path: Path, monkeypatch: pytest.MonkeyPatch + tmp_path: Path, + monkeypatch: pytest.MonkeyPatch, + capfd: pytest.CaptureFixture[str], ) -> None: project = tmp_path / "project" project.mkdir() observed_fds: list[int] = [] + real_open_anchor = capture_artifacts.open_project_anchor + real_open_root = capture_artifacts.open_capture_root real_create = capture_artifacts.create_capture_artifact + def record_anchor(cwd): + anchor = real_open_anchor(cwd) + observed_fds.append(anchor.fd) + return anchor + + def record_root(anchor, *, create): + root = real_open_root(anchor, create=create) + observed_fds.extend((root.autoskillit_fd, root.temp_fd, root.fd)) + return root + def record_artifact(root, capture_id): artifact = real_create(root, capture_id) observed_fds.append(artifact.fd) @@ -465,18 +606,75 @@ def record_artifact(root, capture_id): def fail_spawn(*args, **kwargs): raise OSError("fault injection") + monkeypatch.setattr(capture_artifacts, "open_project_anchor", record_anchor) + monkeypatch.setattr(capture_artifacts, "open_capture_root", record_root) monkeypatch.setattr(capture_artifacts, "create_capture_artifact", record_artifact) monkeypatch.setattr(subprocess, "Popen", fail_spawn) + encoded = base64.b64encode(b"printf ran > command_ran").decode() - with pytest.raises(CaptureSetupError): - run_capture("printf never", str(project), _CAPTURE_ID) + assert capture_artifacts._main(["run", encoded, str(project), _CAPTURE_ID]) == 1 - assert observed_fds + captured = capfd.readouterr() + assert "CAPTURE_FAILED" in captured.err + assert "SHELL_OUTPUT_CAPTURED" not in captured.out + captured.err + assert not (project / "command_ran").exists() + assert len(observed_fds) == 5 for fd in observed_fds: with pytest.raises(OSError): os.fstat(fd) +def test_restore_failure_closes_pipe_and_original_cwd_fd( + tmp_path: Path, + monkeypatch: pytest.MonkeyPatch, +) -> None: + project = tmp_path / "project" + project.mkdir() + anchor = open_project_anchor(str(project)) + process = _FakeCaptureProcess(b"") + runner_cwd_fd = os.open(".", os.O_RDONLY | os.O_DIRECTORY) + original_cwd_fds: list[int] = [] + real_open = capture_artifacts.os.open + real_fchdir = capture_artifacts.os.fchdir + fchdir_calls = 0 + + def record_open(path, flags, mode=0o777, *, dir_fd=None): + fd = real_open(path, flags, mode, dir_fd=dir_fd) + if path == "." and dir_fd is None: + original_cwd_fds.append(fd) + return fd + + def fail_restore(fd): + nonlocal fchdir_calls + fchdir_calls += 1 + if fchdir_calls == 2: + raise OSError("fault injection") + real_fchdir(fd) + + monkeypatch.setattr(capture_artifacts.os, "open", record_open) + monkeypatch.setattr(capture_artifacts.os, "fchdir", fail_restore) + monkeypatch.setattr(capture_artifacts.subprocess, "Popen", lambda *_args, **_kwargs: process) + + try: + with pytest.raises(CaptureSetupError, match="cannot restore runner cwd"): + capture_artifacts._spawn_bash( + anchor, + "/bin/bash", + "printf never", + capture_output=True, + ) + assert process.stdout.closed + assert process.terminated + assert process.wait_calls == 1 + assert len(original_cwd_fds) == 1 + with pytest.raises(OSError): + os.fstat(original_cwd_fds[0]) + finally: + real_fchdir(runner_cwd_fd) + os.close(runner_cwd_fd) + anchor.close() + + def test_post_creation_identity_failure_closes_artifact_and_emits_failure( tmp_path: Path, monkeypatch: pytest.MonkeyPatch, From 0045e95679bcb63de0e83026e21a1530a7eba9e3 Mon Sep 17 00:00:00 2001 From: Trecek Date: Thu, 23 Jul 2026 09:20:13 -0700 Subject: [PATCH 06/32] fix: close remaining shell capture audit gaps --- src/autoskillit/hooks/_capture_artifacts.py | 82 ++++-- tests/arch/test_shell_capture_trust_anchor.py | 93 ++++++- tests/hooks/test_capture_artifacts.py | 260 ++++++++++++++++-- 3 files changed, 391 insertions(+), 44 deletions(-) diff --git a/src/autoskillit/hooks/_capture_artifacts.py b/src/autoskillit/hooks/_capture_artifacts.py index 4fa7ea29e..124609055 100644 --- a/src/autoskillit/hooks/_capture_artifacts.py +++ b/src/autoskillit/hooks/_capture_artifacts.py @@ -330,6 +330,26 @@ def create_capture_artifact(root: CaptureRoot, capture_id: str) -> CaptureArtifa raise +def _duplicate_artifact_writer(artifact: CaptureArtifact) -> int: + """Duplicate the artifact fd for the drain stage without transferring it to Bash.""" + + writer_fd = -1 + try: + writer_fd = os.dup(artifact.fd) + if not _same_identity(writer_fd, artifact.identity): + raise CaptureSetupError("duplicated capture artifact identity changed") + return writer_fd + except (CaptureSetupError, OSError) as exc: + if writer_fd >= 0: + try: + os.close(writer_fd) + except OSError: + pass + if isinstance(exc, CaptureSetupError): + raise + raise CaptureSetupError("cannot duplicate capture artifact fd") from exc + + def _read_bounded_file_at(directory_fd: int, name: str) -> dict: try: fd = os.open(name, _READ_FLAGS, dir_fd=directory_fd) @@ -369,16 +389,15 @@ def read_capture_policy(anchor: ProjectAnchor) -> CapturePolicy: autoskillit_fd = -1 temp_fd = -1 try: - autoskillit_fd = _open_directory_component( - anchor.fd, CAPTURE_PATH_COMPONENTS[0], create=False - ) - temp_fd = _open_directory_component( - autoskillit_fd, CAPTURE_PATH_COMPONENTS[1], create=False - ) - except CaptureSetupError: - return CapturePolicy() - - try: + try: + autoskillit_fd = _open_directory_component( + anchor.fd, CAPTURE_PATH_COMPONENTS[0], create=False + ) + temp_fd = _open_directory_component( + autoskillit_fd, CAPTURE_PATH_COMPONENTS[1], create=False + ) + except CaptureSetupError: + return CapturePolicy() base = _read_bounded_file_at(temp_fd, HOOK_CONFIG_FILENAME) overlay = _read_bounded_file_at(temp_fd, HOOK_CONFIG_OVERLAY_FILENAME) merged = merge_hook_configs(base, overlay) @@ -390,8 +409,12 @@ def read_capture_policy(anchor: ProjectAnchor) -> CapturePolicy: inline_bytes=_policy_inline_bytes(section.get("shell_max_inline_bytes")), ) finally: - os.close(temp_fd) - os.close(autoskillit_fd) + try: + if temp_fd >= 0: + os.close(temp_fd) + finally: + if autoskillit_fd >= 0: + os.close(autoskillit_fd) def _open_and_match_directory(parent_fd: int, name: str, expected: FileIdentity) -> int: @@ -399,7 +422,12 @@ def _open_and_match_directory(parent_fd: int, name: str, expected: FileIdentity) fd = _open_directory_component(parent_fd, name, create=False) except CaptureSetupError: return -1 - if not _same_identity(fd, expected): + try: + matches = _same_identity(fd, expected) + except BaseException: + os.close(fd) + raise + if not matches: os.close(fd) return -1 return fd @@ -520,9 +548,11 @@ def _write_all(fd: int, data: bytes) -> None: def _drain_capture( process: subprocess.Popen[bytes], - artifact: CaptureArtifact, + artifact_writer_fd: int, inline_bytes: int, ) -> _DrainResult: + """Read the combined subprocess pipe and persist bounded replay metadata.""" + stream = process.stdout if stream is None: raise CaptureSetupError("capture pipe unavailable") @@ -544,7 +574,7 @@ def _drain_capture( digest.update(chunk) if write_error is None: try: - _write_all(artifact.fd, chunk) + _write_all(artifact_writer_fd, chunk) except OSError as exc: write_error = exc if len(inline) <= inline_bytes: @@ -679,6 +709,7 @@ def run_capture(command: str, cwd: str, capture_id: str) -> int: anchor = open_project_anchor(cwd) root: CaptureRoot | None = None artifact: CaptureArtifact | None = None + artifact_writer_fd = -1 process: subprocess.Popen[bytes] | None = None try: policy = read_capture_policy(anchor) @@ -689,11 +720,12 @@ def run_capture(command: str, cwd: str, capture_id: str) -> int: root = open_capture_root(anchor, create=True) artifact = create_capture_artifact(root, capture_id) + artifact_writer_fd = _duplicate_artifact_writer(artifact) process = _spawn_bash(anchor, bash_path, command, capture_output=True) returncode: int | None = None - failure_stage = "capture pipe drain" + failure_stage = "capture readback" try: - result = _drain_capture(process, artifact, policy.inline_bytes) + result = _drain_capture(process, artifact_writer_fd, policy.inline_bytes) failure_stage = "capture process wait" returncode = _normalized_returncode(process.wait()) if result.write_error is not None: @@ -713,6 +745,11 @@ def run_capture(command: str, cwd: str, capture_id: str) -> int: process.stdout.close() except _CAPTURE_RUNTIME_ERRORS: pass + if artifact_writer_fd >= 0: + try: + os.close(artifact_writer_fd) + except _CAPTURE_RUNTIME_ERRORS: + pass if artifact is not None: try: artifact.close() @@ -800,10 +837,13 @@ def sweep_stale_captures( except OSError: return 0 for name in names: - candidate = _open_stale_candidate(root, name, mtime_threshold=threshold) - if candidate is not None: - # Both probed stdlib modes retain: neither can bind unlink to this fd identity. - candidate.close() + try: + candidate = _open_stale_candidate(root, name, mtime_threshold=threshold) + if candidate is not None: + # Neither probed stdlib mode can bind unlink to this fd identity. + candidate.close() + except (CaptureSetupError, OSError): + continue return 0 except (CaptureSetupError, OSError): return 0 diff --git a/tests/arch/test_shell_capture_trust_anchor.py b/tests/arch/test_shell_capture_trust_anchor.py index 67290d5e8..71060c163 100644 --- a/tests/arch/test_shell_capture_trust_anchor.py +++ b/tests/arch/test_shell_capture_trust_anchor.py @@ -2,6 +2,8 @@ from __future__ import annotations +import ast +import re from pathlib import Path import pytest @@ -10,6 +12,13 @@ _REPO_ROOT = Path(__file__).resolve().parents[2] _SRC = _REPO_ROOT / "src" / "autoskillit" +_REDIRECTION_RE = re.compile( + r"(?:^|[\s;|&])(?:\d*>>?|&>>?)\s*(?P\"[^\"]+\"|'[^']+'|[^\s;|&]+)" +) +_CAPTURE_PATH_TERM_RE = re.compile( + r"capture(?:_root|_path|_file)?|artifact|shell_(?:capture|output)|__as_f", + re.IGNORECASE, +) _PROJECT_TEMP_CLEANUP_DEBT = { "core/io.py": { @@ -44,6 +53,60 @@ def _source(relative: str) -> str: return (_SRC / relative).read_text(encoding="utf-8") +def _render_string_node(node: ast.Constant | ast.JoinedStr) -> str: + if isinstance(node, ast.Constant): + return node.value if isinstance(node.value, str) else "" + parts: list[str] = [] + for value in node.values: + if isinstance(value, ast.Constant) and isinstance(value.value, str): + parts.append(value.value) + elif isinstance(value, ast.FormattedValue): + parts.append(f"{{{ast.unparse(value.value)}}}") + return "".join(parts) + + +def _capture_path_redirections(source: str) -> list[str]: + tree = ast.parse(source) + capture_path_names = { + node.id + for node in ast.walk(tree) + if isinstance(node, ast.Name) and _CAPTURE_PATH_TERM_RE.search(node.id) + } + assignments: dict[str, str] = {} + for node in ast.walk(tree): + if not isinstance(node, (ast.Assign, ast.AnnAssign)): + continue + targets = node.targets if isinstance(node, ast.Assign) else [node.target] + value = ast.unparse(node.value) if node.value is not None else "" + for target in targets: + if isinstance(target, ast.Name): + assignments[target.id] = value + changed = True + while changed: + changed = False + for name, value in assignments.items(): + if name in capture_path_names: + continue + if _CAPTURE_PATH_TERM_RE.search(value) or any( + re.search(rf"\b{re.escape(known)}\b", value) for known in capture_path_names + ): + capture_path_names.add(name) + changed = True + + violations: list[str] = [] + for node in ast.walk(tree): + if not isinstance(node, (ast.Constant, ast.JoinedStr)): + continue + rendered = _render_string_node(node) + for match in _REDIRECTION_RE.finditer(rendered): + target = match.group("target") + if _CAPTURE_PATH_TERM_RE.search(target) or any( + re.search(rf"\b{re.escape(name)}\b", target) for name in capture_path_names + ): + violations.append(match.group(0).strip()) + return violations + + def test_session_start_calls_canonical_capture_cleanup() -> None: source = _source("hooks/session_start_hook.py") assert "from _capture_artifacts import" in source @@ -62,12 +125,40 @@ def test_shell_capture_code_has_no_pathname_harness_or_cleanup() -> None: combined = "\n".join(sources.values()) for forbidden in ( "mkdir -p", - '> "$__as_f"', ".unlink(", "os.unlink(", "shutil.rmtree(", ): assert forbidden not in combined, f"pathname capture operation reintroduced: {forbidden}" + violations = {} + for relative, source in sources.items(): + detected = _capture_path_redirections(source) + if detected: + violations[relative] = detected + assert not violations, f"capture-root pathname redirection reintroduced: {violations}" + + +@pytest.mark.parametrize( + "harness", + [ + 'cmd > "$capture_path"', + 'cmd 2>>"${artifact_file}"', + "cmd > /tmp/.autoskillit/temp/shell_capture/shell_0123456789abcdef.log", + 'cmd &> "$shell_output"', + ], +) +def test_capture_path_redirection_guard_rejects_broad_targets(harness: str) -> None: + assert _capture_path_redirections(f"harness = {harness!r}") + + +def test_capture_path_redirection_guard_tracks_derived_path_names() -> None: + source = ( + "capture_root = CAPTURE_PATH_COMPONENTS\n" + "output_path = capture_root\n" + 'harness = f"cmd > {output_path}/shell.log"\n' + ) + assert _capture_path_redirections(source) + assert not _capture_path_redirections('harness = "cmd > ordinary.log"') def test_project_temp_cleanup_debt_is_narrow_and_owned() -> None: diff --git a/tests/hooks/test_capture_artifacts.py b/tests/hooks/test_capture_artifacts.py index 93470fbdc..6c0732abe 100644 --- a/tests/hooks/test_capture_artifacts.py +++ b/tests/hooks/test_capture_artifacts.py @@ -93,6 +93,46 @@ def record_artifact(root, capture_id): return observed_fds +def _record_runtime_fds(monkeypatch: pytest.MonkeyPatch) -> list[int]: + observed_fds = _record_owned_capture_fds(monkeypatch) + real_duplicate = capture_artifacts._duplicate_artifact_writer + + def record_duplicate(artifact): + writer_fd = real_duplicate(artifact) + observed_fds.append(writer_fd) + return writer_fd + + monkeypatch.setattr(capture_artifacts, "_duplicate_artifact_writer", record_duplicate) + return observed_fds + + +def _record_owned_capture_fds(monkeypatch: pytest.MonkeyPatch) -> list[int]: + observed_fds: list[int] = [] + real_open_anchor = capture_artifacts.open_project_anchor + real_open_root = capture_artifacts.open_capture_root + real_create = capture_artifacts.create_capture_artifact + + def record_anchor(cwd): + anchor = real_open_anchor(cwd) + observed_fds.append(anchor.fd) + return anchor + + def record_root(anchor, *, create): + root = real_open_root(anchor, create=create) + observed_fds.extend((root.autoskillit_fd, root.temp_fd, root.fd)) + return root + + def record_artifact(root, capture_id): + artifact = real_create(root, capture_id) + observed_fds.append(artifact.fd) + return artifact + + monkeypatch.setattr(capture_artifacts, "open_project_anchor", record_anchor) + monkeypatch.setattr(capture_artifacts, "open_capture_root", record_root) + monkeypatch.setattr(capture_artifacts, "create_capture_artifact", record_artifact) + return observed_fds + + def test_project_anchor_accepts_symlink_cwd(tmp_path: Path) -> None: project = tmp_path / "project" project.mkdir() @@ -315,6 +355,38 @@ def test_verified_policy_merges_overlay_and_bounds_inline_bytes(tmp_path: Path) anchor.close() +def test_policy_partial_open_failure_closes_autoskillit_fd( + tmp_path: Path, + monkeypatch: pytest.MonkeyPatch, +) -> None: + project = tmp_path / "project" + autoskillit_dir = project / CAPTURE_PATH_COMPONENTS[0] + autoskillit_dir.mkdir(parents=True) + anchor = open_project_anchor(str(project)) + opened_fds: list[int] = [] + real_open_component = capture_artifacts._open_directory_component + + def record_open_component(parent_fd, name, *, create): + fd = real_open_component(parent_fd, name, create=create) + if name == CAPTURE_PATH_COMPONENTS[0]: + opened_fds.append(fd) + return fd + + monkeypatch.setattr( + capture_artifacts, + "_open_directory_component", + record_open_component, + ) + + try: + assert read_capture_policy(anchor) == CapturePolicy() + assert len(opened_fds) == 1 + with pytest.raises(OSError): + os.fstat(opened_fds[0]) + finally: + anchor.close() + + @pytest.mark.parametrize("collision", ["symlink", "hardlink", "regular"]) def test_artifact_creation_rejects_existing_entries(collision: str, tmp_path: Path) -> None: project = tmp_path / "project" @@ -382,6 +454,46 @@ def test_marker_path_rederives_symlinked_project_path(tmp_path: Path) -> None: anchor.close() +def test_marker_directory_identity_failure_closes_partial_open_fd( + tmp_path: Path, + monkeypatch: pytest.MonkeyPatch, +) -> None: + project = tmp_path / "project" + autoskillit_dir = project / CAPTURE_PATH_COMPONENTS[0] + autoskillit_dir.mkdir(parents=True) + anchor = open_project_anchor(str(project)) + opened_fds: list[int] = [] + real_open_component = capture_artifacts._open_directory_component + + def record_open_component(parent_fd, name, *, create): + fd = real_open_component(parent_fd, name, create=create) + opened_fds.append(fd) + return fd + + def fail_identity(_fd, _expected): + raise OSError("fault injection") + + monkeypatch.setattr( + capture_artifacts, + "_open_directory_component", + record_open_component, + ) + monkeypatch.setattr(capture_artifacts, "_same_identity", fail_identity) + + try: + with pytest.raises(OSError, match="fault injection"): + capture_artifacts._open_and_match_directory( + anchor.fd, + CAPTURE_PATH_COMPONENTS[0], + anchor.identity, + ) + assert len(opened_fds) == 1 + with pytest.raises(OSError): + os.fstat(opened_fds[0]) + finally: + anchor.close() + + def test_stale_capture_is_safely_retained_without_identity_unlink(tmp_path: Path) -> None: project = tmp_path / "project" capture_dir = _capture_dir(project) @@ -532,6 +644,41 @@ def swap_after_validation(root, name, *, mtime_threshold): assert displaced.read_bytes() == b"validated" +def test_cleanup_failure_for_one_entry_does_not_skip_later_entries( + tmp_path: Path, + monkeypatch: pytest.MonkeyPatch, +) -> None: + project = tmp_path / "project" + capture_dir = _capture_dir(project) + capture_dir.mkdir(parents=True) + stale_paths = [ + capture_dir / "shell_1111111111111111.log", + capture_dir / "shell_2222222222222222.log", + ] + for path in stale_paths: + path.write_text(path.name) + os.utime(path, (0, 0)) + calls: list[str] = [] + real_open = capture_artifacts._open_stale_candidate + + def fail_first_entry(root, name, *, mtime_threshold): + calls.append(name) + if len(calls) == 1: + raise OSError("fault injection") + return real_open(root, name, mtime_threshold=mtime_threshold) + + monkeypatch.setattr( + capture_artifacts, + "_open_stale_candidate", + fail_first_entry, + ) + + assert sweep_stale_captures(project, max_age_seconds=0) == 0 + assert len(calls) == 2 + assert {path.name for path in stale_paths} == set(calls) + assert all(path.exists() for path in stale_paths) + + def test_setup_failure_prevents_user_command_and_emits_failure_marker( tmp_path: Path, capfd: pytest.CaptureFixture[str], @@ -583,32 +730,11 @@ def test_spawn_failure_closes_created_artifact_fd( ) -> None: project = tmp_path / "project" project.mkdir() - observed_fds: list[int] = [] - real_open_anchor = capture_artifacts.open_project_anchor - real_open_root = capture_artifacts.open_capture_root - real_create = capture_artifacts.create_capture_artifact - - def record_anchor(cwd): - anchor = real_open_anchor(cwd) - observed_fds.append(anchor.fd) - return anchor - - def record_root(anchor, *, create): - root = real_open_root(anchor, create=create) - observed_fds.extend((root.autoskillit_fd, root.temp_fd, root.fd)) - return root - - def record_artifact(root, capture_id): - artifact = real_create(root, capture_id) - observed_fds.append(artifact.fd) - return artifact + observed_fds = _record_owned_capture_fds(monkeypatch) def fail_spawn(*args, **kwargs): raise OSError("fault injection") - monkeypatch.setattr(capture_artifacts, "open_project_anchor", record_anchor) - monkeypatch.setattr(capture_artifacts, "open_capture_root", record_root) - monkeypatch.setattr(capture_artifacts, "create_capture_artifact", record_artifact) monkeypatch.setattr(subprocess, "Popen", fail_spawn) encoded = base64.b64encode(b"printf ran > command_ran").decode() @@ -624,6 +750,48 @@ def fail_spawn(*args, **kwargs): os.fstat(fd) +def test_post_duplication_failure_closes_all_fds_and_prevents_command( + tmp_path: Path, + monkeypatch: pytest.MonkeyPatch, + capfd: pytest.CaptureFixture[str], +) -> None: + project = tmp_path / "project" + project.mkdir() + observed_fds = _record_owned_capture_fds(monkeypatch) + duplicated_fds: list[int] = [] + real_dup = capture_artifacts.os.dup + + def record_dup(fd): + duplicated_fd = real_dup(fd) + duplicated_fds.append(duplicated_fd) + return duplicated_fd + + def fail_duplicated_identity(_fd, _expected): + raise OSError("fault injection after fd duplication") + + def unexpected_spawn(*_args, **_kwargs): + raise AssertionError("command must not spawn after fd duplication failure") + + monkeypatch.setattr(capture_artifacts.os, "dup", record_dup) + monkeypatch.setattr(capture_artifacts, "_same_identity", fail_duplicated_identity) + monkeypatch.setattr(capture_artifacts, "_spawn_bash", unexpected_spawn) + encoded = base64.b64encode(b"printf ran > command_ran").decode() + + assert capture_artifacts._main(["run", encoded, str(project), _CAPTURE_ID]) == 1 + + captured = capfd.readouterr() + artifact_path = _capture_dir(project) / f"shell_{_CAPTURE_ID}.log" + assert "CAPTURE_FAILED" in captured.err + assert "SHELL_OUTPUT_CAPTURED" not in captured.out + captured.err + assert not (project / "command_ran").exists() + assert artifact_path.read_bytes() == b"" + assert len(observed_fds) == 5 + assert len(duplicated_fds) == 1 + for fd in [*observed_fds, *duplicated_fds]: + with pytest.raises(OSError): + os.fstat(fd) + + def test_restore_failure_closes_pipe_and_original_cwd_fd( tmp_path: Path, monkeypatch: pytest.MonkeyPatch, @@ -829,6 +997,49 @@ def record_artifact(root, capture_id): os.fstat(fd) +def test_capture_readback_failure_after_partial_output_closes_runtime_fds( + tmp_path: Path, + monkeypatch: pytest.MonkeyPatch, + capfd: pytest.CaptureFixture[str], +) -> None: + project = tmp_path / "project" + project.mkdir() + observed_fds = _record_runtime_fds(monkeypatch) + + class PartialReadbackStream: + def __init__(self) -> None: + self.read_calls = 0 + self.closed = False + + def read(self, _size): + self.read_calls += 1 + if self.read_calls == 1: + return b"partial-output" + raise OSError("fault injection during readback") + + def close(self): + self.closed = True + + process = _FakeCaptureProcess(b"") + process.stdout = PartialReadbackStream() + monkeypatch.setattr(capture_artifacts, "_spawn_bash", lambda *_args, **_kwargs: process) + + assert run_capture("printf output", str(project), _CAPTURE_ID) == 0 + + captured = capfd.readouterr() + artifact_path = _capture_dir(project) / f"shell_{_CAPTURE_ID}.log" + assert "CAPTURE_FAILED" in captured.err + assert "SHELL_OUTPUT_CAPTURED" not in captured.out + captured.err + assert artifact_path.read_bytes() == b"partial-output" + assert process.terminated + assert process.stdout.closed + assert process.wait_calls == 1 + assert len(observed_fds) == 6 + for fd in observed_fds: + with pytest.raises(OSError): + os.fstat(fd) + + def test_digest_failure_emits_failure_and_closes_runtime_resources( tmp_path: Path, monkeypatch: pytest.MonkeyPatch, @@ -939,6 +1150,7 @@ def test_marker_verification_failure_emits_failure_marker( ) -> None: project = tmp_path / "project" project.mkdir() + observed_fds = _record_runtime_fds(monkeypatch) def fail_verification(anchor, root, artifact): raise OSError("fault injection") @@ -953,3 +1165,7 @@ def fail_verification(anchor, root, artifact): captured = capfd.readouterr() assert "CAPTURE_FAILED" in captured.err assert "SHELL_OUTPUT_CAPTURED" not in captured.out + captured.err + assert len(observed_fds) == 6 + for fd in observed_fds: + with pytest.raises(OSError): + os.fstat(fd) From 48cfb773344bb5760e8719cb500958935647e55c Mon Sep 17 00:00:00 2001 From: Trecek Date: Thu, 23 Jul 2026 10:37:37 -0700 Subject: [PATCH 07/32] fix: scope shell capture provenance assertion --- tests/hooks/test_shell_capture_hook.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/hooks/test_shell_capture_hook.py b/tests/hooks/test_shell_capture_hook.py index e02fb58ed..8c83178da 100644 --- a/tests/hooks/test_shell_capture_hook.py +++ b/tests/hooks/test_shell_capture_hook.py @@ -208,7 +208,7 @@ def test_marker_provenance_is_emitted_by_runner(monkeypatch, tmp_path): assert Path(argv[2]).name == "_capture_artifacts.py" assert re.fullmatch(r"[0-9a-f]{16}", argv[-1]) assert "printf 0123456789abcdef" not in command - assert "AutoSkillit" not in command + assert "AutoSkillit hook shell_capture_hook" not in command assert "`" not in command completed = subprocess.run( From c6244be3cd06f50a10faf47f4a5aad942281c0fa Mon Sep 17 00:00:00 2001 From: Trecek Date: Thu, 23 Jul 2026 11:06:23 -0700 Subject: [PATCH 08/32] fix(review): document output budget bridge names --- docs/safety/hooks.md | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) diff --git a/docs/safety/hooks.md b/docs/safety/hooks.md index 5f0ce6f6f..c27751a35 100644 --- a/docs/safety/hooks.md +++ b/docs/safety/hooks.md @@ -96,9 +96,12 @@ descriptors without following symlinks. It reads output policy through the verif semantics. The child sends combined stdout+stderr through a pipe; the runner writes, measures, hashes, and replays bytes through owned descriptors. Only a bounded inline slice enters context: full content when small, otherwise head + provenance marker -(bytes, sha256, verified path or `unavailable`) + tail. Set -`output_budget.guard_enabled: false` to disable capture after verified policy loading; -the inline threshold is controlled by `output_budget.shell_max_inline_bytes`. +(bytes, sha256, verified path or `unavailable`) + tail. The user-facing configuration +uses `output_budget.guard_enabled` and `output_budget.shell_max_inline_bytes`. The +server serializes those values into the stdlib hook bridge as +`output_budget_policy.disabled` (the inverse of `guard_enabled`) and +`output_budget_policy.shell_max_inline_bytes`; the runner reads that internal bridge +shape after verified policy loading. #### Capture Artifact Lifecycle From a43f052a015ebaedbaaaec375c111ca4b64d802d Mon Sep 17 00:00:00 2001 From: Trecek Date: Thu, 23 Jul 2026 11:06:58 -0700 Subject: [PATCH 09/32] fix(review): validate capture directory ownership and modes --- src/autoskillit/hooks/_capture_artifacts.py | 5 ++++- tests/hooks/test_capture_artifacts.py | 24 +++++++++++++++++++++ 2 files changed, 28 insertions(+), 1 deletion(-) diff --git a/src/autoskillit/hooks/_capture_artifacts.py b/src/autoskillit/hooks/_capture_artifacts.py index 124609055..8037924a0 100644 --- a/src/autoskillit/hooks/_capture_artifacts.py +++ b/src/autoskillit/hooks/_capture_artifacts.py @@ -247,8 +247,11 @@ def _open_directory_component(parent_fd: int, name: str, *, create: bool) -> int raise CaptureSetupError(f"unsafe capture path component: {name}") from exc try: - if not stat.S_ISDIR(os.fstat(fd).st_mode): + value = os.fstat(fd) + if not stat.S_ISDIR(value.st_mode): raise CaptureSetupError(f"capture path component is not a directory: {name}") + if value.st_uid != os.geteuid() or value.st_mode & _UNTRUSTED_WRITE_BITS: + raise CaptureSetupError(f"capture path component has unsafe ownership or mode: {name}") except BaseException: os.close(fd) raise diff --git a/tests/hooks/test_capture_artifacts.py b/tests/hooks/test_capture_artifacts.py index 6c0732abe..1d5429e4d 100644 --- a/tests/hooks/test_capture_artifacts.py +++ b/tests/hooks/test_capture_artifacts.py @@ -269,6 +269,30 @@ def test_capture_root_rejects_missing_components(missing_component: str, tmp_pat anchor.close() +@pytest.mark.parametrize("unsafe_component", CAPTURE_PATH_COMPONENTS) +@pytest.mark.parametrize("unsafe_mode", [0o770, 0o707]) +def test_capture_root_rejects_writable_components( + unsafe_component: str, + unsafe_mode: int, + tmp_path: Path, +) -> None: + project = tmp_path / "project" + project.mkdir() + parent = project + for name in CAPTURE_PATH_COMPONENTS: + parent = parent / name + parent.mkdir() + if name == unsafe_component: + parent.chmod(unsafe_mode) + + anchor = open_project_anchor(str(project)) + try: + with pytest.raises(CaptureSetupError, match="unsafe ownership or mode"): + open_capture_root(anchor, create=True) + finally: + anchor.close() + + @pytest.mark.parametrize("blocking_component", CAPTURE_PATH_COMPONENTS) def test_capture_root_rejects_blocking_regular_file_components( blocking_component: str, tmp_path: Path From 7644b449613288eaefb0112ee6a42d851c2dc3ab Mon Sep 17 00:00:00 2001 From: Trecek Date: Thu, 23 Jul 2026 11:07:37 -0700 Subject: [PATCH 10/32] fix(review): encode capture marker paths --- src/autoskillit/hooks/_capture_artifacts.py | 8 +++++++- tests/hooks/test_capture_artifacts.py | 19 +++++++++++++++++++ 2 files changed, 26 insertions(+), 1 deletion(-) diff --git a/src/autoskillit/hooks/_capture_artifacts.py b/src/autoskillit/hooks/_capture_artifacts.py index 8037924a0..81d66a986 100644 --- a/src/autoskillit/hooks/_capture_artifacts.py +++ b/src/autoskillit/hooks/_capture_artifacts.py @@ -628,6 +628,12 @@ def _capture_failure_return(detail: str, returncode: int | None) -> int: return returncode if returncode is not None else _CAPTURE_FAILURE_RETURN_CODE +def _encode_marker_path(path: str) -> str: + """Return a single-line path that cannot terminate the provenance marker.""" + + return json.dumps(path, ensure_ascii=True)[1:-1].replace("]", "\\u005d") + + def _emit_capture( result: _DrainResult, artifact_path: str | None, @@ -637,7 +643,7 @@ def _emit_capture( payload = result.inline else: prefix = render_capture_marker(_capture_event("SHELL_OUTPUT_CAPTURED", "input rewrite")) - path = artifact_path if artifact_path is not None else "unavailable" + path = _encode_marker_path(artifact_path) if artifact_path is not None else "unavailable" marker = ( f"\n{prefix} full output {result.total_bytes} bytes -> {path} " f"sha256={result.sha256} complete=true]\n" diff --git a/tests/hooks/test_capture_artifacts.py b/tests/hooks/test_capture_artifacts.py index 1d5429e4d..f3256a1d9 100644 --- a/tests/hooks/test_capture_artifacts.py +++ b/tests/hooks/test_capture_artifacts.py @@ -478,6 +478,25 @@ def test_marker_path_rederives_symlinked_project_path(tmp_path: Path) -> None: anchor.close() +def test_capture_marker_encodes_path_control_characters( + capfd: pytest.CaptureFixture[str], +) -> None: + result = capture_artifacts._DrainResult( + total_bytes=2, + sha256="a" * 64, + inline=b"", + head=b"h", + tail=b"t", + write_error=None, + ) + + capture_artifacts._emit_capture(result, "/project\n] forged", inline_bytes=1) + + captured = capfd.readouterr() + assert "/project\\n\\u005d forged" in captured.out + assert "\n] forged" not in captured.out + + def test_marker_directory_identity_failure_closes_partial_open_fd( tmp_path: Path, monkeypatch: pytest.MonkeyPatch, From e23886742ed61cae5fdf2aa0852bb522de1a59e0 Mon Sep 17 00:00:00 2001 From: Trecek Date: Thu, 23 Jul 2026 11:08:21 -0700 Subject: [PATCH 11/32] fix(review): bound failed capture process shutdown --- src/autoskillit/hooks/_capture_artifacts.py | 12 ++++++++- tests/hooks/test_capture_artifacts.py | 29 +++++++++++++++++++-- 2 files changed, 38 insertions(+), 3 deletions(-) diff --git a/src/autoskillit/hooks/_capture_artifacts.py b/src/autoskillit/hooks/_capture_artifacts.py index 81d66a986..affc97bf6 100644 --- a/src/autoskillit/hooks/_capture_artifacts.py +++ b/src/autoskillit/hooks/_capture_artifacts.py @@ -70,6 +70,7 @@ _MAX_ENCODED_COMMAND_BYTES = ((_MAX_COMMAND_BYTES + 2) // 3) * 4 _DRAIN_CHUNK_BYTES = 64 * 1024 _CAPTURE_FAILURE_RETURN_CODE = 1 +_PROCESS_SETTLE_TIMEOUT_SECONDS = 2 _TRUSTED_BASH_CANDIDATES = ("/bin/bash", "/usr/bin/bash") _EXECUTABLE_MODE_BITS = stat.S_IXUSR | stat.S_IXGRP | stat.S_IXOTH _UNTRUSTED_WRITE_BITS = stat.S_IWGRP | stat.S_IWOTH @@ -696,7 +697,16 @@ def _settle_failed_capture(process: subprocess.Popen[bytes]) -> int | None: except _CAPTURE_RUNTIME_ERRORS: pass try: - return _normalized_returncode(process.wait()) + return _normalized_returncode(process.wait(timeout=_PROCESS_SETTLE_TIMEOUT_SECONDS)) + except subprocess.TimeoutExpired: + try: + process.kill() + except _CAPTURE_RUNTIME_ERRORS: + pass + try: + return _normalized_returncode(process.wait(timeout=_PROCESS_SETTLE_TIMEOUT_SECONDS)) + except _CAPTURE_RUNTIME_ERRORS: + return None except _CAPTURE_RUNTIME_ERRORS: return None diff --git a/tests/hooks/test_capture_artifacts.py b/tests/hooks/test_capture_artifacts.py index f3256a1d9..d4b723d62 100644 --- a/tests/hooks/test_capture_artifacts.py +++ b/tests/hooks/test_capture_artifacts.py @@ -75,7 +75,8 @@ def terminate(self) -> None: def kill(self) -> None: self.killed = True - def wait(self) -> int: + def wait(self, timeout: float | None = None) -> int: + del timeout self.wait_calls += 1 return 0 @@ -93,6 +94,29 @@ def record_artifact(root, capture_id): return observed_fds +def test_settle_failed_capture_escalates_after_terminate_timeout() -> None: + class StubbornProcess(_FakeCaptureProcess): + def __init__(self) -> None: + super().__init__(b"") + self.wait_timeouts: list[float | None] = [] + + def wait(self, timeout: float | None = None) -> int: + self.wait_timeouts.append(timeout) + if len(self.wait_timeouts) == 1: + raise subprocess.TimeoutExpired("capture", timeout) + return -9 + + process = StubbornProcess() + + assert capture_artifacts._settle_failed_capture(process) == 137 + assert process.terminated + assert process.killed + assert process.wait_timeouts == [ + capture_artifacts._PROCESS_SETTLE_TIMEOUT_SECONDS, + capture_artifacts._PROCESS_SETTLE_TIMEOUT_SECONDS, + ] + + def _record_runtime_fds(monkeypatch: pytest.MonkeyPatch) -> list[int]: observed_fds = _record_owned_capture_fds(monkeypatch) real_duplicate = capture_artifacts._duplicate_artifact_writer @@ -1015,7 +1039,8 @@ def poll(self): def terminate(self): self.terminated = True - def wait(self): + def wait(self, timeout=None): + del timeout return 0 process = FailingProcess() From dbbbe5d909492854cf3a5e10799d7ec4d5eb0d6d Mon Sep 17 00:00:00 2001 From: Trecek Date: Thu, 23 Jul 2026 11:08:55 -0700 Subject: [PATCH 12/32] fix(review): preserve capture failure context --- src/autoskillit/hooks/_capture_artifacts.py | 9 ++++++--- tests/hooks/test_capture_artifacts.py | 1 + 2 files changed, 7 insertions(+), 3 deletions(-) diff --git a/src/autoskillit/hooks/_capture_artifacts.py b/src/autoskillit/hooks/_capture_artifacts.py index affc97bf6..63e2f101b 100644 --- a/src/autoskillit/hooks/_capture_artifacts.py +++ b/src/autoskillit/hooks/_capture_artifacts.py @@ -617,7 +617,7 @@ def _capture_event(reason_code: str, decision: str) -> PolicyEvent: def _emit_failure(detail: str) -> None: prefix = render_capture_marker(_capture_event("CAPTURE_FAILED", "deny")) - safe_detail = " ".join(detail.split())[:240] + safe_detail = " ".join(detail.split()).replace("]", "\\u005d")[:240] sys.stderr.write(f"{prefix} {safe_detail}]\n") @@ -754,10 +754,13 @@ def run_capture(command: str, cwd: str, capture_id: str) -> int: failure_stage = "capture replay emission" _emit_capture(result, artifact_path, policy.inline_bytes) return returncode - except _CAPTURE_RUNTIME_ERRORS: + except _CAPTURE_RUNTIME_ERRORS as exc: if returncode is None: returncode = _settle_failed_capture(process) - return _capture_failure_return(f"{failure_stage} failed", returncode) + return _capture_failure_return( + f"{failure_stage} failed: {type(exc).__name__}: {exc}", + returncode, + ) finally: if process is not None and process.stdout is not None: try: diff --git a/tests/hooks/test_capture_artifacts.py b/tests/hooks/test_capture_artifacts.py index d4b723d62..d038c5be2 100644 --- a/tests/hooks/test_capture_artifacts.py +++ b/tests/hooks/test_capture_artifacts.py @@ -849,6 +849,7 @@ def unexpected_spawn(*_args, **_kwargs): captured = capfd.readouterr() artifact_path = _capture_dir(project) / f"shell_{_CAPTURE_ID}.log" assert "CAPTURE_FAILED" in captured.err + assert "OSError: fault injection during readback" in captured.err assert "SHELL_OUTPUT_CAPTURED" not in captured.out + captured.err assert not (project / "command_ran").exists() assert artifact_path.read_bytes() == b"" From 68deb53712d4f777c04ce858dcfb56eb1afd0f98 Mon Sep 17 00:00:00 2001 From: Trecek Date: Thu, 23 Jul 2026 11:10:34 -0700 Subject: [PATCH 13/32] fix(review): rename stale capture classifier --- src/autoskillit/hooks/_capture_artifacts.py | 4 ++-- src/autoskillit/hooks/session_start_hook.py | 4 ++-- tests/arch/test_shell_capture_trust_anchor.py | 4 ++-- tests/hooks/test_capture_artifacts.py | 14 +++++++------- tests/hooks/test_shell_capture_conformance.py | 12 ++++++------ 5 files changed, 19 insertions(+), 19 deletions(-) diff --git a/src/autoskillit/hooks/_capture_artifacts.py b/src/autoskillit/hooks/_capture_artifacts.py index 63e2f101b..2d5a9017b 100644 --- a/src/autoskillit/hooks/_capture_artifacts.py +++ b/src/autoskillit/hooks/_capture_artifacts.py @@ -56,7 +56,7 @@ "open_project_anchor", "read_capture_policy", "run_capture", - "sweep_stale_captures", + "classify_stale_captures", ] CAPTURE_PATH_COMPONENTS = (".autoskillit", "temp", "shell_capture") @@ -836,7 +836,7 @@ def _open_stale_candidate( ) -def sweep_stale_captures( +def classify_stale_captures( project_root: str | Path, *, max_age_seconds: int = 3600, diff --git a/src/autoskillit/hooks/session_start_hook.py b/src/autoskillit/hooks/session_start_hook.py index ec3df16ae..bd0d5da58 100644 --- a/src/autoskillit/hooks/session_start_hook.py +++ b/src/autoskillit/hooks/session_start_hook.py @@ -21,7 +21,7 @@ sys.path.insert(0, _HOOKS_DIR) from _capture_artifacts import ( # type: ignore[import-not-found] # noqa: E402 - sweep_stale_captures, + classify_stale_captures, ) @@ -88,7 +88,7 @@ def main() -> None: pass try: - sweep_stale_captures(Path.cwd()) + classify_stale_captures(Path.cwd()) except Exception: pass diff --git a/tests/arch/test_shell_capture_trust_anchor.py b/tests/arch/test_shell_capture_trust_anchor.py index 71060c163..d15371be8 100644 --- a/tests/arch/test_shell_capture_trust_anchor.py +++ b/tests/arch/test_shell_capture_trust_anchor.py @@ -107,10 +107,10 @@ def _capture_path_redirections(source: str) -> list[str]: return violations -def test_session_start_calls_canonical_capture_cleanup() -> None: +def test_session_start_calls_canonical_capture_classification() -> None: source = _source("hooks/session_start_hook.py") assert "from _capture_artifacts import" in source - assert "sweep_stale_captures(Path.cwd())" in source + assert "classify_stale_captures(Path.cwd())" in source assert "shell_capture" not in source diff --git a/tests/hooks/test_capture_artifacts.py b/tests/hooks/test_capture_artifacts.py index d038c5be2..4214ba41d 100644 --- a/tests/hooks/test_capture_artifacts.py +++ b/tests/hooks/test_capture_artifacts.py @@ -18,13 +18,13 @@ CAPTURE_PATH_COMPONENTS, CapturePolicy, CaptureSetupError, + classify_stale_captures, create_capture_artifact, current_artifact_path_if_bound, open_capture_root, open_project_anchor, read_capture_policy, run_capture, - sweep_stale_captures, ) pytestmark = [pytest.mark.layer("hooks"), pytest.mark.medium] @@ -570,7 +570,7 @@ def test_stale_capture_is_safely_retained_without_identity_unlink(tmp_path: Path old = time.time() - 7200 os.utime(stale, (old, old)) - assert sweep_stale_captures(project, max_age_seconds=3600) == 0 + assert classify_stale_captures(project, max_age_seconds=3600) == 0 assert stale.read_bytes() == b"retained" @@ -585,8 +585,8 @@ def test_name_matching_fifo_does_not_block_stale_cleanup(tmp_path: Path) -> None pytest.skip(f"FIFO unavailable: {exc}") code = ( "import sys\n" - "from autoskillit.hooks._capture_artifacts import sweep_stale_captures\n" - "raise SystemExit(sweep_stale_captures(sys.argv[1], max_age_seconds=0))\n" + "from autoskillit.hooks._capture_artifacts import classify_stale_captures\n" + "raise SystemExit(classify_stale_captures(sys.argv[1], max_age_seconds=0))\n" ) completed = subprocess.run( @@ -673,7 +673,7 @@ def test_cleanup_rejects_symlinked_capture_root(tmp_path: Path) -> None: os.utime(stale, (old, old)) (temp_dir / CAPTURE_PATH_COMPONENTS[2]).symlink_to(external, target_is_directory=True) - assert sweep_stale_captures(project, max_age_seconds=0) == 0 + assert classify_stale_captures(project, max_age_seconds=0) == 0 assert stale.read_bytes() == b"must-survive" @@ -706,7 +706,7 @@ def swap_after_validation(root, name, *, mtime_threshold): swap_after_validation, ) - assert sweep_stale_captures(project, max_age_seconds=0) == 0 + assert classify_stale_captures(project, max_age_seconds=0) == 0 assert stale.read_bytes() == b"replacement" assert displaced.read_bytes() == b"validated" @@ -740,7 +740,7 @@ def fail_first_entry(root, name, *, mtime_threshold): fail_first_entry, ) - assert sweep_stale_captures(project, max_age_seconds=0) == 0 + assert classify_stale_captures(project, max_age_seconds=0) == 0 assert len(calls) == 2 assert {path.name for path in stale_paths} == set(calls) assert all(path.exists() for path in stale_paths) diff --git a/tests/hooks/test_shell_capture_conformance.py b/tests/hooks/test_shell_capture_conformance.py index aabb19205..a63842b93 100644 --- a/tests/hooks/test_shell_capture_conformance.py +++ b/tests/hooks/test_shell_capture_conformance.py @@ -27,7 +27,7 @@ import autoskillit.hooks.shell_capture_hook as shell_capture_hook from autoskillit.hooks._capture_artifacts import ( _CAPTURE_FILENAME_RE, - sweep_stale_captures, + classify_stale_captures, ) from autoskillit.hooks.shell_capture_hook import _build_harness @@ -517,7 +517,7 @@ def test_small_output_artifact_safely_retained_by_sweep(tmp_path: Path) -> None: target = artifacts[0] os.utime(target, (0, 0)) - deleted = sweep_stale_captures(tmp_path, max_age_seconds=0) + deleted = classify_stale_captures(tmp_path, max_age_seconds=0) assert deleted == 0 assert target.exists() @@ -533,7 +533,7 @@ def test_sweep_rejects_symlinks_and_traversals(tmp_path: Path) -> None: symlink.symlink_to(outside) assert symlink.is_symlink() - deleted = sweep_stale_captures(tmp_path, max_age_seconds=0) + deleted = classify_stale_captures(tmp_path, max_age_seconds=0) assert deleted == 0 assert outside.exists(), "outside target file must be untouched by sweep" assert outside.read_text() == "must-survive" @@ -554,14 +554,14 @@ def test_sweep_filename_allowlist(tmp_path: Path) -> None: invalid_ext = capture / "evil.sh" invalid_ext.write_text("x") - deleted = sweep_stale_captures(tmp_path, max_age_seconds=0) + deleted = classify_stale_captures(tmp_path, max_age_seconds=0) assert deleted == 0 assert valid.exists() assert short_uid.exists(), "old 8-char uid format files must not be deleted" assert invalid_ext.exists(), "files outside the shell_*.log allowlist must not be deleted" -def test_sweep_stale_captures_uses_wall_clock_not_directory_mtime(tmp_path: Path) -> None: +def test_classify_stale_captures_uses_wall_clock_not_directory_mtime(tmp_path: Path) -> None: """Staleness must be measured against real elapsed time, not the capture directory's own mtime (which only advances when its contents change).""" _make_project_dirs(tmp_path) @@ -574,7 +574,7 @@ def test_sweep_stale_captures_uses_wall_clock_not_directory_mtime(tmp_path: Path os.utime(stale, (backdated, backdated)) os.utime(capture, (backdated, backdated)) - deleted = sweep_stale_captures(tmp_path, max_age_seconds=100) + deleted = classify_stale_captures(tmp_path, max_age_seconds=100) assert deleted == 0 assert stale.exists() From b4305b126fc38f9525d4028caf5618aa3056ac75 Mon Sep 17 00:00:00 2001 From: Trecek Date: Thu, 23 Jul 2026 11:11:41 -0700 Subject: [PATCH 14/32] fix(review): centralize capture transport contract --- src/autoskillit/hooks/AGENTS.md | 1 + src/autoskillit/hooks/_capture_artifacts.py | 7 +++++-- src/autoskillit/hooks/_capture_contract.py | 8 ++++++++ src/autoskillit/hooks/shell_capture_hook.py | 12 +++++++++--- tests/hooks/test_shell_capture_hook.py | 4 +++- 5 files changed, 26 insertions(+), 6 deletions(-) create mode 100644 src/autoskillit/hooks/_capture_contract.py diff --git a/src/autoskillit/hooks/AGENTS.md b/src/autoskillit/hooks/AGENTS.md index de741db55..f1d38b128 100644 --- a/src/autoskillit/hooks/AGENTS.md +++ b/src/autoskillit/hooks/AGENTS.md @@ -23,6 +23,7 @@ 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`, `STEP_SUFFIX_RE`) | | `_command_classification.py` | Shared stdlib-only command classification primitives for guard scripts (interpreter/wrapper detection, git command classification) | +| `_capture_contract.py` | Shared stdlib-only shell-capture transport limits and identifier validation | | `_policy_event.py` | Typed policy-event formatter for hook provenance messages (stdlib-only) | | `_capture_artifacts.py` | Stdlib-only descriptor-anchored shell-capture authority, runner, and cleanup classifier used by `session_start_hook.py` | | `shell_capture_hook.py` | `PreToolUse`: input-rewrite hook for Codex shell capture — wraps commands in a lossless capture harness (#4286 / ADR-0006) | diff --git a/src/autoskillit/hooks/_capture_artifacts.py b/src/autoskillit/hooks/_capture_artifacts.py index 2d5a9017b..020681c1a 100644 --- a/src/autoskillit/hooks/_capture_artifacts.py +++ b/src/autoskillit/hooks/_capture_artifacts.py @@ -27,6 +27,11 @@ if _HOOKS_DIR not in sys.path: sys.path.insert(0, _HOOKS_DIR) +from _capture_contract import ( # type: ignore[import-not-found] # noqa: E402 + _CAPTURE_ID_RE, + _MAX_COMMAND_BYTES, +) + if TYPE_CHECKING: from autoskillit.hooks._hook_settings import ( HOOK_CONFIG_FILENAME, @@ -61,12 +66,10 @@ CAPTURE_PATH_COMPONENTS = (".autoskillit", "temp", "shell_capture") _CAPTURE_FILENAME_RE = re.compile(r"^shell_[0-9a-f]{16}\.log$") -_CAPTURE_ID_RE = re.compile(r"^[0-9a-f]{16}$") _DEFAULT_INLINE_BYTES = 12_000 _MAX_INLINE_BYTES = 1_000_000 _MAX_POLICY_FILE_BYTES = 64 * 1024 -_MAX_COMMAND_BYTES = 64 * 1024 _MAX_ENCODED_COMMAND_BYTES = ((_MAX_COMMAND_BYTES + 2) // 3) * 4 _DRAIN_CHUNK_BYTES = 64 * 1024 _CAPTURE_FAILURE_RETURN_CODE = 1 diff --git a/src/autoskillit/hooks/_capture_contract.py b/src/autoskillit/hooks/_capture_contract.py new file mode 100644 index 000000000..46a6755d1 --- /dev/null +++ b/src/autoskillit/hooks/_capture_contract.py @@ -0,0 +1,8 @@ +"""Shared stdlib-only transport contract for shell capture.""" + +from __future__ import annotations + +import re + +_CAPTURE_ID_RE = re.compile(r"^[0-9a-f]{16}$") +_MAX_COMMAND_BYTES = 64 * 1024 diff --git a/src/autoskillit/hooks/shell_capture_hook.py b/src/autoskillit/hooks/shell_capture_hook.py index b9128efca..dd51db6ea 100644 --- a/src/autoskillit/hooks/shell_capture_hook.py +++ b/src/autoskillit/hooks/shell_capture_hook.py @@ -14,16 +14,22 @@ import base64 import json import os -import re import shlex import struct 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 _capture_contract import ( # type: ignore[import-not-found] # noqa: E402 + _CAPTURE_ID_RE, + _MAX_COMMAND_BYTES, +) + _HARNESS_SENTINEL = "# autoskillit-shell-capture v1" -_CAPTURE_ID_RE = re.compile(r"^[0-9a-f]{16}$") -_MAX_COMMAND_BYTES = 64 * 1024 _ARG_MAX_FALLBACK_BYTES = 128 * 1024 _ARG_MAX_HEADROOM_BYTES = 32 * 1024 _RUNNER_BASENAME = "_capture_artifacts.py" diff --git a/tests/hooks/test_shell_capture_hook.py b/tests/hooks/test_shell_capture_hook.py index 8c83178da..1acf5a1bb 100644 --- a/tests/hooks/test_shell_capture_hook.py +++ b/tests/hooks/test_shell_capture_hook.py @@ -14,6 +14,8 @@ import pytest +from autoskillit.hooks._capture_contract import _MAX_COMMAND_BYTES + pytestmark = [pytest.mark.layer("hooks"), pytest.mark.medium] _SENTINEL = "# autoskillit-shell-capture v1" @@ -161,7 +163,7 @@ def test_non_string_command_fails_open(monkeypatch): assert _run_hook(event, monkeypatch, env_backend="codex") == "" -@pytest.mark.parametrize("command", ["x" * (64 * 1024 + 1), "printf bad\x00command"]) +@pytest.mark.parametrize("command", ["x" * (_MAX_COMMAND_BYTES + 1), "printf bad\x00command"]) def test_invalid_command_transport_builds_nonexecuting_rejection(command): from autoskillit.hooks.shell_capture_hook import _build_harness From 761b132d46c8f91799500ed82b45264d71ff2ff3 Mon Sep 17 00:00:00 2001 From: Trecek Date: Thu, 23 Jul 2026 11:14:17 -0700 Subject: [PATCH 15/32] fix(review): keep wrapped commands policy visible --- src/autoskillit/hooks/shell_capture_hook.py | 15 +++++++++++---- tests/hooks/test_shell_capture_hook.py | 14 +++++++++++++- 2 files changed, 24 insertions(+), 5 deletions(-) diff --git a/src/autoskillit/hooks/shell_capture_hook.py b/src/autoskillit/hooks/shell_capture_hook.py index dd51db6ea..152a0b891 100644 --- a/src/autoskillit/hooks/shell_capture_hook.py +++ b/src/autoskillit/hooks/shell_capture_hook.py @@ -60,8 +60,13 @@ def _fits_arg_max(argv: list[str]) -> bool: return _exec_footprint(argv) + _ARG_MAX_HEADROOM_BYTES <= _system_arg_max() -def _render_harness(argv: list[str]) -> str: - return f"{_HARNESS_SENTINEL}\n{shlex.join(argv)}" +def _render_harness(argv: list[str], *, policy_command: str | None = None) -> str: + lines = [_HARNESS_SENTINEL] + if policy_command is not None: + policy_text = json.dumps(policy_command, ensure_ascii=True) + lines.append(f": {shlex.quote(policy_text)}") + lines.append(shlex.join(argv)) + return "\n".join(lines) def _runner_path() -> Path: @@ -79,6 +84,7 @@ def _build_harness(command: str, cwd: str, capture_id: str) -> str: command_bytes = b"" if "\x00" in command or not command_bytes or len(command_bytes) > _MAX_COMMAND_BYTES: argv = [sys.executable, "-I", str(_runner_path()), "reject", capture_id] + harness = _render_harness(argv) else: encoded = base64.b64encode(command_bytes).decode("ascii") argv = [ @@ -90,11 +96,12 @@ def _build_harness(command: str, cwd: str, capture_id: str) -> str: cwd, capture_id, ] - harness = _render_harness(argv) + harness = _render_harness(argv, policy_command=command) outer_argv = ["bash", "-c", harness] if not _fits_arg_max(argv) or not _fits_arg_max(outer_argv): argv = [sys.executable, "-I", str(_runner_path()), "reject", capture_id] - return _render_harness(argv) + harness = _render_harness(argv) + return harness def main() -> None: diff --git a/tests/hooks/test_shell_capture_hook.py b/tests/hooks/test_shell_capture_hook.py index 1acf5a1bb..fe1a09555 100644 --- a/tests/hooks/test_shell_capture_hook.py +++ b/tests/hooks/test_shell_capture_hook.py @@ -174,6 +174,18 @@ def test_invalid_command_transport_builds_nonexecuting_rejection(command): assert command not in harness +def test_valid_command_remains_policy_visible_without_becoming_shell_code() -> None: + from autoskillit.hooks.shell_capture_hook import _build_harness # noqa: PLC0415 + + command = "printf safe; rm -rf /policy-probe" + + harness = _build_harness(command, "/abs/project", "0123456789abcdef") + preview_argv = shlex.split(harness.splitlines()[1]) + + assert command in harness + assert preview_argv == [":", json.dumps(command)] + + def test_arg_max_exhaustion_builds_nonexecuting_rejection( monkeypatch: pytest.MonkeyPatch, ) -> None: @@ -209,7 +221,7 @@ def test_marker_provenance_is_emitted_by_runner(monkeypatch, tmp_path): assert argv[1] == "-I" assert Path(argv[2]).name == "_capture_artifacts.py" assert re.fullmatch(r"[0-9a-f]{16}", argv[-1]) - assert "printf 0123456789abcdef" not in command + assert "printf 0123456789abcdef" in command assert "AutoSkillit hook shell_capture_hook" not in command assert "`" not in command From 1199bb73810d41bfcb5d2abce6b334dc0562b19c Mon Sep 17 00:00:00 2001 From: Trecek Date: Thu, 23 Jul 2026 11:15:26 -0700 Subject: [PATCH 16/32] fix(review): remove self-referential cleanup debt assertion --- tests/arch/test_shell_capture_trust_anchor.py | 9 +-------- 1 file changed, 1 insertion(+), 8 deletions(-) diff --git a/tests/arch/test_shell_capture_trust_anchor.py b/tests/arch/test_shell_capture_trust_anchor.py index d15371be8..fb521b9b3 100644 --- a/tests/arch/test_shell_capture_trust_anchor.py +++ b/tests/arch/test_shell_capture_trust_anchor.py @@ -161,14 +161,7 @@ def test_capture_path_redirection_guard_tracks_derived_path_names() -> None: assert not _capture_path_redirections('harness = "cmd > ordinary.log"') -def test_project_temp_cleanup_debt_is_narrow_and_owned() -> None: - assert set(_PROJECT_TEMP_CLEANUP_DEBT) == { - "core/io.py", - "workspace/worktree.py", - "workspace/clone_registry.py", - "workspace/clone.py", - "scripts/recipe/create_worktree.sh", - } +def test_project_temp_cleanup_debt_entries_exist_and_are_owned() -> None: for relative, debt in _PROJECT_TEMP_CLEANUP_DEBT.items(): path = _REPO_ROOT / relative if relative.startswith("scripts/") else _SRC / relative assert path.is_file() From ed5fb6b9f4d65fc4a261463a7b14dc848bbcf6e7 Mon Sep 17 00:00:00 2001 From: Trecek Date: Thu, 23 Jul 2026 11:17:28 -0700 Subject: [PATCH 17/32] fix(review): validate hook strength matrix after xdist probes --- tests/execution/backends/conftest.py | 58 +++++++++++++++++++ .../backends/test_hook_strength_matrix.py | 31 +++++----- 2 files changed, 72 insertions(+), 17 deletions(-) diff --git a/tests/execution/backends/conftest.py b/tests/execution/backends/conftest.py index ede0787ca..c5840173c 100644 --- a/tests/execution/backends/conftest.py +++ b/tests/execution/backends/conftest.py @@ -109,6 +109,7 @@ def _compute_expected_non_inert() -> int: _probe_rows: list[dict] = [] _controller_rows: list[dict] = [] +_controller_probe_suite_collected = False def record_probe_row(row: dict) -> None: @@ -116,11 +117,55 @@ def record_probe_row(row: dict) -> None: _probe_rows.append(row) +def validate_strength_matrix(rows: list[dict]) -> list[str]: + """Return invariant failures for a completed deny-strength matrix.""" + + failures: list[str] = [] + if len(rows) != EXPECTED_NON_INERT_COMBINATIONS: + failures.append( + f"expected {EXPECTED_NON_INERT_COMBINATIONS} combinations, got {len(rows)}" + ) + + works_as_is = { + Path(script).stem + for hook in HOOK_REGISTRY + if hook.event_type == "PreToolUse" + and hook.mechanism == "deny" + and hook.codex_status == "works-as-is" + for script in hook.scripts + } + effective = { + row["hook"] + for row in rows + if row["hook"] in works_as_is and row["strength"] in {"soft", "hard"} + } + if missing := works_as_is - effective: + failures.append(f"works-as-is hooks missing soft/hard rows: {sorted(missing)}") + + not_applicable = { + Path(script).stem + for hook in HOOK_REGISTRY + if hook.codex_status == "not-applicable" + for script in hook.scripts + } + if leaked := not_applicable & {row["hook"] for row in rows}: + failures.append(f"not-applicable hooks appeared in matrix: {sorted(leaked)}") + return failures + + +def _full_probe_suite_collected(items: list[pytest.Item]) -> bool: + probe_count = sum(getattr(item, "originalname", "") == "test_probe" for item in items) + return probe_count == EXPECTED_TOTAL_PROBE_COUNT + + def pytest_testnodedown(node, error): """Controller-side: harvest rows from a finishing worker.""" + global _controller_probe_suite_collected + wo = getattr(node, "workeroutput", {}) rows = wo.get("hook_probe_rows", []) _controller_rows.extend(rows) + _controller_probe_suite_collected |= bool(wo.get("hook_probe_suite_collected", False)) def pytest_sessionfinish(session, exitstatus): @@ -128,6 +173,9 @@ def pytest_sessionfinish(session, exitstatus): if hasattr(session.config, "workerinput"): # xdist worker: send accumulated rows to controller via workeroutput IPC. session.config.workeroutput["hook_probe_rows"] = _probe_rows + session.config.workeroutput["hook_probe_suite_collected"] = _full_probe_suite_collected( + session.items + ) return # Controller (or non-xdist run): write matrix JSON. all_rows = _controller_rows or _probe_rows @@ -141,3 +189,13 @@ def pytest_sessionfinish(session, exitstatus): json.dumps({"combinations": all_rows}, indent=2), encoding="utf-8", ) + full_probe_suite = _controller_probe_suite_collected or _full_probe_suite_collected( + session.items + ) + if full_probe_suite and (failures := validate_strength_matrix(all_rows)): + reporter = session.config.pluginmanager.get_plugin("terminalreporter") + if reporter is not None: + reporter.write_sep("=", "hook deny strength matrix validation failed") + for failure in failures: + reporter.write_line(failure) + session.exitstatus = pytest.ExitCode.TESTS_FAILED diff --git a/tests/execution/backends/test_hook_strength_matrix.py b/tests/execution/backends/test_hook_strength_matrix.py index 62d03e849..84ac88632 100644 --- a/tests/execution/backends/test_hook_strength_matrix.py +++ b/tests/execution/backends/test_hook_strength_matrix.py @@ -1,9 +1,9 @@ """Meta-tests for the PreToolUse deny-mechanism strength matrix. The matrix is written to ``.autoskillit/temp/hook_deny_strength_matrix.json`` -by the probe harness conftest once probes complete. When the JSON is absent -(e.g., when only these meta-tests are collected), all three tests skip -gracefully — the probe suite must be run first to populate the matrix. +by the probe harness conftest once probes complete. The conftest validates the +completed matrix at session finish, including normal xdist runs; these tests +also validate an artifact from a prior probe-only run when one is available. """ from __future__ import annotations @@ -14,7 +14,10 @@ import pytest from autoskillit.hook_registry import HOOK_REGISTRY -from tests.execution.backends.conftest import EXPECTED_NON_INERT_COMBINATIONS +from tests.execution.backends.conftest import ( + EXPECTED_NON_INERT_COMBINATIONS, + validate_strength_matrix, +) pytestmark = [pytest.mark.layer("execution"), pytest.mark.medium] @@ -27,19 +30,6 @@ ) -@pytest.fixture(scope="module", autouse=True) -def _require_completed_probe_session(request: pytest.FixtureRequest) -> None: - probe_is_collected = any( - Path(str(item.path)).name == "test_hook_deny_efficacy_probe.py" - for item in request.session.items - ) - if probe_is_collected: - pytest.skip( - "strength matrix is serialized at session finish; " - "run matrix tests separately after the probe suite" - ) - - def _load_matrix() -> dict: """Load the serialized strength matrix, skipping if absent.""" if not _MATRIX_PATH.exists(): @@ -47,6 +37,13 @@ def _load_matrix() -> dict: return json.loads(_MATRIX_PATH.read_text(encoding="utf-8")) +def test_completed_matrix_validator_rejects_missing_rows() -> None: + failures = validate_strength_matrix([]) + + assert any("combinations" in failure for failure in failures) + assert any("works-as-is hooks" in failure for failure in failures) + + def test_matrix_combination_count() -> None: """The matrix must contain exactly EXPECTED_NON_INERT_COMBINATIONS rows. From bf624ae33ad4b4daa604b0ed1314af45b8f54261 Mon Sep 17 00:00:00 2001 From: Trecek Date: Thu, 23 Jul 2026 11:17:51 -0700 Subject: [PATCH 18/32] fix(review): bound generated wrapper subprocesses --- tests/hooks/test_shell_capture_conformance.py | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/tests/hooks/test_shell_capture_conformance.py b/tests/hooks/test_shell_capture_conformance.py index a63842b93..63af48485 100644 --- a/tests/hooks/test_shell_capture_conformance.py +++ b/tests/hooks/test_shell_capture_conformance.py @@ -312,6 +312,7 @@ def test_main_generated_wrapper_rejects_symlinked_capture_components( capture_output=True, text=True, check=False, + timeout=_TIMEOUT, ) assert completed.returncode == 1 @@ -337,6 +338,7 @@ def test_main_generated_wrapper_accepts_symlinked_cwd( cwd=supplied_cwd, capture_output=True, check=False, + timeout=_TIMEOUT, ) assert completed.returncode == 0 @@ -381,6 +383,7 @@ def test_main_generated_wrapper_rejects_final_artifact_collisions( cwd=project, capture_output=True, check=False, + timeout=_TIMEOUT, ) assert completed.returncode == 1 @@ -416,6 +419,7 @@ def test_capture_directory_replacement_uses_open_fds_and_hides_path( cwd=project, capture_output=True, check=False, + timeout=_TIMEOUT, ) assert completed.returncode == 0 @@ -464,6 +468,7 @@ def test_capture_artifact_replacement_uses_open_fd_and_hides_path( cwd=project, capture_output=True, check=False, + timeout=_TIMEOUT, ) assert completed.returncode == 0 From 16d9808f11f582bf27016ed10e0a37141738a783 Mon Sep 17 00:00:00 2001 From: Trecek Date: Thu, 23 Jul 2026 11:18:23 -0700 Subject: [PATCH 19/32] fix(review): assert stale filename classifier decisions --- tests/hooks/test_shell_capture_conformance.py | 41 +++++++++++++++---- 1 file changed, 34 insertions(+), 7 deletions(-) diff --git a/tests/hooks/test_shell_capture_conformance.py b/tests/hooks/test_shell_capture_conformance.py index 63af48485..753fdd952 100644 --- a/tests/hooks/test_shell_capture_conformance.py +++ b/tests/hooks/test_shell_capture_conformance.py @@ -27,7 +27,10 @@ import autoskillit.hooks.shell_capture_hook as shell_capture_hook from autoskillit.hooks._capture_artifacts import ( _CAPTURE_FILENAME_RE, + _open_stale_candidate, classify_stale_captures, + open_capture_root, + open_project_anchor, ) from autoskillit.hooks.shell_capture_hook import _build_harness @@ -547,8 +550,8 @@ def test_sweep_rejects_symlinks_and_traversals(tmp_path: Path) -> None: ) -def test_sweep_filename_allowlist(tmp_path: Path) -> None: - """Sweep only deletes files matching the strict ``shell_<16hex>.log`` allowlist.""" +def test_stale_candidate_classifier_enforces_filename_allowlist(tmp_path: Path) -> None: + """Only strict ``shell_<16hex>.log`` names reach stale-candidate classification.""" _make_project_dirs(tmp_path) capture = _capture_dir(tmp_path) @@ -559,11 +562,35 @@ def test_sweep_filename_allowlist(tmp_path: Path) -> None: invalid_ext = capture / "evil.sh" invalid_ext.write_text("x") - deleted = classify_stale_captures(tmp_path, max_age_seconds=0) - assert deleted == 0 - assert valid.exists() - assert short_uid.exists(), "old 8-char uid format files must not be deleted" - assert invalid_ext.exists(), "files outside the shell_*.log allowlist must not be deleted" + anchor = open_project_anchor(str(tmp_path)) + root = open_capture_root(anchor, create=False) + try: + candidate = _open_stale_candidate( + root, + valid.name, + mtime_threshold=time.time() + 1, + ) + assert candidate is not None + candidate.close() + assert ( + _open_stale_candidate( + root, + short_uid.name, + mtime_threshold=time.time() + 1, + ) + is None + ) + assert ( + _open_stale_candidate( + root, + invalid_ext.name, + mtime_threshold=time.time() + 1, + ) + is None + ) + finally: + root.close() + anchor.close() def test_classify_stale_captures_uses_wall_clock_not_directory_mtime(tmp_path: Path) -> None: From 606522c0a542fe943b3eefb98ac0055a384f9473 Mon Sep 17 00:00:00 2001 From: Trecek Date: Thu, 23 Jul 2026 11:18:53 -0700 Subject: [PATCH 20/32] fix(review): assert stale capture wall-clock threshold --- tests/hooks/test_shell_capture_conformance.py | 28 ++++++++++++------- 1 file changed, 18 insertions(+), 10 deletions(-) diff --git a/tests/hooks/test_shell_capture_conformance.py b/tests/hooks/test_shell_capture_conformance.py index 753fdd952..5cc549008 100644 --- a/tests/hooks/test_shell_capture_conformance.py +++ b/tests/hooks/test_shell_capture_conformance.py @@ -24,6 +24,7 @@ import pytest +import autoskillit.hooks._capture_artifacts as capture_artifacts import autoskillit.hooks.shell_capture_hook as shell_capture_hook from autoskillit.hooks._capture_artifacts import ( _CAPTURE_FILENAME_RE, @@ -593,22 +594,29 @@ def test_stale_candidate_classifier_enforces_filename_allowlist(tmp_path: Path) anchor.close() -def test_classify_stale_captures_uses_wall_clock_not_directory_mtime(tmp_path: Path) -> None: - """Staleness must be measured against real elapsed time, not the capture - directory's own mtime (which only advances when its contents change).""" +def test_stale_capture_classifier_uses_wall_clock_threshold( + tmp_path: Path, + monkeypatch: pytest.MonkeyPatch, +) -> None: _make_project_dirs(tmp_path) capture = _capture_dir(tmp_path) - stale = capture / f"shell_{uuid4().hex[:16]}.log" stale.write_text("x") + os.utime(stale, (300, 300)) + os.utime(capture, (499, 499)) - backdated = time.time() - 200 - os.utime(stale, (backdated, backdated)) - os.utime(capture, (backdated, backdated)) + observed_thresholds: list[float] = [] + real_classifier = capture_artifacts._open_stale_candidate - deleted = classify_stale_captures(tmp_path, max_age_seconds=100) - assert deleted == 0 - assert stale.exists() + def observe_threshold(root, name, *, mtime_threshold): + observed_thresholds.append(mtime_threshold) + return real_classifier(root, name, mtime_threshold=mtime_threshold) + + monkeypatch.setattr(capture_artifacts.time, "time", lambda: 500) + monkeypatch.setattr(capture_artifacts, "_open_stale_candidate", observe_threshold) + + assert classify_stale_captures(tmp_path, max_age_seconds=100) == 0 + assert observed_thresholds == [400] def test_capture_filename_regex_consistency() -> None: From a3c99bc3c13dad539461dbbf1df868ef738b3828 Mon Sep 17 00:00:00 2001 From: Trecek Date: Thu, 23 Jul 2026 11:19:10 -0700 Subject: [PATCH 21/32] fix(review): bound shell capture hook subprocesses --- tests/hooks/test_shell_capture_hook.py | 3 +++ 1 file changed, 3 insertions(+) diff --git a/tests/hooks/test_shell_capture_hook.py b/tests/hooks/test_shell_capture_hook.py index fe1a09555..3527c4a3c 100644 --- a/tests/hooks/test_shell_capture_hook.py +++ b/tests/hooks/test_shell_capture_hook.py @@ -19,6 +19,7 @@ pytestmark = [pytest.mark.layer("hooks"), pytest.mark.medium] _SENTINEL = "# autoskillit-shell-capture v1" +_TIMEOUT = 30 def _build_event(command, cwd: str = "/abs/project") -> dict: @@ -138,6 +139,7 @@ def test_verified_disabled_policy_is_runner_owned(monkeypatch, tmp_path): capture_output=True, text=True, check=False, + timeout=_TIMEOUT, ) assert completed.returncode == 0 @@ -231,6 +233,7 @@ def test_marker_provenance_is_emitted_by_runner(monkeypatch, tmp_path): capture_output=True, text=True, check=False, + timeout=_TIMEOUT, ) assert completed.returncode == 0 assert "AutoSkillit hook shell_capture_hook" in completed.stdout From f55812dae29c16aabe5f250242c25e64b277adc1 Mon Sep 17 00:00:00 2001 From: Trecek Date: Thu, 23 Jul 2026 11:27:23 -0700 Subject: [PATCH 22/32] fix: align tests with capture review changes --- src/autoskillit/hooks/_capture_artifacts.py | 7 ++----- src/autoskillit/hooks/shell_capture_hook.py | 9 +++++---- tests/execution/backends/conftest.py | 16 +++++++++------ .../backends/test_hook_deny_efficacy_probe.py | 2 +- .../backends/test_hook_strength_matrix.py | 20 ++++++++++++------- tests/hooks/test_capture_artifacts.py | 2 +- tests/hooks/test_shell_capture_hook.py | 4 ++-- 7 files changed, 34 insertions(+), 26 deletions(-) diff --git a/src/autoskillit/hooks/_capture_artifacts.py b/src/autoskillit/hooks/_capture_artifacts.py index 020681c1a..64bfd9f41 100644 --- a/src/autoskillit/hooks/_capture_artifacts.py +++ b/src/autoskillit/hooks/_capture_artifacts.py @@ -27,12 +27,8 @@ if _HOOKS_DIR not in sys.path: sys.path.insert(0, _HOOKS_DIR) -from _capture_contract import ( # type: ignore[import-not-found] # noqa: E402 - _CAPTURE_ID_RE, - _MAX_COMMAND_BYTES, -) - if TYPE_CHECKING: + from autoskillit.hooks._capture_contract import _CAPTURE_ID_RE, _MAX_COMMAND_BYTES from autoskillit.hooks._hook_settings import ( HOOK_CONFIG_FILENAME, HOOK_CONFIG_OVERLAY_FILENAME, @@ -40,6 +36,7 @@ ) from autoskillit.hooks._policy_event import PolicyEvent, render_capture_marker else: + from _capture_contract import _CAPTURE_ID_RE, _MAX_COMMAND_BYTES from _hook_settings import ( HOOK_CONFIG_FILENAME, HOOK_CONFIG_OVERLAY_FILENAME, diff --git a/src/autoskillit/hooks/shell_capture_hook.py b/src/autoskillit/hooks/shell_capture_hook.py index 152a0b891..c0665efc0 100644 --- a/src/autoskillit/hooks/shell_capture_hook.py +++ b/src/autoskillit/hooks/shell_capture_hook.py @@ -18,16 +18,17 @@ import struct import sys from pathlib import Path +from typing import TYPE_CHECKING 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 _capture_contract import ( # type: ignore[import-not-found] # noqa: E402 - _CAPTURE_ID_RE, - _MAX_COMMAND_BYTES, -) +if TYPE_CHECKING: + from autoskillit.hooks._capture_contract import _CAPTURE_ID_RE, _MAX_COMMAND_BYTES +else: + from _capture_contract import _CAPTURE_ID_RE, _MAX_COMMAND_BYTES _HARNESS_SENTINEL = "# autoskillit-shell-capture v1" _ARG_MAX_FALLBACK_BYTES = 128 * 1024 diff --git a/tests/execution/backends/conftest.py b/tests/execution/backends/conftest.py index c5840173c..31cd23e4a 100644 --- a/tests/execution/backends/conftest.py +++ b/tests/execution/backends/conftest.py @@ -110,6 +110,7 @@ def _compute_expected_non_inert() -> int: _probe_rows: list[dict] = [] _controller_rows: list[dict] = [] _controller_probe_suite_collected = False +_probe_suite_collected = False def record_probe_row(row: dict) -> None: @@ -158,6 +159,13 @@ def _full_probe_suite_collected(items: list[pytest.Item]) -> bool: return probe_count == EXPECTED_TOTAL_PROBE_COUNT +def pytest_collection_finish(session: pytest.Session) -> None: + """Record full probe collection before xdist assigns subsets to workers.""" + + global _probe_suite_collected + _probe_suite_collected = _full_probe_suite_collected(session.items) + + def pytest_testnodedown(node, error): """Controller-side: harvest rows from a finishing worker.""" global _controller_probe_suite_collected @@ -173,9 +181,7 @@ def pytest_sessionfinish(session, exitstatus): if hasattr(session.config, "workerinput"): # xdist worker: send accumulated rows to controller via workeroutput IPC. session.config.workeroutput["hook_probe_rows"] = _probe_rows - session.config.workeroutput["hook_probe_suite_collected"] = _full_probe_suite_collected( - session.items - ) + session.config.workeroutput["hook_probe_suite_collected"] = _probe_suite_collected return # Controller (or non-xdist run): write matrix JSON. all_rows = _controller_rows or _probe_rows @@ -189,9 +195,7 @@ def pytest_sessionfinish(session, exitstatus): json.dumps({"combinations": all_rows}, indent=2), encoding="utf-8", ) - full_probe_suite = _controller_probe_suite_collected or _full_probe_suite_collected( - session.items - ) + full_probe_suite = _controller_probe_suite_collected or _probe_suite_collected if full_probe_suite and (failures := validate_strength_matrix(all_rows)): reporter = session.config.pluginmanager.get_plugin("terminalreporter") if reporter is not None: diff --git a/tests/execution/backends/test_hook_deny_efficacy_probe.py b/tests/execution/backends/test_hook_deny_efficacy_probe.py index 16c73de75..0bf1043fb 100644 --- a/tests/execution/backends/test_hook_deny_efficacy_probe.py +++ b/tests/execution/backends/test_hook_deny_efficacy_probe.py @@ -367,7 +367,7 @@ def test_shell_capture_hook_is_input_rewrite_and_excluded_from_deny_matrix( assert hook_output["permissionDecision"] == "allow" updated_command = hook_output["updatedInput"]["command"] assert "autoskillit-shell-capture" in updated_command - assert _OUTPUT_BUDGET_PROBE_COMMAND not in updated_command + assert _OUTPUT_BUDGET_PROBE_COMMAND in updated_command argv = shlex.split(updated_command.splitlines()[-1]) runner_index = next( diff --git a/tests/execution/backends/test_hook_strength_matrix.py b/tests/execution/backends/test_hook_strength_matrix.py index 84ac88632..4c1fa3056 100644 --- a/tests/execution/backends/test_hook_strength_matrix.py +++ b/tests/execution/backends/test_hook_strength_matrix.py @@ -30,8 +30,14 @@ ) -def _load_matrix() -> dict: +def _load_matrix(request: pytest.FixtureRequest) -> dict: """Load the serialized strength matrix, skipping if absent.""" + probe_is_collected = any( + Path(str(item.path)).name == "test_hook_deny_efficacy_probe.py" + for item in request.session.items + ) + if probe_is_collected: + pytest.skip("current probe matrix is validated at session finish") if not _MATRIX_PATH.exists(): pytest.skip("probe suite was not run — matrix JSON absent") return json.loads(_MATRIX_PATH.read_text(encoding="utf-8")) @@ -44,23 +50,23 @@ def test_completed_matrix_validator_rejects_missing_rows() -> None: assert any("works-as-is hooks" in failure for failure in failures) -def test_matrix_combination_count() -> None: +def test_matrix_combination_count(request: pytest.FixtureRequest) -> None: """The matrix must contain exactly EXPECTED_NON_INERT_COMBINATIONS rows. Inert probes return early without calling ``record_probe_row``, so the matrix holds only non-inert combinations. """ - matrix = _load_matrix() + matrix = _load_matrix(request) assert len(matrix["combinations"]) == EXPECTED_NON_INERT_COMBINATIONS -def test_works_as_is_hooks_have_soft_or_better() -> None: +def test_works_as_is_hooks_have_soft_or_better(request: pytest.FixtureRequest) -> None: """For each works-as-is hook, at least one matrix row must be soft or hard. A "works-as-is" hook claims codex parity — its guard must produce a non-inert outcome (``soft`` or ``hard``) for at least one matrix row. """ - matrix = _load_matrix() + matrix = _load_matrix(request) rows = matrix["combinations"] works_as_is_stems: set[str] = { @@ -80,9 +86,9 @@ def test_works_as_is_hooks_have_soft_or_better() -> None: assert not missing, f"works-as-is hooks missing soft/hard rows: {missing}" -def test_not_applicable_hooks_appear_only_as_inert() -> None: +def test_not_applicable_hooks_appear_only_as_inert(request: pytest.FixtureRequest) -> None: """Not-applicable hooks must be absent from the matrix (inert probes are not recorded).""" - matrix = _load_matrix() + matrix = _load_matrix(request) rows = matrix["combinations"] not_applicable_stems: set[str] = { diff --git a/tests/hooks/test_capture_artifacts.py b/tests/hooks/test_capture_artifacts.py index 4214ba41d..2327ec574 100644 --- a/tests/hooks/test_capture_artifacts.py +++ b/tests/hooks/test_capture_artifacts.py @@ -849,7 +849,6 @@ def unexpected_spawn(*_args, **_kwargs): captured = capfd.readouterr() artifact_path = _capture_dir(project) / f"shell_{_CAPTURE_ID}.log" assert "CAPTURE_FAILED" in captured.err - assert "OSError: fault injection during readback" in captured.err assert "SHELL_OUTPUT_CAPTURED" not in captured.out + captured.err assert not (project / "command_ran").exists() assert artifact_path.read_bytes() == b"" @@ -1098,6 +1097,7 @@ def close(self): captured = capfd.readouterr() artifact_path = _capture_dir(project) / f"shell_{_CAPTURE_ID}.log" assert "CAPTURE_FAILED" in captured.err + assert "OSError: fault injection during readback" in captured.err assert "SHELL_OUTPUT_CAPTURED" not in captured.out + captured.err assert artifact_path.read_bytes() == b"partial-output" assert process.terminated diff --git a/tests/hooks/test_shell_capture_hook.py b/tests/hooks/test_shell_capture_hook.py index 3527c4a3c..f3f82063f 100644 --- a/tests/hooks/test_shell_capture_hook.py +++ b/tests/hooks/test_shell_capture_hook.py @@ -51,8 +51,8 @@ def _updated_command(output: str) -> str: def _runner_argv(command: str) -> list[str]: lines = command.splitlines() assert lines[0] == _SENTINEL - assert len(lines) == 2 - return shlex.split(lines[1]) + assert len(lines) >= 2 + return shlex.split(lines[-1]) def _transported_command(command: str) -> str: From 6568403cfccc1d8e00c7db9135c54679d293a797 Mon Sep 17 00:00:00 2001 From: Trecek Date: Thu, 23 Jul 2026 11:32:23 -0700 Subject: [PATCH 23/32] fix: update remaining policy visibility assertions --- tests/hooks/test_shell_capture_hook.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/tests/hooks/test_shell_capture_hook.py b/tests/hooks/test_shell_capture_hook.py index f3f82063f..3e5d4e5d9 100644 --- a/tests/hooks/test_shell_capture_hook.py +++ b/tests/hooks/test_shell_capture_hook.py @@ -80,7 +80,7 @@ def test_rewrites_on_codex_env(monkeypatch): command = payload["hookSpecificOutput"]["updatedInput"]["command"] assert _SENTINEL in command assert _transported_command(command) == "echo hello" - assert "echo hello" not in command + assert "echo hello" in command def test_rewrites_on_turn_id_payload(monkeypatch): @@ -101,7 +101,7 @@ def test_always_wraps_sentinel_prefixed_command(monkeypatch): output = _run_hook(_build_event(already_wrapped), monkeypatch, env_backend="codex") command = _updated_command(output) - assert command.count(_SENTINEL) == 1 + assert command.splitlines()[0] == _SENTINEL assert _transported_command(command) == already_wrapped From dc07baa0d47416a6a7db1299f9c6d6538890cb4c Mon Sep 17 00:00:00 2001 From: Trecek Date: Thu, 23 Jul 2026 12:14:13 -0700 Subject: [PATCH 24/32] fix(review): make capture authorities factory-only --- src/autoskillit/hooks/_capture_artifacts.py | 39 ++++++++++++++++----- tests/hooks/test_capture_artifacts.py | 30 ++++++++++++++++ 2 files changed, 61 insertions(+), 8 deletions(-) diff --git a/src/autoskillit/hooks/_capture_artifacts.py b/src/autoskillit/hooks/_capture_artifacts.py index 64bfd9f41..4cba39bc3 100644 --- a/src/autoskillit/hooks/_capture_artifacts.py +++ b/src/autoskillit/hooks/_capture_artifacts.py @@ -18,7 +18,7 @@ import subprocess import sys import time -from dataclasses import dataclass +from dataclasses import InitVar, dataclass from enum import Enum from pathlib import Path from typing import TYPE_CHECKING @@ -82,6 +82,7 @@ UnicodeError, ValueError, ) +_AUTHORITY_FACTORY_TOKEN = object() _DIRECTORY_FLAGS = ( os.O_RDONLY @@ -119,7 +120,7 @@ def from_stat(cls, value: os.stat_result) -> FileIdentity: return cls(device=value.st_dev, inode=value.st_ino) -@dataclass(slots=True) +@dataclass(frozen=True, slots=True) class ProjectAnchor: """Opened project directory and its post-open physical-path hint.""" @@ -127,14 +128,19 @@ class ProjectAnchor: identity: FileIdentity supplied_path: str physical_path: Path + _factory_token: InitVar[object | None] = None + + def __post_init__(self, _factory_token: object | None) -> None: + if _factory_token is not _AUTHORITY_FACTORY_TOKEN: + raise CaptureSetupError("ProjectAnchor must be created by open_project_anchor") def close(self) -> None: if self.fd >= 0: os.close(self.fd) - self.fd = -1 + object.__setattr__(self, "fd", -1) -@dataclass(slots=True) +@dataclass(frozen=True, slots=True) class CaptureRoot: """Opened capture-root chain retained for the capture lifetime.""" @@ -144,27 +150,37 @@ class CaptureRoot: autoskillit_identity: FileIdentity temp_identity: FileIdentity identity: FileIdentity + _factory_token: InitVar[object | None] = None + + def __post_init__(self, _factory_token: object | None) -> None: + if _factory_token is not _AUTHORITY_FACTORY_TOKEN: + raise CaptureSetupError("CaptureRoot must be created by open_capture_root") def close(self) -> None: for field_name in ("fd", "temp_fd", "autoskillit_fd"): fd = getattr(self, field_name) if fd >= 0: os.close(fd) - setattr(self, field_name, -1) + object.__setattr__(self, field_name, -1) -@dataclass(slots=True) +@dataclass(frozen=True, slots=True) class CaptureArtifact: """Exclusive capture artifact retained by descriptor.""" fd: int name: str identity: FileIdentity + _factory_token: InitVar[object | None] = None + + def __post_init__(self, _factory_token: object | None) -> None: + if _factory_token is not _AUTHORITY_FACTORY_TOKEN: + raise CaptureSetupError("CaptureArtifact must be created by create_capture_artifact") def close(self) -> None: if self.fd >= 0: os.close(self.fd) - self.fd = -1 + object.__setattr__(self, "fd", -1) @dataclass(frozen=True, slots=True) @@ -279,6 +295,7 @@ def open_project_anchor(cwd: str) -> ProjectAnchor: identity=FileIdentity.from_stat(anchor_stat), supplied_path=cwd, physical_path=physical_path, + _factory_token=_AUTHORITY_FACTORY_TOKEN, ) except BaseException: os.close(fd) @@ -307,6 +324,7 @@ def open_capture_root(anchor: ProjectAnchor, *, create: bool) -> CaptureRoot: autoskillit_identity=_identity(autoskillit_fd), temp_identity=_identity(temp_fd), identity=_identity(capture_fd), + _factory_token=_AUTHORITY_FACTORY_TOKEN, ) except BaseException: for fd in reversed(opened): @@ -328,7 +346,12 @@ def create_capture_artifact(root: CaptureRoot, capture_id: str) -> CaptureArtifa value = os.fstat(fd) if not stat.S_ISREG(value.st_mode) or value.st_nlink != 1 or value.st_mode & stat.S_IWOTH: raise CaptureSetupError("unsafe capture artifact") - return CaptureArtifact(fd=fd, name=name, identity=FileIdentity.from_stat(value)) + return CaptureArtifact( + fd=fd, + name=name, + identity=FileIdentity.from_stat(value), + _factory_token=_AUTHORITY_FACTORY_TOKEN, + ) except BaseException: os.close(fd) raise diff --git a/tests/hooks/test_capture_artifacts.py b/tests/hooks/test_capture_artifacts.py index 2327ec574..de5bb4c20 100644 --- a/tests/hooks/test_capture_artifacts.py +++ b/tests/hooks/test_capture_artifacts.py @@ -9,6 +9,7 @@ import subprocess import sys import time +from dataclasses import FrozenInstanceError from pathlib import Path import pytest @@ -46,6 +47,35 @@ def _open_authority(project: Path): return anchor, root +def test_capture_authorities_are_factory_only_and_externally_immutable(tmp_path: Path) -> None: + identity = capture_artifacts.FileIdentity(device=1, inode=2) + with pytest.raises(CaptureSetupError, match="open_project_anchor"): + capture_artifacts.ProjectAnchor( + fd=-1, + identity=identity, + supplied_path=str(tmp_path), + physical_path=tmp_path, + ) + with pytest.raises(CaptureSetupError, match="open_capture_root"): + capture_artifacts.CaptureRoot( + autoskillit_fd=-1, + temp_fd=-1, + fd=-1, + autoskillit_identity=identity, + temp_identity=identity, + identity=identity, + ) + with pytest.raises(CaptureSetupError, match="create_capture_artifact"): + capture_artifacts.CaptureArtifact(fd=-1, name="shell.log", identity=identity) + + anchor = open_project_anchor(str(tmp_path)) + try: + with pytest.raises(FrozenInstanceError): + setattr(anchor, "supplied_path", "/unvalidated") + finally: + anchor.close() + + class _ReadableStream: def __init__(self, value: bytes) -> None: self._value = value From c382f446c9a76f475bdecfc18a8c4d50eb9ec0d9 Mon Sep 17 00:00:00 2001 From: Trecek Date: Thu, 23 Jul 2026 12:14:52 -0700 Subject: [PATCH 25/32] fix(review): preserve native bash command name --- src/autoskillit/hooks/_capture_artifacts.py | 2 +- tests/hooks/test_capture_artifacts.py | 12 ++++++++++++ 2 files changed, 13 insertions(+), 1 deletion(-) diff --git a/src/autoskillit/hooks/_capture_artifacts.py b/src/autoskillit/hooks/_capture_artifacts.py index 4cba39bc3..a9ee6a5ce 100644 --- a/src/autoskillit/hooks/_capture_artifacts.py +++ b/src/autoskillit/hooks/_capture_artifacts.py @@ -541,7 +541,7 @@ def _spawn_bash( try: os.fchdir(anchor.fd) process = subprocess.Popen( - [bash_path, "-c", _wrap_user_command(command), "autoskillit-capture"], + [bash_path, "-c", _wrap_user_command(command)], stdout=subprocess.PIPE if capture_output else None, stderr=subprocess.STDOUT if capture_output else None, close_fds=True, diff --git a/tests/hooks/test_capture_artifacts.py b/tests/hooks/test_capture_artifacts.py index de5bb4c20..8cc0cb086 100644 --- a/tests/hooks/test_capture_artifacts.py +++ b/tests/hooks/test_capture_artifacts.py @@ -820,6 +820,18 @@ def test_verified_disabled_policy_runs_without_capture( assert not _capture_dir(project).exists() +def test_capture_preserves_native_bash_command_name( + tmp_path: Path, capfd: pytest.CaptureFixture[str] +) -> None: + project = tmp_path / "project" + project.mkdir() + + assert run_capture('printf "%s" "$0"', str(project), _CAPTURE_ID) == 0 + + captured = capfd.readouterr() + assert captured.out == capture_artifacts._resolve_bash() + + def test_spawn_failure_closes_created_artifact_fd( tmp_path: Path, monkeypatch: pytest.MonkeyPatch, From de7d8e013351e58144f2ead1e8a16503d9b7c061 Mon Sep 17 00:00:00 2001 From: Trecek Date: Thu, 23 Jul 2026 12:15:45 -0700 Subject: [PATCH 26/32] fix(review): verify captured artifact bytes before publication --- src/autoskillit/hooks/_capture_artifacts.py | 31 +++++++++++++++++++++ tests/hooks/test_capture_artifacts.py | 28 +++++++++++++++++++ 2 files changed, 59 insertions(+) diff --git a/src/autoskillit/hooks/_capture_artifacts.py b/src/autoskillit/hooks/_capture_artifacts.py index a9ee6a5ce..fdf5136cc 100644 --- a/src/autoskillit/hooks/_capture_artifacts.py +++ b/src/autoskillit/hooks/_capture_artifacts.py @@ -228,6 +228,7 @@ def _require_capabilities() -> _CleanupDeletionMode: any(getattr(os, flag, 0) == 0 for flag in required_flags) or not hasattr(os, "fchdir") or not hasattr(os, "fstat") + or not hasattr(os, "pread") or any(function not in os.supports_dir_fd for function in required_dir_fd) or os.stat not in getattr(os, "supports_follow_symlinks", ()) or os.listdir not in getattr(os, "supports_fd", ()) @@ -624,6 +625,34 @@ def _drain_capture( ) +def _verify_capture_artifact(artifact: CaptureArtifact, result: _DrainResult) -> None: + """Verify persisted bytes through the retained artifact descriptor.""" + + value = os.fstat(artifact.fd) + if ( + FileIdentity.from_stat(value) != artifact.identity + or not stat.S_ISREG(value.st_mode) + or value.st_nlink != 1 + or value.st_size != result.total_bytes + ): + raise CaptureSetupError("capture artifact metadata changed") + + digest = hashlib.sha256() + offset = 0 + while offset < result.total_bytes: + chunk = os.pread( + artifact.fd, + min(_DRAIN_CHUNK_BYTES, result.total_bytes - offset), + offset, + ) + if not chunk: + raise CaptureSetupError("capture artifact readback ended early") + digest.update(chunk) + offset += len(chunk) + if digest.hexdigest() != result.sha256: + raise CaptureSetupError("capture artifact content changed") + + def _normalized_returncode(returncode: int) -> int: return 128 + (-returncode) if returncode < 0 else returncode @@ -772,6 +801,8 @@ def run_capture(command: str, cwd: str, capture_id: str) -> int: returncode = _normalized_returncode(process.wait()) if result.write_error is not None: return _capture_failure_return("capture artifact write failed", returncode) + failure_stage = "capture artifact integrity verification" + _verify_capture_artifact(artifact, result) failure_stage = "capture marker verification" artifact_path = current_artifact_path_if_bound(anchor, root, artifact) failure_stage = "capture replay emission" diff --git a/tests/hooks/test_capture_artifacts.py b/tests/hooks/test_capture_artifacts.py index 8cc0cb086..a66ba81ff 100644 --- a/tests/hooks/test_capture_artifacts.py +++ b/tests/hooks/test_capture_artifacts.py @@ -1184,6 +1184,34 @@ def hexdigest(self) -> str: os.fstat(fd) +def test_artifact_content_tampering_prevents_capture_publication( + tmp_path: Path, + monkeypatch: pytest.MonkeyPatch, + capfd: pytest.CaptureFixture[str], +) -> None: + project = tmp_path / "project" + project.mkdir() + process = _FakeCaptureProcess(b"captured-output") + real_drain = capture_artifacts._drain_capture + + def tamper_after_drain(process, artifact_writer_fd, inline_bytes): + result = real_drain(process, artifact_writer_fd, inline_bytes) + os.ftruncate(artifact_writer_fd, 0) + os.lseek(artifact_writer_fd, 0, os.SEEK_SET) + capture_artifacts._write_all(artifact_writer_fd, b"tampered") + return result + + monkeypatch.setattr(capture_artifacts, "_spawn_bash", lambda *_args, **_kwargs: process) + monkeypatch.setattr(capture_artifacts, "_drain_capture", tamper_after_drain) + + assert run_capture("printf output", str(project), _CAPTURE_ID) == 0 + captured = capfd.readouterr() + assert "CAPTURE_FAILED" in captured.err + assert "capture artifact integrity verification failed" in captured.err + assert "SHELL_OUTPUT_CAPTURED" not in captured.out + captured.err + assert captured.out == "" + + def test_success_marker_emission_failure_closes_resources_without_success( tmp_path: Path, monkeypatch: pytest.MonkeyPatch, From 9c5ebf4ee5986e57969d5a7e699e9539550ccc59 Mon Sep 17 00:00:00 2001 From: Trecek Date: Thu, 23 Jul 2026 12:17:39 -0700 Subject: [PATCH 27/32] fix(review): fail successful commands on capture integrity errors --- src/autoskillit/hooks/_capture_artifacts.py | 4 +++- tests/hooks/test_capture_artifacts.py | 14 +++++++------- 2 files changed, 10 insertions(+), 8 deletions(-) diff --git a/src/autoskillit/hooks/_capture_artifacts.py b/src/autoskillit/hooks/_capture_artifacts.py index fdf5136cc..f6a4b3e9e 100644 --- a/src/autoskillit/hooks/_capture_artifacts.py +++ b/src/autoskillit/hooks/_capture_artifacts.py @@ -678,7 +678,9 @@ def _capture_failure_return(detail: str, returncode: int | None) -> int: _emit_failure(detail) except _CAPTURE_RUNTIME_ERRORS: return _CAPTURE_FAILURE_RETURN_CODE - return returncode if returncode is not None else _CAPTURE_FAILURE_RETURN_CODE + if returncode is None or returncode == 0: + return _CAPTURE_FAILURE_RETURN_CODE + return returncode def _encode_marker_path(path: str) -> str: diff --git a/tests/hooks/test_capture_artifacts.py b/tests/hooks/test_capture_artifacts.py index a66ba81ff..0f138ca57 100644 --- a/tests/hooks/test_capture_artifacts.py +++ b/tests/hooks/test_capture_artifacts.py @@ -1095,7 +1095,7 @@ def record_artifact(root, capture_id): monkeypatch.setattr(capture_artifacts, "create_capture_artifact", record_artifact) monkeypatch.setattr(capture_artifacts, "_spawn_bash", lambda *_args, **_kwargs: process) - assert run_capture("printf output", str(project), _CAPTURE_ID) == 0 + assert run_capture("printf output", str(project), _CAPTURE_ID) == 1 captured = capfd.readouterr() assert "CAPTURE_FAILED" in captured.err assert "SHELL_OUTPUT_CAPTURED" not in captured.out + captured.err @@ -1134,7 +1134,7 @@ def close(self): process.stdout = PartialReadbackStream() monkeypatch.setattr(capture_artifacts, "_spawn_bash", lambda *_args, **_kwargs: process) - assert run_capture("printf output", str(project), _CAPTURE_ID) == 0 + assert run_capture("printf output", str(project), _CAPTURE_ID) == 1 captured = capfd.readouterr() artifact_path = _capture_dir(project) / f"shell_{_CAPTURE_ID}.log" @@ -1171,7 +1171,7 @@ def hexdigest(self) -> str: monkeypatch.setattr(capture_artifacts, "_spawn_bash", lambda *_args, **_kwargs: process) monkeypatch.setattr(capture_artifacts.hashlib, "sha256", BrokenDigest) - assert run_capture("printf output", str(project), _CAPTURE_ID) == 0 + assert run_capture("printf output", str(project), _CAPTURE_ID) == 1 captured = capfd.readouterr() assert "CAPTURE_FAILED" in captured.err assert "SHELL_OUTPUT_CAPTURED" not in captured.out + captured.err @@ -1204,7 +1204,7 @@ def tamper_after_drain(process, artifact_writer_fd, inline_bytes): monkeypatch.setattr(capture_artifacts, "_spawn_bash", lambda *_args, **_kwargs: process) monkeypatch.setattr(capture_artifacts, "_drain_capture", tamper_after_drain) - assert run_capture("printf output", str(project), _CAPTURE_ID) == 0 + assert run_capture("printf output", str(project), _CAPTURE_ID) == 1 captured = capfd.readouterr() assert "CAPTURE_FAILED" in captured.err assert "capture artifact integrity verification failed" in captured.err @@ -1228,7 +1228,7 @@ def fail_success_marker(*_args, **_kwargs) -> None: monkeypatch.setattr(capture_artifacts, "_spawn_bash", lambda *_args, **_kwargs: process) monkeypatch.setattr(capture_artifacts, "_emit_capture", fail_success_marker) - assert run_capture("printf output", str(project), _CAPTURE_ID) == 0 + assert run_capture("printf output", str(project), _CAPTURE_ID) == 1 captured = capfd.readouterr() assert "CAPTURE_FAILED" in captured.err assert "SHELL_OUTPUT_CAPTURED" not in captured.out + captured.err @@ -1275,7 +1275,7 @@ def fail_write(fd, data): monkeypatch.setattr(capture_artifacts, "_write_all", fail_write) - assert run_capture("printf ran > command_ran; printf output", str(project), _CAPTURE_ID) == 0 + assert run_capture("printf ran > command_ran; printf output", str(project), _CAPTURE_ID) == 1 captured = capfd.readouterr() assert (project / "command_ran").read_text() == "ran" assert "CAPTURE_FAILED" in captured.err @@ -1300,7 +1300,7 @@ def fail_verification(anchor, root, artifact): fail_verification, ) - assert run_capture("printf output", str(project), _CAPTURE_ID) == 0 + assert run_capture("printf output", str(project), _CAPTURE_ID) == 1 captured = capfd.readouterr() assert "CAPTURE_FAILED" in captured.err assert "SHELL_OUTPUT_CAPTURED" not in captured.out + captured.err From a9e01aa5b3e844341a49925d3ab31d717625aac4 Mon Sep 17 00:00:00 2001 From: Trecek Date: Thu, 23 Jul 2026 12:19:40 -0700 Subject: [PATCH 28/32] fix(review): enforce capture trust guards with AST analysis --- tests/arch/test_shell_capture_trust_anchor.py | 151 ++++++++++++++++-- 1 file changed, 138 insertions(+), 13 deletions(-) diff --git a/tests/arch/test_shell_capture_trust_anchor.py b/tests/arch/test_shell_capture_trust_anchor.py index fb521b9b3..1e08d435e 100644 --- a/tests/arch/test_shell_capture_trust_anchor.py +++ b/tests/arch/test_shell_capture_trust_anchor.py @@ -65,6 +65,100 @@ def _render_string_node(node: ast.Constant | ast.JoinedStr) -> str: return "".join(parts) +def _called_operation_name(node: ast.expr) -> str | None: + if isinstance(node, ast.Name): + return node.id + if isinstance(node, ast.Attribute): + return node.attr + if ( + isinstance(node, ast.Call) + and isinstance(node.func, ast.Name) + and node.func.id == "getattr" + and len(node.args) >= 2 + and isinstance(node.args[1], ast.Constant) + and isinstance(node.args[1].value, str) + ): + return node.args[1].value + return None + + +def _pathname_operation_calls(source: str) -> list[str]: + tree = ast.parse(source) + imported_aliases = { + alias.asname or alias.name + for node in ast.walk(tree) + if isinstance(node, ast.ImportFrom) and node.module in {"os", "pathlib", "shutil"} + for alias in node.names + if alias.name in {"unlink", "rmtree"} + } + violations: list[str] = [] + for node in ast.walk(tree): + if not isinstance(node, ast.Call): + continue + operation = _called_operation_name(node.func) + if operation in {"unlink", "rmtree"} or operation in imported_aliases: + violations.append(ast.unparse(node.func)) + return violations + + +def _shell_mkdir_calls(source: str) -> list[str]: + return [ + node.value + for node in ast.walk(ast.parse(source)) + if isinstance(node, ast.Constant) + and isinstance(node.value, str) + and re.search(r"(?:^|[\s;&|])mkdir\s+-p(?:\s|$)", node.value) + ] + + +def _imports_symbol(tree: ast.Module, module: str, symbol: str) -> bool: + return any( + isinstance(node, ast.ImportFrom) + and node.module == module + and any(alias.name == symbol for alias in node.names) + for node in tree.body + ) + + +def _is_path_cwd_call(node: ast.expr) -> bool: + return ( + isinstance(node, ast.Call) + and isinstance(node.func, ast.Attribute) + and isinstance(node.func.value, ast.Name) + and node.func.value.id == "Path" + and node.func.attr == "cwd" + and not node.args + and not node.keywords + ) + + +def _main_directly_classifies_captures(tree: ast.Module) -> bool: + main = next( + ( + node + for node in tree.body + if isinstance(node, (ast.FunctionDef, ast.AsyncFunctionDef)) and node.name == "main" + ), + None, + ) + if main is None: + return False + for statement in main.body: + if not isinstance(statement, ast.Try): + continue + for guarded in statement.body: + if ( + isinstance(guarded, ast.Expr) + and isinstance(guarded.value, ast.Call) + and isinstance(guarded.value.func, ast.Name) + and guarded.value.func.id == "classify_stale_captures" + and len(guarded.value.args) == 1 + and _is_path_cwd_call(guarded.value.args[0]) + ): + return True + return False + + def _capture_path_redirections(source: str) -> list[str]: tree = ast.parse(source) capture_path_names = { @@ -109,9 +203,16 @@ def _capture_path_redirections(source: str) -> list[str]: def test_session_start_calls_canonical_capture_classification() -> None: source = _source("hooks/session_start_hook.py") - assert "from _capture_artifacts import" in source - assert "classify_stale_captures(Path.cwd())" in source - assert "shell_capture" not in source + tree = ast.parse(source) + assert _imports_symbol(tree, "_capture_artifacts", "classify_stale_captures") + assert _main_directly_classifies_captures(tree) + imported_modules: set[str] = set() + for node in ast.walk(tree): + if isinstance(node, ast.ImportFrom) and node.module is not None: + imported_modules.add(node.module) + elif isinstance(node, ast.Import): + imported_modules.update(alias.name for alias in node.names) + assert not any("shell_capture" in module for module in imported_modules) def test_shell_capture_code_has_no_pathname_harness_or_cleanup() -> None: @@ -122,20 +223,44 @@ def test_shell_capture_code_has_no_pathname_harness_or_cleanup() -> None: "hooks/_capture_artifacts.py", ) } - combined = "\n".join(sources.values()) - for forbidden in ( - "mkdir -p", - ".unlink(", - "os.unlink(", - "shutil.rmtree(", - ): - assert forbidden not in combined, f"pathname capture operation reintroduced: {forbidden}" violations = {} for relative, source in sources.items(): - detected = _capture_path_redirections(source) + detected = _pathname_operation_calls(source) + _shell_mkdir_calls(source) if detected: violations[relative] = detected - assert not violations, f"capture-root pathname redirection reintroduced: {violations}" + assert not violations, f"pathname capture operation reintroduced: {violations}" + redirection_violations = {} + for relative, source in sources.items(): + detected = _capture_path_redirections(source) + if detected: + redirection_violations[relative] = detected + assert not redirection_violations, ( + f"capture-root pathname redirection reintroduced: {redirection_violations}" + ) + + +@pytest.mark.parametrize( + "source", + [ + "import os as filesystem\nfilesystem.unlink('artifact')", + "from os import unlink as remove\nremove('artifact')", + "from pathlib import Path\ngetattr(Path('artifact'), 'unlink')()", + "import shutil as cleanup\ncleanup.rmtree('capture-root')", + ], +) +def test_pathname_operation_guard_rejects_aliases_and_getattr(source: str) -> None: + assert _pathname_operation_calls(source) + + +def test_canonical_classification_guard_rejects_dead_branch() -> None: + tree = ast.parse( + "from _capture_artifacts import classify_stale_captures\n" + "from pathlib import Path\n" + "def main():\n" + " if False:\n" + " classify_stale_captures(Path.cwd())\n" + ) + assert not _main_directly_classifies_captures(tree) @pytest.mark.parametrize( From f7ef69118d8ca822dc5687d7745c07de5a12d525 Mon Sep 17 00:00:00 2001 From: Trecek Date: Thu, 23 Jul 2026 12:20:20 -0700 Subject: [PATCH 29/32] fix(review): cover capture policy clamp boundaries --- tests/hooks/test_capture_artifacts.py | 23 ++++++++++++++++++++--- 1 file changed, 20 insertions(+), 3 deletions(-) diff --git a/tests/hooks/test_capture_artifacts.py b/tests/hooks/test_capture_artifacts.py index 0f138ca57..842fdffe5 100644 --- a/tests/hooks/test_capture_artifacts.py +++ b/tests/hooks/test_capture_artifacts.py @@ -408,7 +408,24 @@ def test_symlinked_policy_leaf_is_not_trusted(tmp_path: Path) -> None: anchor.close() -def test_verified_policy_merges_overlay_and_bounds_inline_bytes(tmp_path: Path) -> None: +@pytest.mark.parametrize( + ("configured", "expected"), + [ + pytest.param(1, 1, id="minimum-valid"), + pytest.param(31, 31, id="ordinary-valid"), + pytest.param(1_000_000, 1_000_000, id="maximum-valid"), + pytest.param(1_000_001, 1_000_000, id="clamped-above-maximum"), + pytest.param(0, CapturePolicy().inline_bytes, id="zero-defaults"), + pytest.param(-1, CapturePolicy().inline_bytes, id="negative-defaults"), + pytest.param(True, CapturePolicy().inline_bytes, id="boolean-defaults"), + pytest.param("31", CapturePolicy().inline_bytes, id="non-integer-defaults"), + ], +) +def test_verified_policy_merges_overlay_and_bounds_inline_bytes( + configured: object, + expected: int, + tmp_path: Path, +) -> None: project = tmp_path / "project" temp_dir = project.joinpath(*CAPTURE_PATH_COMPONENTS[:2]) temp_dir.mkdir(parents=True) @@ -417,7 +434,7 @@ def test_verified_policy_merges_overlay_and_bounds_inline_bytes(tmp_path: Path) { "output_budget_policy": { "disabled": False, - "shell_max_inline_bytes": 31, + "shell_max_inline_bytes": configured, } } ) @@ -428,7 +445,7 @@ def test_verified_policy_merges_overlay_and_bounds_inline_bytes(tmp_path: Path) anchor = open_project_anchor(str(project)) try: - assert read_capture_policy(anchor) == CapturePolicy(disabled=True, inline_bytes=31) + assert read_capture_policy(anchor) == CapturePolicy(disabled=True, inline_bytes=expected) finally: anchor.close() From 7b8321b7f85c0c29d4e2f53aa4be105f262aaea5 Mon Sep 17 00:00:00 2001 From: Trecek Date: Thu, 23 Jul 2026 14:41:05 -0700 Subject: [PATCH 30/32] fix(review): link shell capture retention debt --- docs/decisions/0006-output-containment.md | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/docs/decisions/0006-output-containment.md b/docs/decisions/0006-output-containment.md index 003803057..7657c7f16 100644 --- a/docs/decisions/0006-output-containment.md +++ b/docs/decisions/0006-output-containment.md @@ -90,7 +90,9 @@ hook can be retired in favor of that mechanism. 9. Portable Linux/macOS Python exposes descriptor-relative unlink but no expected-inode conditional unlink. SessionStart therefore retains stale candidates rather than making a security claim across a validation/deletion - race. Artifact quota and lifecycle reclamation remain follow-up work. + race. Artifact quota and lifecycle reclamation remain follow-up work tracked + by [#4327](https://github.com/TalonT-Org/AutoSkillit/issues/4327) and + [#4320](https://github.com/TalonT-Org/AutoSkillit/issues/4320), respectively. ## Consequences From 3b9e1f0b2e2434f9870cbda854bc2439d49fabd4 Mon Sep 17 00:00:00 2001 From: Trecek Date: Thu, 23 Jul 2026 14:41:21 -0700 Subject: [PATCH 31/32] fix(review): assert generated hook exits cleanly --- tests/hooks/test_shell_capture_conformance.py | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/tests/hooks/test_shell_capture_conformance.py b/tests/hooks/test_shell_capture_conformance.py index 5cc549008..fc7f723e2 100644 --- a/tests/hooks/test_shell_capture_conformance.py +++ b/tests/hooks/test_shell_capture_conformance.py @@ -133,8 +133,9 @@ def _main_generated_wrapper(command: str, cwd: Path, monkeypatch: pytest.MonkeyP monkeypatch.setenv("AUTOSKILLIT_AGENT_BACKEND", "codex") monkeypatch.setattr("sys.stdin", io.StringIO(json.dumps(event))) output = io.StringIO() - with pytest.raises(SystemExit), redirect_stdout(output): + with pytest.raises(SystemExit) as exit_info, redirect_stdout(output): shell_capture_hook.main() + assert exit_info.value.code == 0 payload = json.loads(output.getvalue()) return payload["hookSpecificOutput"]["updatedInput"]["command"] From c5a2503a06e9230cc3c2bc12dd94fb6410c4f02a Mon Sep 17 00:00:00 2001 From: Trecek Date: Thu, 23 Jul 2026 14:42:19 -0700 Subject: [PATCH 32/32] fix(review): exercise complete exec footprint limit --- tests/hooks/test_shell_capture_hook.py | 52 +++++++++++++++++++++++--- 1 file changed, 47 insertions(+), 5 deletions(-) diff --git a/tests/hooks/test_shell_capture_hook.py b/tests/hooks/test_shell_capture_hook.py index 3e5d4e5d9..aafda97f2 100644 --- a/tests/hooks/test_shell_capture_hook.py +++ b/tests/hooks/test_shell_capture_hook.py @@ -193,17 +193,59 @@ def test_arg_max_exhaustion_builds_nonexecuting_rejection( ) -> None: import autoskillit.hooks.shell_capture_hook as shell_capture_hook - monkeypatch.setattr(shell_capture_hook.os, "sysconf", lambda _name: 1) + command = "printf must-not-run" + cwd = "/abs/project" + capture_id = "0123456789abcdef" + encoded = base64.b64encode(command.encode()).decode("ascii") + runner_argv = [ + sys.executable, + "-I", + str(shell_capture_hook._runner_path()), + "run", + encoded, + cwd, + capture_id, + ] + preflight_harness = shell_capture_hook._render_harness( + runner_argv, + policy_command=command, + ) + argv_candidates = (runner_argv, ["bash", "-c", preflight_harness]) + + def _argv_bytes(argv: list[str]) -> int: + return sum(len(shell_capture_hook.os.fsencode(argument)) + 1 for argument in argv) + + environment_bytes = sum( + len(shell_capture_hook.os.fsencode(key)) + len(shell_capture_hook.os.fsencode(value)) + 2 + for key, value in shell_capture_hook.os.environ.items() + ) + pointer_size = shell_capture_hook.struct.calcsize("P") + for argv in argv_candidates: + pointer_bytes = (len(argv) + len(shell_capture_hook.os.environ) + 2) * pointer_size + assert shell_capture_hook._exec_footprint(argv) == ( + _argv_bytes(argv) + environment_bytes + pointer_bytes + ) + + arg_max = ( + max(shell_capture_hook._exec_footprint(argv) for argv in argv_candidates) + + shell_capture_hook._ARG_MAX_HEADROOM_BYTES + - 1 + ) + assert all( + _argv_bytes(argv) + shell_capture_hook._ARG_MAX_HEADROOM_BYTES <= arg_max + for argv in argv_candidates + ) + monkeypatch.setattr(shell_capture_hook.os, "sysconf", lambda _name: arg_max) harness = shell_capture_hook._build_harness( - "printf must-not-run", - "/abs/project", - "0123456789abcdef", + command, + cwd, + capture_id, ) argv = _runner_argv(harness) assert argv[3] == "reject" - assert "printf must-not-run" not in harness + assert command not in harness def test_marker_provenance_is_emitted_by_runner(monkeypatch, tmp_path):