diff --git a/Dockerfile b/Dockerfile index 2642f0c9..220c39c6 100644 --- a/Dockerfile +++ b/Dockerfile @@ -9,6 +9,14 @@ RUN pip install --no-cache-dir . # Copy application source COPY src/ src/ COPY ui/ ui/ + +# Working directory for local user commands (tools.local_working_dir). +# Deliberately OUTSIDE the install root (/app here) and outside the data dir: +# local commands run here so a bare relative path — e.g. cleaning up after +# extracting an archive whose internal layout starts with data/ — cannot +# resolve against the live install. Validation fails closed if it is missing +# or not 0700, so this must exist in the image. +RUN mkdir -p /var/lib/odin-workspace && chmod 0700 /var/lib/odin-workspace COPY config.yml . CMD ["python", "-m", "src"] diff --git a/README.md b/README.md index 4e44f3d7..e0163cd2 100644 --- a/README.md +++ b/README.md @@ -117,6 +117,17 @@ playwright install chromium # optional — enables browser_* tools cp .env.example .env # set DISCORD_TOKEN $EDITOR config.yml # hosts, LLM, permissions + +# Local commands run in a workspace OUTSIDE the install, so a bare relative +# path (e.g. cleaning up after extracting an archive whose internal layout +# starts with `data/`) can never resolve against your live data. It must +# exist, be private, and be owned by you — validation fails closed rather +# than silently falling back to the install directory. +sudo install -d -m 0700 -o "$USER" -g "$(id -gn)" /var/lib/odin-workspace +# …or point `tools.local_working_dir` in config.yml at any private directory +# outside the checkout, e.g.: +# mkdir -p -m 0700 ~/.local/share/odin-workspace + python -m src ``` diff --git a/config.yml b/config.yml index 480df65b..97f3f9de 100644 --- a/config.yml +++ b/config.yml @@ -92,6 +92,15 @@ sessions: tools: enabled: true + # Working directory for local user commands (run_command, run_script, + # run_command_multi, background processes, skill run_on_host). A bare + # relative path in a command resolves HERE, not in the install directory — + # which is what stopped an extracted archive's `rm -rf data` from deleting + # Odin's own live state. Must be an absolute path, owned by the service + # account, mode 0700, and outside the install and data directories. + # Provisioned automatically on start; if it is unusable, local commands + # refuse to run rather than falling back to the install directory. + local_working_dir: /var/lib/odin-workspace # Customize SSH key path for your deployment ssh_key_path: /app/.ssh/id_ed25519 # Customize SSH known hosts path for your deployment diff --git a/docker-compose.yml b/docker-compose.yml index dbb08ed8..35d6d404 100644 --- a/docker-compose.yml +++ b/docker-compose.yml @@ -10,6 +10,11 @@ services: volumes: - ./config.yml:/app/config.yml:ro - ./data:/app/data + # NAMED volume, not a bind mount: Docker initializes a named volume from + # the image directory, preserving the 0700 ownership the resolver + # requires. A fresh bind mount is created 755 root:root, which fails + # closed and costs local-command capability on first run (PR #239 r2). + - odin-workspace:/var/lib/odin-workspace - ./data/context:/app/data/context:ro - ./ssh:/app/.ssh:ro extra_hosts: @@ -57,3 +62,8 @@ services: capabilities: [gpu] networks: - default + +volumes: + # Local command workspace (tools.local_working_dir). Initialized from the + # image's 0700 directory so a fresh deployment satisfies the resolver. + odin-workspace: diff --git a/packaging/odin.service b/packaging/odin.service index df1c0121..f2f923ad 100644 --- a/packaging/odin.service +++ b/packaging/odin.service @@ -9,6 +9,15 @@ Type=simple User=odin Group=odin WorkingDirectory=/opt/odin +# Local command workspace (tools.local_working_dir). systemd creates +# /var/lib/odin-workspace on EVERY start, owned by User= with mode 0700, so +# fresh installs, package upgrades, and restarts after a self-update all get a +# valid workspace with no manual step. Local commands run there instead of the +# install directory, so a bare relative path — e.g. cleaning up after +# extracting an archive whose internal layout starts with data/ — cannot +# resolve against live data. +StateDirectory=odin-workspace +StateDirectoryMode=0700 ExecStart=/opt/odin/.venv/bin/python -m src EnvironmentFile=/etc/odin/.env Restart=always diff --git a/packaging/postinstall.sh b/packaging/postinstall.sh index da3189c9..205cef46 100755 --- a/packaging/postinstall.sh +++ b/packaging/postinstall.sh @@ -14,6 +14,10 @@ SERVICE_GROUP="odin" APP_DIR="/opt/odin" CONFIG_DIR="/etc/odin" DATA_DIR="/var/lib/odin" +# Working directory for local user commands (tools.local_working_dir). +# A SIBLING of DATA_DIR, never inside it: local commands run here so a bare +# relative path cannot resolve against the install or the live data. +WORKSPACE_DIR="/var/lib/odin-workspace" LOG_DIR="/var/log/odin" WEB_PORT="3000" # matches config.yml default web.port @@ -34,6 +38,7 @@ fi mkdir -p "$CONFIG_DIR" mkdir -p "$DATA_DIR"/{sessions,context,skills,search,knowledge,trajectories} mkdir -p "$LOG_DIR" +mkdir -p "$WORKSPACE_DIR" # Enable passwordless sudo for the odin user (Odin runs privileged host # operations; scope this down in /etc/sudoers.d/99-odin-passwordless if you @@ -99,6 +104,11 @@ fi # Set ownership and permissions chown -R "$SERVICE_USER:$SERVICE_GROUP" "$APP_DIR" "$DATA_DIR" "$LOG_DIR" +# Odin refuses to run local commands unless this is 0700 and owned by the +# service account — validation fails closed rather than falling back to the +# install directory, which is what allowed the 2026-07-27 data wipe. +chown "$SERVICE_USER:$SERVICE_GROUP" "$WORKSPACE_DIR" +chmod 0700 "$WORKSPACE_DIR" chown -R "$SERVICE_USER:$SERVICE_GROUP" "$CONFIG_DIR" chmod 600 "$CONFIG_DIR/.env" chown root:root /usr/lib/systemd/system/odin.service diff --git a/scripts/incus-deploy.sh b/scripts/incus-deploy.sh index 97a6bf57..999c3b7b 100755 --- a/scripts/incus-deploy.sh +++ b/scripts/incus-deploy.sh @@ -71,6 +71,12 @@ incus exec "$INSTANCE" -- bash -c " id odin &>/dev/null || useradd -m -s /bin/bash odin mkdir -p /app/src /app/data/context /app/data/sessions /app/data/logs \ /app/data/usage /app/data/skills /app/data/chromadb /app/.ssh + # Local command workspace (tools.local_working_dir). OUTSIDE /app so a bare + # relative path in a command cannot resolve against the install or its data. + # The unprivileged odin user cannot create it under /var/lib itself. + mkdir -p /var/lib/odin-workspace + chown odin:odin /var/lib/odin-workspace + chmod 0700 /var/lib/odin-workspace chown -R odin:odin /app chmod 700 /app/.ssh " @@ -118,6 +124,8 @@ After=network.target Type=simple User=odin WorkingDirectory=/app +StateDirectory=odin-workspace +StateDirectoryMode=0700 EnvironmentFile=/app/.env ExecStart=/usr/bin/python3 -m src Restart=on-failure diff --git a/src/__main__.py b/src/__main__.py index f5ce35c3..711420d8 100644 --- a/src/__main__.py +++ b/src/__main__.py @@ -32,6 +32,11 @@ def _wire_observability(health, bot, log) -> None: tool_executor = getattr(bot, "tool_executor", None) if tool_executor is not None and hasattr(tool_executor, "get_metrics"): metrics.register_source("tools", tool_executor.get_metrics) + if tool_executor is not None and hasattr(tool_executor, "get_workspace_metrics"): + # Paired with the deliberate absence of auto-pruning: growth in the + # local command workspace must be alertable rather than silently + # unbounded (PR #239 round-2 review). + metrics.register_source("workspace", tool_executor.get_workspace_metrics) cost_tracker = getattr(bot, "cost_tracker", None) if cost_tracker is not None and hasattr(cost_tracker, "get_prometheus_metrics"): @@ -108,6 +113,57 @@ def main() -> None: log = get_logger("main") log.info("Starting Odin") + # STARTUP MIGRATION — must run after the real configuration is loaded and + # before any command service begins. + # + # Local user commands run in a validated workspace outside the install and + # refuse to run without one. A preflight in the self-updater cannot + # bootstrap that: the update which INTRODUCES the preflight is executed by + # the previous release's handler, which has none, so the very first upgrade + # would re-exec into code whose workspace was never created (PR #239 + # round-5 review, verified against a live install). Provisioning here runs + # in the NEW code on the restart that follows any update, however the + # update arrived. + # + # Failure is logged, not fatal: an unusable workspace must not prevent + # Odin from starting and answering on Discord. Local commands then fail + # closed individually, with the same actionable error. + try: + from src.tools.workspace import ( + WorkspaceError, + provision_startup_workspace, + provisioning_hint, + ) + + def _warn_fallback(path, configured, reason) -> None: + # A fallback is not a failure, but it must never look like normal + # operation: on a packaged install it means the packaged default + # could not be provisioned, which the operator needs to know + # (cross-review of PR #239 round 13). + log.warning( + "Local command workspace fell back to %s — the configured " + "default could not be provisioned (%s). Local commands work, " + "but this indicates a packaging or permissions problem. %s", + path, + reason, + provisioning_hint(configured), + ) + + workspace = provision_startup_workspace( + config.tools, + protected_roots=_command_protected_roots(config), + on_fallback=_warn_fallback, + ) + log.info("Local command workspace ready: %s", workspace) + except WorkspaceError as exc: + log.error( + "Local command workspace unusable — local commands will refuse to run: %s. %s", + exc, + provisioning_hint(config.tools.local_working_dir), + ) + except Exception as exc: # never block startup on provisioning + log.error("Local command workspace provisioning failed unexpectedly: %s", exc) + health = HealthServer( port=config.web.port, webhook_config=config.webhook, @@ -273,5 +329,31 @@ async def shutdown() -> None: sys.exit(exit_code) +def _command_protected_roots(config) -> list[str]: + """Install root plus canonical live-data roots for the startup migration. + + Delegates to the ONE shared derivation so startup, the self-update + preflight, and the executor protect exactly the same directories. Deriving + them separately here silently protected nothing (``Config`` has no + ``memory`` section) while the executor protected live memory.json — so a + workspace beside it was provisioned at startup and then refused on every + command (PR #239 round-6 review). + + The FULL config is passed so every independently relocatable live-state + path is covered. ``memory_path`` is left at its default: production wiring + hardcodes that path, and startup runs before wiring exists. + """ + from src.tools.workspace import command_protected_roots + + return command_protected_roots(Path(__file__).absolute().parents[1], config) + + +# The entrypoint guard MUST stay the last statement in this module. Python +# executes a module top-to-bottom, so a guard placed above a helper runs main() +# before that helper's `def` is reached: the startup migration raised NameError +# and its own nonfatal handler swallowed it, leaving the workspace uncreated and +# resurrecting the first-update bootstrap failure this migration exists to fix +# (PR #239 round-6 review). tests/test_local_workspace.py executes `python -m src` +# for real to keep this honest. if __name__ == "__main__": main() diff --git a/src/config/schema.py b/src/config/schema.py index d5be54d5..861f20a1 100644 --- a/src/config/schema.py +++ b/src/config/schema.py @@ -187,6 +187,12 @@ class GovernorConfig(BaseModel): host_overrides: dict[str, str] = Field(default_factory=dict) +# The default local command workspace, spelled ONCE: the field default, the +# blank-value normalizer, the tracked config.yml template and the packaging +# scripts must never drift apart. +DEFAULT_LOCAL_WORKING_DIR = "/var/lib/odin-workspace" + + class ToolsConfig(BaseModel): enabled: bool = True governor: GovernorConfig = GovernorConfig() @@ -217,6 +223,45 @@ class ToolsConfig(BaseModel): # Loops typically need more budget for exploration + execution + verify + commit. max_tool_iterations_chat: int = 500 max_tool_iterations_loop: int = 500 + # Working directory for USER-COMMAND local execution (run_command, + # run_script, manage_process). Before this existed, those subprocesses + # inherited systemd's WorkingDirectory=/opt/odin, so a bare relative path + # in a command resolved against the live install — on 2026-07-27 an AE2 jar + # whose internal layout is `data/` was extracted and cleaned up with + # `rm -rf data`, which deleted /opt/odin/data. + # + # Deliberately a SIBLING of /var/lib/odin, not a child: packaged installs + # use /var/lib/odin as the live data directory behind /opt/odin/data. + # Not /tmp or /var/tmp (tmpfiles policy can age those out) and not $HOME + # (packaged Odin declares /opt/odin as the service account's home). + # + # Stable and persistent BY DESIGN: a fresh directory per command would + # break two-step workflows that write a relative file in one command and + # read it in the next, which would cost capability. Restart-required, not + # hot-reloadable — swapping workspaces at runtime would break exactly the + # cross-command continuity this preserves. + local_working_dir: str = DEFAULT_LOCAL_WORKING_DIR + + @field_validator("local_working_dir") + @classmethod + def _workspace_blank_means_default(cls, v): + """Blank or whitespace-only normalizes to the default, here at the + boundary, so every consumer sees the same value. + + The field accepts free strings and can be blanked through + PUT /api/config. Left un-normalized, the self-update preflight + substituted the default and approved, while the restarted process + loaded the blank value and failed closed on every local command — + preflight and runtime disagreeing about the very path being validated + (PR #239 round-7 review, reproduced). + + Normalizing rather than rejecting keeps the update seamless: a blanked + value costs no capability and cannot brick startup, which a hard + validation error on a persisted config would. + """ + if not isinstance(v, str) or not v.strip(): + return DEFAULT_LOCAL_WORKING_DIR + return v.strip() @field_validator("command_timeout_seconds") @classmethod @@ -866,6 +911,18 @@ def replacer(match: re.Match) -> str: # (a test or one-off script that never called load_config) cannot silently # overwrite a real deployment's config.yml from the wrong working directory. _ACTIVE_CONFIG_PATH: Path | None = None +# The path AS GIVEN (absolutized, symlinks intact). restart.reexec() replays +# sys.argv, so an alias like /etc/odin/config.yml -> /srv/real/odin.yml is what +# the restarted process opens — protecting only the canonical target would let +# a relative command delete the alias and break the next restart (PR #239 +# round-10 review, reproduced). +_LAUNCH_CONFIG_PATH: Path | None = None + + +def active_config_launch_path() -> Path | None: + """The config path as given on the command line, absolutized but with + symlinks intact — what ``restart.reexec()`` will hand the next process.""" + return _LAUNCH_CONFIG_PATH def active_config_path() -> Path | None: @@ -878,8 +935,9 @@ def set_active_config_path(path: str | Path | None) -> None: """Record (or clear) the active config path. ``load_config`` calls this on a successful load; tests/tools that persist a hand-built Config point it at their own file.""" - global _ACTIVE_CONFIG_PATH + global _ACTIVE_CONFIG_PATH, _LAUNCH_CONFIG_PATH _ACTIVE_CONFIG_PATH = Path(path).resolve() if path is not None else None + _LAUNCH_CONFIG_PATH = Path(os.path.abspath(path)) if path is not None else None def load_config(path: str | Path = "config.yml") -> Config: diff --git a/src/discord/wiring.py b/src/discord/wiring.py index 1e757f13..9352af49 100644 --- a/src/discord/wiring.py +++ b/src/discord/wiring.py @@ -46,6 +46,7 @@ from ..sessions import SessionManager from ..tools import SkillManager, ToolExecutor from ..tools.autonomous_loop import LoopManager +from ..tools.workspace import DEFAULT_MEMORY_PATH from ..trajectories.saver import TrajectorySaver from .channel_config import ChannelConfigManager from .channel_logger import ChannelLogger @@ -206,7 +207,10 @@ def build_services(config: Config) -> BotServices: # noqa: PLR0915 — linear c ) sessions.load() - memory_path = "./data/memory.json" + # Imported, not re-spelled: the startup migration and the self-update + # preflight protect this file before/without wiring, and three + # independent spellings is how they fell out of sync (PR #239 round 6). + memory_path = DEFAULT_MEMORY_PATH channel_config = ChannelConfigManager("./data/channel_config.json") # Passive channel logger — writes ALL guild messages to JSONL (zero LLM tokens) @@ -257,6 +261,9 @@ def build_services(config: Config) -> BotServices: # noqa: PLR0915 — linear c tool_executor = ToolExecutor( config.tools, memory_path=memory_path, + # Full config so the workspace's protected roots cover every relocatable + # live-state path, not just the ones ToolsConfig declares. + app_config=config, browser_manager=browser_manager, output_streamer=output_streamer, host_access_manager=host_access_manager, diff --git a/src/health/metrics.py b/src/health/metrics.py index f28e98de..5c46b342 100644 --- a/src/health/metrics.py +++ b/src/health/metrics.py @@ -118,6 +118,38 @@ def render(self) -> str: except Exception: pass + # -- Local command workspace (no auto-prune, so growth is observable) -- + ws_source = self._sources.get("workspace") + if ws_source: + try: + ws = ws_source() or {} + if ws: + gauges = { + "bytes": ( + "odin_workspace_bytes", + "Bytes stored in the local command workspace", + ), + "files": ( + "odin_workspace_files", + "Files in the local command workspace", + ), + "free_bytes": ( + "odin_workspace_free_bytes", + "Free bytes on the workspace filesystem", + ), + "free_inodes": ( + "odin_workspace_free_inodes", + "Free inodes on the workspace filesystem", + ), + } + for key, (name, helptext) in gauges.items(): + if key in ws: + sections.append(f"# HELP {name} {helptext}") + sections.append(f"# TYPE {name} gauge") + sections.append(_format_metric(name, ws[key], include_header=False)) + except Exception: + pass + # -- Tool execution metrics -- tool_source = self._sources.get("tools") if tool_source: diff --git a/src/health/startup.py b/src/health/startup.py index 816b9c81..935f17af 100644 --- a/src/health/startup.py +++ b/src/health/startup.py @@ -426,6 +426,84 @@ def check_config_sections(config: Any) -> DiagnosticResult: ) +def check_local_workspace(config: Any) -> DiagnosticResult: + """Verify the local command workspace against the full live config. + + Local user commands FAIL CLOSED when this directory is missing, wrongly + owned, or not 0700 — deliberately, because falling back to the install + directory is the hazard this exists to remove. That makes it an operator- + visible dependency: without this check a broken workspace shows up as + every run_command failing, with nothing in the startup report to say why. + """ + from ..tools.workspace import WorkspaceError, provisioning_hint, resolve_workspace + + # Production passes the full Config so independently relocated sessions, + # context, logs, credentials, and other state are protected exactly as the + # startup migration and executor protect them. Accept ToolsConfig directly + # only for focused callers/tests; that intentionally yields the reduced + # fallback contract. + tools_config = getattr(config, "tools", config) + full_config = config if tools_config is not config else None + configured = getattr(tools_config, "local_working_dir", "") or "" + try: + workspace = resolve_workspace( + configured, + protected_roots=_workspace_protected_roots(full_config), + ) + except WorkspaceError as exc: + return DiagnosticResult( + name="local_workspace", + passed=False, + detail=f"Local command workspace unusable: {exc}", + recommendation=provisioning_hint(configured), + metadata={"configured": configured}, + ) + except Exception as exc: # pragma: no cover - defensive + return DiagnosticResult( + name="local_workspace", + passed=False, + detail=f"Local command workspace check failed: {exc}", + recommendation=provisioning_hint(configured), + metadata={"configured": configured}, + ) + # A legacy-config fallback is usable, so this still passes — but it must + # be visible in the report an operator reads, not buried as a path they + # would have to notice (cross-review of PR #239 round 13). + from ..tools.workspace import startup_fallback + + fallback = startup_fallback() + if fallback is not None and str(workspace) == fallback[0]: + _active, intended, reason = fallback + return DiagnosticResult( + name="local_workspace", + passed=True, + detail=( + f"Local command workspace ready at FALLBACK {workspace} — the " + f"configured default {intended!r} could not be provisioned ({reason})" + ), + recommendation=provisioning_hint(intended), + metadata={ + "path": str(workspace), + "configured": intended, + "fallback": True, + }, + ) + return DiagnosticResult( + name="local_workspace", + passed=True, + detail=f"Local command workspace ready: {workspace}", + metadata={"path": str(workspace)}, + ) + + +def _workspace_protected_roots(config: object = None) -> list[str]: + """Protected roots for the diagnostic, from the same full live config and + shared derivation used by startup migration and executor.""" + from ..tools.workspace import command_protected_roots + + return command_protected_roots(Path(__file__).absolute().parents[2], config) + + def check_data_directories() -> DiagnosticResult: """Verify core data directories exist or can be created.""" dirs = [ @@ -508,6 +586,9 @@ def check_codex_model(codex_config: Any) -> DiagnosticResult: ("ssh_hosts", check_ssh_hosts, "tools"), ("sessions_directory", check_sessions_directory, "sessions"), ("knowledge_db", check_knowledge_db, "search"), + # Full Config, not only ToolsConfig: every relocatable live-state path + # must be judged by the same contract as runtime command execution. + ("local_workspace", check_local_workspace, None), ("config_consistency", check_config_sections, None), # uses full Config ] diff --git a/src/tools/executor.py b/src/tools/executor.py index 774a533a..a8da2ac2 100644 --- a/src/tools/executor.py +++ b/src/tools/executor.py @@ -3,6 +3,9 @@ import asyncio import contextvars import json +import os +import shutil +import threading import time from pathlib import Path from typing import Any @@ -43,6 +46,7 @@ run_ssh_command, ) from .ssh_pool import SSHConnectionPool +from .workspace import DEFAULT_MEMORY_PATH, command_protected_roots, resolve_workspace log = get_logger("tools") @@ -78,6 +82,11 @@ "odin_current_tool_timeout", default=None ) +# How long a workspace size/count walk is reused. Growth is an operator signal +# measured in hours, so a minute of staleness costs nothing; scraping /metrics +# in a loop against an ever-growing directory costs the event loop a great deal. +WORKSPACE_METRICS_TTL = 60.0 + def _validate_memory_shape(data: dict) -> None: """Nested-shape check for memory.json, run inside json_store's backup @@ -155,9 +164,36 @@ def __init__( output_streamer: ToolOutputStreamer | None = None, host_access_manager: HostAccessManager | None = None, email_config: object | None = None, + app_config: object | None = None, ) -> None: self.config = config or ToolsConfig() + # The FULL live config, supplied by wiring. Live state is not confined + # to the data directory — sessions, context, logs, usage, the search + # index, permissions and Codex credentials are each independently + # relocatable, and a workspace overlapping any of them is as dangerous + # as one inside ./data (PR #239 round-8 review, reproduced). Optional + # so tests and the __new__ patch seam still construct. + self._app_config = app_config self._email_config = email_config + # The configured workspace VALUE is restart-required, but it is + # re-validated on every local command rather than cached: existence, + # type, ownership and mode are mutable filesystem state (see + # _ensure_local_workspace). + # + # Resolution is LAZY (first local command) rather than at construction: + # constructing an executor must not depend on deployment having created + # the directory, or every test and fresh checkout breaks. The safety + # property is unchanged, because it binds where it matters — a local + # command never runs with an unvalidated cwd, and an unusable workspace + # raises instead of silently falling back to the inherited cwd, which + # is what would restore the 2026-07-27 hazard. + self._local_workspace: str | None = None + self._local_workspace_resolved = False + # (completed_at_monotonic, bytes, files) from the last usage walk, + # refreshed off-thread; the lock makes the refresh single-flight. + self._workspace_usage_cache: tuple[float, float, float] | None = None + self._workspace_usage_refreshing = False + self._workspace_usage_lock = threading.Lock() self._memory_path = Path(memory_path) if memory_path else None self._browser_manager = browser_manager self._permission_manager = permission_manager @@ -299,6 +335,174 @@ def _resolve_default_host(self, user_id: str | None) -> str: hosts = list(self.config.hosts.keys()) return hosts[0] if hosts else "" + def _protected_roots(self) -> list[str]: + """Roots the local workspace must not overlap, derived from the RUNNING + application rather than assumed (PR #239 review). + + A hardcoded ``/opt/odin`` is wrong under Docker (install root ``/app``) + and for source checkouts, and packaged ``/opt/odin/data`` is a symlink + to ``/var/lib/odin`` — so the live-data root is taken from the actual + configured data paths and canonicalized, not string-joined. + + Derivation itself lives in workspace.command_protected_roots so the + startup migration and the self-update preflight protect exactly the + same directories. When each caller derived its own, the preflight + accepted (and created) a workspace beside live memory.json that the + executor then rejected (PR #239 round-6 review). + """ + return command_protected_roots( + # Install root: the package's own location (…/src/tools/executor.py). + Path(__file__).absolute().parents[2], + # getattr-guarded throughout: the sanctioned __new__ patch seam + # builds executors without __init__, so these may not exist. + getattr(self, "_app_config", None), + tools=getattr(self, "config", None), + # The live memory.json is supplied by wiring, not ToolsConfig; + # falling back to the shared default rather than no protection. + memory_path=getattr(self, "_memory_path", None) or DEFAULT_MEMORY_PATH, + ) + + def get_workspace_metrics(self) -> dict[str, float]: + """Usage of the local command workspace, for Prometheus. + + The accepted design deliberately does NOT auto-prune — age-based + deletion would destroy the cross-command continuity the stable + workspace exists to provide — so growth must be observable instead, + with cleanup an explicit operator action (PR #239 round-2 review). + + Never raises: metrics collection must not be able to break a command + path, and an unresolvable workspace simply reports nothing. + """ + try: + root = Path(self._ensure_local_workspace()) + except Exception: + return {} + + # The size/count walk NEVER runs on the calling thread. /metrics is + # unauthenticated and served on the event loop, and this directory + # deliberately never prunes, so a synchronous walk is a stall that only + # grows (PR #239 round-9/10 review). A stale cache triggers a + # single-flight background refresh and the previous numbers are served + # meanwhile; until the first refresh completes, usage is simply absent + # rather than blocking. The timestamp is recorded when the walk + # COMPLETES — stamping at the start makes a walk longer than the TTL + # instantly stale, so every scrape would launch another one. + self._refresh_workspace_usage(root) + cached = getattr(self, "_workspace_usage_cache", None) + metrics: dict[str, float] = {} + if cached is not None: + metrics["bytes"], metrics["files"] = cached[1], cached[2] + # Cheap statvfs calls: these stay LIVE on every scrape, cached usage + # or not, because free space is the number an operator alerts on. + try: + usage = shutil.disk_usage(root) + metrics["free_bytes"] = float(usage.free) + except OSError: + pass + try: + stats = os.statvfs(root) + metrics["free_inodes"] = float(stats.f_favail) + except (OSError, AttributeError): + pass + return metrics + + def _refresh_workspace_usage(self, root: Path) -> None: + """Start a single-flight background walk if the cache is stale. + + Never blocks the caller. Never raises: metrics collection must not be + able to break anything, least of all the event loop it runs on. + """ + cached = getattr(self, "_workspace_usage_cache", None) + if cached is not None and (time.monotonic() - cached[0]) < WORKSPACE_METRICS_TTL: + return + lock = getattr(self, "_workspace_usage_lock", None) + if lock is None: # __new__ patch seam + return + with lock: + # Re-check freshness after acquiring the lock. Another scrape may + # have completed and published while this caller was waiting; the + # unlocked fast-path observation above is not authoritative once + # lock acquisition blocks. Without this second check, concurrent + # scrapes can launch an immediate duplicate walk after a fast first + # refresh finishes. + cached = getattr(self, "_workspace_usage_cache", None) + if cached is not None and (time.monotonic() - cached[0]) < WORKSPACE_METRICS_TTL: + return + if self._workspace_usage_refreshing: + return + self._workspace_usage_refreshing = True + + def _walk() -> None: + total_bytes = 0.0 + files = 0.0 + published = False + try: + for dirpath, _dirnames, filenames in os.walk(root, followlinks=False): + for name in filenames: + files += 1 + try: + total_bytes += os.lstat(os.path.join(dirpath, name)).st_size + except OSError: + pass + # Publish the completed cache and clear the single-flight flag + # as ONE locked transition. Clearing first leaves a race where + # a scrape launches a duplicate walk before the fresh cache is + # visible (Odin, PR #239 round-12). + completed_at = time.monotonic() + with lock: + self._workspace_usage_cache = (completed_at, total_bytes, files) + self._workspace_usage_refreshing = False + published = True + except Exception: + # A metrics walk is best-effort. Letting an unexpected error + # escape a daemon thread still invokes threading.excepthook + # (and produces an unhandled-thread warning in pytest/logging), + # contradicting this collector's never-raises contract. The + # finally block below clears the single-flight flag so the next + # scrape can recover. + pass + finally: + # The flag must never survive this thread. Clearing it only on + # the OSError and success paths wedged usage metrics FOREVER on + # any other exception — the flag stayed set, so every later + # scrape declined to start a refresh (cross-review of round 12, + # reproduced). Guarded by `published` so the success path's + # atomic publish is not split apart. + if not published: + with lock: + self._workspace_usage_refreshing = False + + try: + threading.Thread( + target=_walk, name="odin-workspace-usage", daemon=True + ).start() + except Exception: # pragma: no cover - thread creation cannot realistically fail + with lock: + self._workspace_usage_refreshing = False + + def _ensure_local_workspace(self) -> str: + """Resolve and re-validate the cwd for local user commands. + + Raises :class:`WorkspaceError` if the configured directory is unusable. + Deliberately no fallback: inheriting the process cwd is exactly the + behaviour that let a bare `rm -rf data` delete the live install. + """ + # Validated on EVERY call, not cached (PR #239 round-3 review): the + # configured VALUE is restart-required, but existence, type, ownership + # and mode are mutable filesystem state. Caching them meant fail-closed + # only applied to the first command — replacing the directory with a + # symlink into the install afterwards was accepted, and a post- + # validation chmod was ignored. The check is a handful of stat calls. + workspace = str( + resolve_workspace( + self.config.local_working_dir, + protected_roots=self._protected_roots(), + ) + ) + self._local_workspace = workspace + self._local_workspace_resolved = True + return workspace + def _ensure_process_registry(self): """Lazy-init the ProcessRegistry ON THE EXECUTOR (RFC-004 P4). @@ -309,7 +513,9 @@ def _ensure_process_registry(self): if not hasattr(self, "_process_registry"): from .process_manager import ProcessRegistry - self._process_registry = ProcessRegistry() + # Pass the RESOLVER, not a resolved string: each background spawn + # must re-verify the workspace's mutable filesystem invariants. + self._process_registry = ProcessRegistry(workspace=self._ensure_local_workspace) return self._process_registry def check_permission(self, tool_name: str, user_id: str | None) -> str | None: @@ -618,6 +824,7 @@ async def _exec_command( ssh_user: str = "root", timeout: int | None = None, on_output: OutputCallback | None = None, + use_workspace: bool = False, ) -> tuple[int, str]: """Execute a command locally or via SSH depending on host address. @@ -634,16 +841,31 @@ async def _exec_command( if timeout is None: timeout = _current_tool_timeout_ctx.get() or self.config.command_timeout_seconds if is_local_address(address): + # The workspace applies ONLY to raw user commands, and only because + # the caller asked for it. This primitive also backs git_ops, + # docker, terraform, kubectl, claude_code, PDF host reads and + # validation probes, whose documented defaults resolve against the + # process cwd — git_ops with `repo` omitted means ".", i.e. the + # install repo, and silently repointing that at a scratch directory + # broke `git_ops status` with "fatal: not a git repository" + # (PR #239 round-8 review, reproduced). Default False keeps every + # such tool byte-identical to pre-PR behaviour. + cwd = self._ensure_local_workspace() if use_workspace else None bh = self.bulkheads.get("subprocess") if bh: try: async with bh.acquire(): return await run_local_command( - command, timeout=timeout, on_output=on_output + command, + timeout=timeout, + on_output=on_output, + cwd=cwd, ) except BulkheadFullError: return 1, "Error: subprocess bulkhead full — too many concurrent local commands" - return await run_local_command(command, timeout=timeout, on_output=on_output) + return await run_local_command( + command, timeout=timeout, on_output=on_output, cwd=cwd + ) ssh_retry = self.config.ssh_retry ssh_kwargs: dict[str, Any] = dict( host=address, @@ -667,12 +889,23 @@ async def _exec_command( return 1, "Error: SSH bulkhead full — too many concurrent SSH commands" return await run_ssh_command(**ssh_kwargs) - async def _run_on_host(self, alias: str, command: str) -> str | tuple[str, int]: + async def _run_on_host( + self, alias: str, command: str, use_workspace: bool = False + ) -> str | tuple[str, int]: + """Run a command on an aliased host. + + ``use_workspace`` is opt-in for the same reason as _exec_command: this + also backs read_file/write_file host reads, skill_context.run_on_host, + and the audit diff tracker, whose paths are absolute and whose cwd + semantics must not change. + """ resolved = self._resolve_host(alias) if not resolved: return f"Unknown or disallowed host: {alias}" address, ssh_user, _os = resolved - code, output = await self._exec_command(address, command, ssh_user) + code, output = await self._exec_command( + address, command, ssh_user, use_workspace=use_workspace + ) if code != 0: return f"Command failed (exit {code}):\n{output}", code return output, 0 diff --git a/src/tools/handlers/files_docs.py b/src/tools/handlers/files_docs.py index 1820bee6..29cc435d 100644 --- a/src/tools/handlers/files_docs.py +++ b/src/tools/handlers/files_docs.py @@ -10,6 +10,7 @@ from __future__ import annotations import base64 +import posixpath import shlex from .deps import HandlerBase @@ -46,7 +47,28 @@ async def _handle_write_file(self, inp: dict) -> str: return "Error: 'content' is required for write_file." if not host: return "Error: 'host' is required for write_file." + # The schema documents this path as absolute, but nothing enforced it, + # so a relative path silently resolved against Odin's install directory + # and wrote there (PR #239 round-10 review, reproduced). Rejecting is + # better than quietly redirecting into the workspace: a write whose + # destination the caller did not choose is its own hazard, and no + # documented capability is lost. + if not str(path).startswith("/"): + return ( + f"Error: write_file requires an absolute path, got {path!r}. " + "A relative path would resolve against Odin's working directory " + "rather than where you intend." + ) + path = str(path) safe_path = shlex.quote(path) + # Compute and quote the parent as its own shell argument. Embedding a + # quoted path in ``$(dirname ...)`` and then leaving the substitution + # unquoted word-splits a parent containing spaces; ``mkdir`` can create + # those extra relative words in Odin's inherited install cwd even though + # the requested file path itself is absolute (PR #239 final review, + # reproduced). Remote managed hosts are POSIX, matching the absolute-path + # contract above. + safe_parent = shlex.quote(posixpath.dirname(path) or "/") # Govern the write before executing — write_file reaches the filesystem # via _run_on_host, which does NOT itself govern. Check a representative # redirect-to-path command so policy (e.g. writes to sensitive targets) @@ -55,8 +77,11 @@ async def _handle_write_file(self, inp: dict) -> str: if not allowed: return denial # Base64-encode content to avoid shell injection via heredoc delimiter - encoded = base64.b64encode(content.encode()).decode() - cmd = f"mkdir -p $(dirname {safe_path}) && echo '{encoded}' | base64 -d > {safe_path}" + encoded = shlex.quote(base64.b64encode(content.encode()).decode()) + cmd = ( + f"mkdir -p -- {safe_parent} && " + f"printf %s {encoded} | base64 -d > {safe_path}" + ) return await self._run_on_host(host, cmd) @staticmethod diff --git a/src/tools/handlers/system.py b/src/tools/handlers/system.py index ebeae971..93bc890b 100644 --- a/src/tools/handlers/system.py +++ b/src/tools/handlers/system.py @@ -61,6 +61,8 @@ async def _handle_run_command(self, inp: dict) -> str | tuple[str, int]: command, ssh_user, on_output=on_output, + # run_command is THE tool the 2026-07-27 wipe came through. + use_workspace=True, ) if finish_cb: try: @@ -144,6 +146,8 @@ async def _handle_run_script(self, inp: dict) -> str | tuple[str, int]: cmd, ssh_user, on_output=on_output, + # run_script executes arbitrary user script text, same hazard. + use_workspace=True, ) if finish_cb: try: @@ -193,7 +197,7 @@ async def _handle_run_command_multi(self, inp: dict) -> str | tuple[str, int]: allowed_hosts.append(h) async def _run_one(alias: str) -> tuple[str, bool]: - raw = await self._run_on_host(alias, command) + raw = await self._run_on_host(alias, command, use_workspace=True) if isinstance(raw, tuple): text, code = raw[0], raw[1] host_err = code != 0 diff --git a/src/tools/handlers/validation.py b/src/tools/handlers/validation.py index 988aca55..ac1fe85a 100644 --- a/src/tools/handlers/validation.py +++ b/src/tools/handlers/validation.py @@ -42,7 +42,12 @@ async def _handle_validate_action(self, inp: dict) -> str: governor = getattr(self, "command_governor", None) async def _exec( - address: str, command: str, ssh_user: str, *, timeout: int + address: str, + command: str, + ssh_user: str, + *, + timeout: int, + use_workspace: bool = False, ) -> tuple[int, str]: # Never mutate shared state here — concurrent checks would race. # _exec_command accepts a per-call timeout, which is honored @@ -61,7 +66,14 @@ async def _exec( return 1, f"validate_action: governor check raised {type(ge).__name__}: {ge}" if not decision.allowed: return 1, f"governor-blocked: {decision.denial_message()}" - return await self._exec_command(address, command, ssh_user, timeout=timeout) + # Forwarded per check from run_bundle: True only for type=command + # (user-supplied text, a raw command route like run_command — + # round 10); fixed-shape probes must keep pre-PR cwd semantics so + # an unusable workspace cannot disable service/process/http/port + # validation (round 11). + return await self._exec_command( + address, command, ssh_user, timeout=timeout, use_workspace=use_workspace + ) report = await run_bundle( raw_checks, diff --git a/src/tools/post_validation.py b/src/tools/post_validation.py index e2f0e58a..43643792 100644 --- a/src/tools/post_validation.py +++ b/src/tools/post_validation.py @@ -283,11 +283,17 @@ def _build_command(check: Check) -> str | None: h, p = tgt.rsplit(":", 1) else: h, p = "127.0.0.1", tgt - if not p.isdigit(): + if not h or not p.isdigit(): return None + # Do not interpolate a quoted host inside another quoted shell program. + # ``shlex.quote(h)`` embedded in ``bash -c '...{h}...'`` lets the outer + # shell evaluate substitutions in crafted hosts before Bash starts. + # Positional arguments keep the inner program constant and quote each + # user value exactly once for the outer shell. + script = 'cat < /dev/null > "/dev/tcp/$1/$2"' return ( - f"timeout {timeout} bash -c 'cat < /dev/null > /dev/tcp/{shlex.quote(h)}/{p}'" - " && echo OPEN || echo CLOSED" + f"timeout {timeout} bash -c {shlex.quote(script)} _ " + f"{shlex.quote(h)} {shlex.quote(p)} && echo OPEN || echo CLOSED" ) if t == "service": return f"systemctl is-active {shlex.quote(tgt)} 2>/dev/null || true" @@ -464,7 +470,14 @@ async def run_bundle( ) -> ValidationReport: """Run a validation bundle. Host resolution: explicit > default > localhost. - exec_command signature: (address, command, ssh_user, timeout=...) -> (exit_code, output) + exec_command signature: + (address, command, ssh_user, timeout=..., use_workspace=...) -> (exit_code, output) + ``use_workspace`` is True ONLY for ``type=command`` checks — those execute + user-supplied command text, exactly the raw route the local workspace + exists for. Fixed-shape probes (http/port/service/process/log) are + generated command strings whose behaviour must not depend on the + workspace: an unusable workspace must not stop a service probe + (PR #239 round-11 review, reproduced). """ start = time.monotonic() if grace_seconds > 0: @@ -526,7 +539,15 @@ async def _run_one(idx: int, check: Check) -> CheckResult: return result try: exit_code, output = await asyncio.wait_for( - exec_command(address, command, ssh_user, timeout=check.timeout_seconds), + exec_command( + address, + command, + ssh_user, + timeout=check.timeout_seconds, + # Raw user command text opts into the workspace; + # fixed-shape probes keep pre-PR cwd semantics. + use_workspace=check.type == "command", + ), timeout=check.timeout_seconds + 5, ) except TimeoutError: diff --git a/src/tools/process_manager.py b/src/tools/process_manager.py index 7a10bb50..42b0a299 100644 --- a/src/tools/process_manager.py +++ b/src/tools/process_manager.py @@ -10,9 +10,12 @@ import asyncio import time from collections import deque +from collections.abc import Callable from dataclasses import dataclass, field +from pathlib import Path from ..odin_log import get_logger +from .workspace import WorkspaceError, workspace_env log = get_logger("process_manager") @@ -43,8 +46,24 @@ class ProcessInfo: class ProcessRegistry: """Registry for background processes with full lifecycle management.""" - def __init__(self) -> None: + def __init__(self, workspace: str | Callable[[], str] | None = None) -> None: self._processes: dict[int, ProcessInfo] = {} + # Background starts share the foreground workspace. Without this, + # `manage_process start` stays an alternate route to the 2026-07-27 + # incident: a bare relative path resolving against the live install + # (29 historical background starts had no explicit cd). + # + # A CALLABLE is preferred: the workspace's existence, type, ownership + # and mode are mutable, so they must be re-verified immediately before + # each spawn rather than trusted from construction time (PR #239 + # round-3 review — a cached path accepted a directory later replaced + # by a symlink into the install). + self._workspace = workspace + + def _resolve_workspace(self) -> str | None: + if callable(self._workspace): + return self._workspace() + return self._workspace # ------------------------------------------------------------------ # Public API @@ -69,12 +88,22 @@ async def start(self, host: str, command: str, timeout: int = 300) -> str: # start_new_session puts the shell at the head of its own process # group, so kill()/shutdown() can take out descendants # (`sh -c 'x & ...'`) instead of just the shell leader. + workspace = self._resolve_workspace() + env = workspace_env(Path(workspace)) if workspace else None + except WorkspaceError as e: + # The workspace is unusable. This is a REFUSAL, not a spawn error: + # it must read as a failure to the tool loop, not as a started + # process (PR #239 round-4 — the plain string was classified ok). + return f"Error: cannot start background process — {e}" + try: proc = await asyncio.create_subprocess_shell( command, stdout=asyncio.subprocess.PIPE, stderr=asyncio.subprocess.STDOUT, stdin=asyncio.subprocess.PIPE, start_new_session=True, + cwd=workspace, + env=env, ) except Exception as e: return f"Failed to start process: {e}" @@ -155,9 +184,7 @@ async def kill(self, pid: int) -> str: # would otherwise outlive an in-place restart's exec). # owned_pgid: start() spawns with start_new_session=True, so # the group stays signallable after the leader exits. - await terminate_process_tree( - info.process, grace=5.0, owned_pgid=info.process.pid - ) + await terminate_process_tree(info.process, grace=5.0, owned_pgid=info.process.pid) info.status = "failed" info.exit_code = -9 log.info("Killed process PID %d", pid) @@ -272,9 +299,7 @@ async def _read_output(self, info: ProcessInfo) -> None: from ..tools.ssh import terminate_process_tree try: - await terminate_process_tree( - info.process, grace=2.0, owned_pgid=info.process.pid - ) + await terminate_process_tree(info.process, grace=2.0, owned_pgid=info.process.pid) except Exception: log.debug("group reap after PID %d exit failed", info.pid, exc_info=True) diff --git a/src/tools/skill_context.py b/src/tools/skill_context.py index 8900dd15..75c7530f 100644 --- a/src/tools/skill_context.py +++ b/src/tools/skill_context.py @@ -164,7 +164,13 @@ async def run_on_host(self, alias: str, command: str) -> str: # _run_on_host returns (output, exit_code) for resolved hosts and a bare # denial string for unknown ones (TS-0004 — forwarding the raw tuple # violated this method's documented str contract for every real host). - raw = await self._executor._run_on_host(alias, command) + # use_workspace=True: this is arbitrary command execution exposed to + # user-created skills, so it is a raw user-command route exactly like + # run_command — leaving it out made it an alternate path back into the + # 2026-07-27 wipe, reproduced through this method (PR #239 round-9 + # review). Remote hosts are unaffected: the workspace applies only + # after local-address resolution. + raw = await self._executor._run_on_host(alias, command, use_workspace=True) if isinstance(raw, tuple): return raw[0] return raw diff --git a/src/tools/ssh.py b/src/tools/ssh.py index 95a9ee02..13f6ee6a 100644 --- a/src/tools/ssh.py +++ b/src/tools/ssh.py @@ -4,10 +4,12 @@ import os import signal from collections.abc import Awaitable, Callable +from pathlib import Path from typing import TYPE_CHECKING from ..llm.backoff import compute_backoff from ..odin_log import get_logger +from .workspace import workspace_env if TYPE_CHECKING: from .ssh_pool import SSHConnectionPool @@ -115,9 +117,7 @@ async def terminate_process_tree( # (``pgid == proc.pid``) and (b) a safe, foreign target. A pgid that is # broadcast/own-group — however it arose (a bad owned_pgid, a recycled pid) — # is dropped here and cleanup falls back to signalling the child alone. - owns_group = ( - pgid is not None and pgid == proc.pid and _is_signallable_group(pgid) - ) + owns_group = pgid is not None and pgid == proc.pid and _is_signallable_group(pgid) if proc.returncode is not None and not owns_group: return # child-only cleanup and the child is already reaped @@ -207,17 +207,29 @@ async def run_local_command( command: str, timeout: int = 30, on_output: OutputCallback | None = None, + cwd: str | None = None, ) -> tuple[int, str]: """Run a command locally via subprocess. Returns (exit_code, output). Used for localhost hosts — no SSH overhead, no key needed. When *on_output* is provided, stdout is streamed line-by-line to the callback in addition to being collected for the return value. + + *cwd* is the resolved local workspace (``tools.local_working_dir``). It only + changes where a BARE RELATIVE path lands: explicit ``cd``, absolute paths, + and ``git -C `` are unaffected, so deliberately working inside the + install remains possible. ``None`` inherits the process cwd — the + pre-2026-07-27 behaviour, kept only for internal callers that legitimately + depend on the application directory. """ log.info("Local exec: %s", command) proc: asyncio.subprocess.Process | None = None try: + # PWD/OLDPWD are normalized alongside cwd: cwd= alone leaves an + # inherited OLDPWD pointing at the install, so a bare `cd -` would walk + # right back into it (review finding, 2026-07-27). + env = workspace_env(Path(cwd)) if cwd else None # start_new_session puts the shell at the head of its own process # group, so timeout/cancellation cleanup can take out descendants # (`sh -c 'x & ...'`) instead of just the shell leader. @@ -226,11 +238,11 @@ async def run_local_command( stdout=asyncio.subprocess.PIPE, stderr=asyncio.subprocess.STDOUT, start_new_session=True, + cwd=cwd, + env=env, ) if on_output is not None: - return await _read_lines_with_callback( - proc, timeout, on_output, owned_pgid=proc.pid - ) + return await _read_lines_with_callback(proc, timeout, on_output, owned_pgid=proc.pid) stdout, _ = await asyncio.wait_for(proc.communicate(), timeout=timeout) output = stdout.decode("utf-8", errors="replace") return proc.returncode or 0, _truncate_output(output) diff --git a/src/tools/workspace.py b/src/tools/workspace.py new file mode 100644 index 00000000..ec5785f2 --- /dev/null +++ b/src/tools/workspace.py @@ -0,0 +1,517 @@ +"""Resolution and validation of the local command workspace. + +User-command execution (``run_command``, ``run_script``, ``manage_process``) +used to inherit the service's working directory, which is the install root. +A bare relative path in a command therefore resolved against the live install: +on 2026-07-27 an AE2 jar whose internal layout is ``data/`` was extracted and +then cleaned up with ``rm -rf data``, deleting ``/opt/odin/data``. + +This module resolves ONE validated directory that those subprocesses run in +instead. Scope is deliberately narrow: + +- It changes where a *bare relative path* lands. Nothing else. +- Explicit ``cd`` still works. Absolute paths still work. ``git -C `` + still works. Deliberately operating inside the install remains possible. +- It does NOT confine the process. ``..``, an absolute path, a symlink placed + inside the workspace, or archive path traversal can still reach the install. + Preventing those needs filesystem confinement and would cost capability; + what this closes is the accidental basename collision that actually bit. + +Validation fails CLOSED — callers raise instead of silently falling back to +the inherited cwd, because that fallback is precisely the hazard. + +Protected roots are supplied by the caller and canonicalized here, never +assumed (PR #239 review): the install root is ``/app`` under Docker and an +arbitrary path for source checkouts, and packaged ``/opt/odin/data`` is a +SYMLINK to ``/var/lib/odin``, so a naive ``install / "data"`` string check +would happily accept a workspace sitting inside the real live-data directory. +Overlap is rejected in BOTH directions: a workspace that *contains* a +protected root is just as unusable as one nested inside it. +""" + +from __future__ import annotations + +import os +import stat +from collections.abc import Callable, Sequence +from pathlib import Path +from types import SimpleNamespace + +# The accepted operational contract: a real directory owned by the execution +# identity, private, and fully usable by that owner (0700). 0300 is private +# and writable but not readable, which breaks ordinary workspace use. +REQUIRED_MODE = 0o700 + +# The live working-memory file. Production wiring hardcodes this path rather +# than taking it from config, so it is defined HERE and imported there: the +# startup migration and the self-update preflight both run before (or without) +# wiring and must protect the same file the executor protects. Three +# independent spellings of "the protected roots" is exactly how round 6 found +# the updater creating a workspace beside live memory.json and reporting +# success, then handing over to an executor that refused every command. +DEFAULT_MEMORY_PATH = "./data/memory.json" + +# Set once at startup when the legacy-config fallback engages. Process-wide +# startup state, like the active config path — read by the startup diagnostic +# so a fallback is VISIBLE rather than indistinguishable from normal operation. +# Keep the failed configured path as well as the active fallback: startup +# mutates ToolsConfig to the fallback so every runtime consumer agrees, and +# without this copy later remediation would point at the already-working path. +_STARTUP_FALLBACK: tuple[str, str, str] | None = None + + +def startup_fallback() -> tuple[str, str, str] | None: + """``(active_workspace, configured_workspace, reason)`` after fallback.""" + return _STARTUP_FALLBACK + + +class WorkspaceError(RuntimeError): + """The configured local workspace is unusable. Never fall back to cwd.""" + + +def _canonical(path: str | os.PathLike[str]) -> Path: + """Fully resolved path, symlinks included — aliases must not smuggle a + protected root past the overlap check.""" + return Path(path).expanduser().resolve(strict=False) + + +def _lexical_absolute(path: str | os.PathLike[str]) -> Path: + """Absolute spelling with ``..`` removed but symlink components intact. + + Canonical paths protect the object reached through an alias. Lexical paths + protect the alias components a running process will traverse again. Both + matter: deleting an alias can strand sessions, credentials, or the checkout + used for an in-place restart even when the canonical target survives. + """ + return Path(os.path.abspath(Path(path).expanduser())) + + +def _path_spellings(path: str | os.PathLike[str]) -> tuple[Path, ...]: + """Unique lexical and canonical spellings, in stable order.""" + lexical = _lexical_absolute(path) + canonical = _canonical(path) + return (lexical,) if lexical == canonical else (lexical, canonical) + + +def _overlaps(workspace: Path, root: Path) -> bool: + """True when the two paths are the same or either contains the other.""" + return workspace == root or root in workspace.parents or workspace in root.parents + + +def _reject_overlap( + workspace: Path, protected_roots: Sequence[str | os.PathLike[str]] | None +) -> None: + """Raise if ``workspace`` overlaps any protected root, in either direction.""" + for root in protected_roots or []: + # BOTH spellings: lexical-absolute (as given) and canonical (symlinks + # resolved). For ordinary roots they coincide; for the deliberately- + # lexical launch-path root they do not, and resolving it here would + # collapse the alias back onto its target and un-protect the component + # the restart traverses (PR #239 round-11 review). + for candidate in _path_spellings(root): + if _overlaps(workspace, candidate): + raise WorkspaceError( + f"local_working_dir must not overlap {candidate}: {workspace}" + ) + + +# Live-state paths declared by the FULL configuration, with their DECLARED +# file/directory semantics. Any of these can be relocated independently of the +# data directory, and a workspace overlapping one is as dangerous as a +# workspace inside ./data — round 8 reproduced a valid Config whose +# sessions.persist_directory WAS the workspace, accepted by every caller +# because only audit/trajectory/memory were protected. +# +# Deliberately excluded: tools.claude_code_dir and email.allowed_attachment_dirs +# are working directories for a tool and user-nominated source directories, not +# Odin's own state; protecting them would reject legitimate configurations. +# Paths wiring hardcodes (channel_config.json, channel_logs) sit beside +# memory.json and are covered by its parent. +_DECLARED_STATE_PATHS: tuple[tuple[str, bool], ...] = ( + ("tools.audit_log_path", True), + ("tools.trajectory_path", False), + ("tools.ssh_key_path", True), + ("tools.ssh_known_hosts_path", True), + ("tools.ssh_pool.socket_dir", False), + ("context.directory", False), + ("sessions.persist_directory", False), + ("logging.directory", False), + ("usage.directory", False), + # Treated as a file: wiring derives its sibling fts.db via `.parent`. + ("search.search_db_path", True), + ("permissions.overrides_path", True), + ("openai_codex.credentials_path", True), + ("attachments.temp_directory", False), +) + + +def _active_config_roots() -> list[str]: + """Directory roots the live config depends on, as FINAL root strings. + + Two spellings, deliberately different in kind: + + - the CANONICAL target's directory (symlinks fully resolved), so the real + file is protected wherever any alias points; + - the LAUNCH path's parent kept LEXICAL — absolutized but with symlinks + NOT resolved — because ``restart.reexec()`` replays ``sys.argv``. With a + symlinked ancestor component (``workspace/cfg -> real/``, launched as + ``workspace/cfg/odin.yml``), canonicalizing the parent collapses it onto + ``real`` and leaves the component the restart actually traverses + unprotected: a workspace-relative ``rm -rf cfg`` breaks the next re-exec + while the canonical target survives (PR #239 round-11 review, + reproduced; round 10 had only handled a symlinked leaf FILE). + + Imported lazily and guarded: workspace validation must never depend on the + config module being importable, and a process that never loaded a config + (tests, one-off scripts) has nothing to protect. + """ + try: + from ..config.schema import active_config_launch_path, active_config_path + + canonical = active_config_path() + launch = active_config_launch_path() + except Exception: # pragma: no cover - defensive + return [] + roots: list[str] = [] + if canonical: + roots.append(str(_canonical(canonical).parent)) + if launch: + # os.path.abspath normalizes WITHOUT resolving symlinks — that is the + # point. (active_config_launch_path already stores an abspath; applied + # again here defensively, it is idempotent.) + roots.append(str(Path(os.path.abspath(launch)).parent)) + return roots + + +def _dotted(source: object, path: str) -> object: + """Resolve ``a.b.c`` against nested config objects, tolerating absence.""" + current = source + for part in path.split("."): + current = getattr(current, part, None) + if current is None: + return None + return current + + +def command_protected_roots( + install_root: str | os.PathLike[str], + config: object = None, + *, + tools: object = None, + memory_path: object = DEFAULT_MEMORY_PATH, +) -> list[str]: + """THE derivation of directories a command workspace must never overlap. + + Every caller — executor, startup migration, self-update preflight — uses + this one function, so a workspace accepted by one is accepted by all. When + they each derived their own, the preflight approved (and created) a + workspace beside live memory.json that the executor then rejected. + + Pass the FULL ``config`` wherever one exists: live state is not confined to + the data directory, and sessions, context, logs, usage, the search index, + permissions and Codex credentials can each be relocated independently. + ``tools`` is the reduced fallback for callers holding only a ToolsConfig + (the executor's ``__new__`` patch seam and unit tests); it yields a strict + SUBSET, never a different answer. + + Paths are classified by DECLARED semantics, never guessed from the name: a + ``Path.suffix`` heuristic misreads dotted directories and extensionless + files. Each declared path is kept under both its lexical and canonical + spelling before file parents are taken: the lexical spelling protects the + alias a running subsystem traverses, while the canonical spelling protects + its target (``/aliases/memory.json -> /live-data/memory.json`` needs both). + """ + source: object = config + if source is None: + source = SimpleNamespace(tools=tools) if tools is not None else SimpleNamespace() + + roots: list[str] = [] + + def _append_path(path: object, *, is_file: bool) -> None: + """Protect both how a path is named and what that name reaches.""" + if path is None: + return + text = str(path).strip() + if not text: + return + for spelling in _path_spellings(text): + root = str(spelling.parent if is_file else spelling) + if root not in roots: + roots.append(root) + + _append_path(install_root, is_file=False) + declared: list[tuple[object, bool]] = [ + (_dotted(source, dotted), is_file) for dotted, is_file in _DECLARED_STATE_PATHS + ] + # The live memory.json is supplied by wiring rather than by config, so it + # is passed in rather than declared above. + declared.append((memory_path, True)) + # The ACTIVE config file is runtime state, not a Config field: Odin accepts + # `python -m src /arbitrary/path/odin.yml`. Its directory must be protected + # too, or a bare relative command can delete the file needed to restart — + # reproduced with an alternate config whose parent WAS the configured + # workspace (PR #239 round-9 review). + for config_root in _active_config_roots(): + _append_path(config_root, is_file=False) + + for configured, is_file in declared: + _append_path(configured, is_file=is_file) + return roots + + +def resolve_workspace( + configured: str, + *, + protected_roots: Sequence[str | os.PathLike[str]] | None = None, + require_owner: bool = True, + owner_uid: int | None = None, + create_if_missing: bool = True, +) -> Path: + """Validate ``configured`` and return the canonical workspace path. + + ``protected_roots`` are the install root and live-data root(s) the + workspace must not overlap. Callers derive them from the running + application rather than relying on a hardcoded ``/opt/odin``, which is + wrong under Docker (``/app``), for source checkouts, and for packaged + installs where ``data`` is a symlink elsewhere. + """ + if not configured or not str(configured).strip(): + raise WorkspaceError("tools.local_working_dir is empty") + + raw = Path(str(configured).strip()).expanduser() + if not raw.is_absolute(): + raise WorkspaceError(f"local_working_dir must be absolute: {configured!r}") + + # Rejected before resolution: a symlink could be repointed later, silently + # moving every command's working directory. + if raw.is_symlink(): + raise WorkspaceError(f"local_working_dir must not be a symlink: {raw}") + + workspace = _canonical(raw) + + # Protected-root overlap is checked BEFORE any mkdir. Creating the + # directory first and rejecting afterwards would leave a new directory + # inside the very tree this exists to protect — fail-closed must not mean + # "reject after modifying the place we promised not to touch" (PR #239 + # round-4 review, reproduced). + _reject_overlap(workspace, protected_roots) + + # Self-provision when we can. Upgrades must be seamless: a packaged install + # gets this from systemd StateDirectory= and the postinstall, but a source + # checkout or a git-based self-update lands on new code whose unit file was + # never refreshed, and failing closed there would silently cost local + # commands. Creating it is NOT the dangerous fallback — that would be + # inheriting the install directory. Everything below still validates. + if create_if_missing and not workspace.exists(): + try: + workspace.mkdir(mode=REQUIRED_MODE, parents=False) + # mkdir's mode is masked by umask; set it explicitly so a + # self-provisioned workspace satisfies the same 0700 contract that + # a deployment-provisioned one does. + workspace.chmod(REQUIRED_MODE) + except OSError: + pass # unwritable parent (e.g. root-owned /var/lib) — report below + + if not workspace.exists(): + raise WorkspaceError( + f"local_working_dir does not exist and could not be created: {workspace}. " + f"Create it as the service account, e.g. " + f"sudo install -d -m 0700 -o odin -g odin {workspace}" + ) + if not workspace.is_dir(): + raise WorkspaceError(f"local_working_dir is not a directory: {workspace}") + + # Re-checked after creation/canonicalization: the pre-mkdir check used the + # same canonical path, but re-running it keeps the guarantee local to the + # value actually about to be used. + _reject_overlap(workspace, protected_roots) + + info = workspace.stat() + + if require_owner: + expected = os.getuid() if owner_uid is None else owner_uid + if info.st_uid != expected: + raise WorkspaceError( + f"local_working_dir must be owned by uid {expected}: " + f"{workspace} is owned by uid {info.st_uid}" + ) + + mode = stat.S_IMODE(info.st_mode) + if mode != REQUIRED_MODE: + # Both halves matter: group/world bits leak a workspace that may hold + # command output, and an owner mode like 0300 is private but not + # readable, which breaks ordinary use. + raise WorkspaceError( + f"local_working_dir must be mode {REQUIRED_MODE:o}: {workspace} is {mode:o}" + ) + + if not os.access(workspace, os.R_OK | os.W_OK | os.X_OK): + raise WorkspaceError(f"local_working_dir is not fully usable: {workspace}") + + return workspace + + +def workspace_env(workspace: Path, base: dict[str, str] | None = None) -> dict[str, str]: + """Environment for a child shell started in ``workspace``. + + ``cwd=`` sets ``PWD`` correctly, but an inherited ``OLDPWD`` pointing at the + install means a bare ``cd -`` walks straight back into it — a small but real + hole in the boundary (PR #239 review). Both are normalized. + """ + env = dict(os.environ if base is None else base) + env["PWD"] = str(workspace) + env["OLDPWD"] = str(workspace) + return env + + +def provision_workspace( + configured: str, + *, + protected_roots: Sequence[str | os.PathLike[str]] | None = None, + allow_sudo: bool = True, + owner_uid: int | None = None, +) -> Path: + """THE authoritative provision-then-validate routine. Returns the workspace. + + One implementation, used by every caller — startup migration and the + self-update preflight both go through here. A second, weaker contract + elsewhere is worse than none: it can create a directory the runtime will + then refuse, or provision a different directory from the one actually used + (PR #239 round-5 review, which reproduced four such mismatches). + + Order matters. Everything that can be judged from the path alone — + absolute, not a symlink, not overlapping a protected root — is checked + BEFORE anything is created, so a rejected configuration never leaves a + directory inside the tree this protects. Creation is attempted directly, + then via ``sudo -n`` (packaged installs configure passwordless sudo for + exactly this class of operation), and the FULL :func:`resolve_workspace` + contract is applied afterwards. + """ + if not configured or not str(configured).strip(): + raise WorkspaceError("tools.local_working_dir is empty") + + raw = Path(str(configured).strip()).expanduser() + if not raw.is_absolute(): + raise WorkspaceError(f"local_working_dir must be absolute: {configured!r}") + if raw.is_symlink(): + raise WorkspaceError(f"local_working_dir must not be a symlink: {raw}") + + target = _canonical(raw) + _reject_overlap(target, protected_roots) + + if not target.exists(): + try: + # parents=False deliberately: silently materialising a whole path + # prefix is how a typo becomes a new directory tree. + target.mkdir(mode=REQUIRED_MODE, parents=False) + target.chmod(REQUIRED_MODE) + except OSError: + if allow_sudo: + _sudo_create(target, owner_uid) + + # Full contract, including ownership, exact mode and usability. Creation + # above is never a substitute for validation. + return resolve_workspace( + str(target), + protected_roots=protected_roots, + owner_uid=owner_uid, + create_if_missing=False, + ) + + +def provision_startup_workspace( + tools_config: object, + *, + protected_roots: Sequence[str | os.PathLike[str]] | None = None, + owner_uid: int | None = None, + on_fallback: Callable[[Path, str, WorkspaceError], None] | None = None, +) -> Path: + """Provision the workspace used by the incoming process at startup. + + Existing source/git installs have config files from before + ``local_working_dir`` existed. On their first in-place update the old + updater cannot run the new preflight, and an unprivileged developer account + commonly cannot create the schema default under ``/var/lib`` or use + ``sudo -n``. Failing closed there would preserve safety by silently losing + all local-command capability. + + Explicit configuration is authoritative and never substituted. Only a + genuinely absent legacy field may fall back, after the normal default + cannot be provisioned, to a stable private directory in the service user's + home. The selected value is written into the live ``ToolsConfig`` object so + startup, the executor, diagnostics, and the next self-update all use the + identical path for this process. A later PUT /api/config persists it through + the normal validated config path; otherwise the deterministic migration is + repeated on future starts. + """ + configured = str(getattr(tools_config, "local_working_dir", "") or "") + try: + return provision_workspace( + configured, + protected_roots=protected_roots, + owner_uid=owner_uid, + ) + except WorkspaceError as default_error: + fields_set: set[str] = set(getattr(tools_config, "model_fields_set", set())) + if "local_working_dir" in fields_set: + raise + + # A direct child of HOME needs no recursive parent creation, remains + # stable across commands/restarts, and is outside a normal source + # checkout. + # + # It is NOT a safety boundary for packaged installs. The packaged unit + # sets User= but no Environment=HOME, so HOME comes from the account + # record and is typically OUTSIDE the install (verified: /home/odin on + # a real deployment) — a broken packaged default therefore falls back + # here instead of rejecting. That is deliberate, because losing every + # local command is worse; it is made VISIBLE instead, via the warning + # below and the startup diagnostic, so a packaging failure is reported + # rather than hidden (cross-review of PR #239 round 13). + fallback = Path.home() / ".odin-workspace" + try: + workspace = provision_workspace( + str(fallback), + protected_roots=protected_roots, + owner_uid=owner_uid, + allow_sudo=False, + ) + except WorkspaceError: + raise default_error + setattr(tools_config, "local_working_dir", str(workspace)) + global _STARTUP_FALLBACK + _STARTUP_FALLBACK = (str(workspace), configured, str(default_error)) + if on_fallback is not None: + on_fallback(workspace, configured, default_error) + return workspace + + +def _sudo_create(target: Path, owner_uid: int | None) -> None: + """Best-effort privileged creation; failures fall through to validation, + which produces the actionable error.""" + import subprocess + + uid = os.getuid() if owner_uid is None else owner_uid + try: + subprocess.run( + [ + "sudo", "-n", "install", "-d", + f"-m{REQUIRED_MODE:o}", + "-o", str(uid), + "-g", str(os.getgid()), + str(target), + ], + capture_output=True, + timeout=15, + check=False, + ) + except Exception: + pass + + +def provisioning_hint(configured: str) -> str: + """The operator-actionable instruction, in one place.""" + return ( + f"Create it as the service account: " + f"sudo install -d -m 0700 -o odin -g odin {configured}" + ) diff --git a/src/web/api/config_admin.py b/src/web/api/config_admin.py index e0e9dd71..32fec9f3 100644 --- a/src/web/api/config_admin.py +++ b/src/web/api/config_admin.py @@ -17,7 +17,7 @@ from aiohttp import web from ... import restart -from ...config.schema import Config +from ...config.schema import Config, active_config_path from ...odin_log import get_logger from ...setup_wizard import ( build_config, @@ -387,7 +387,13 @@ async def update_config(request: web.Request) -> web.Response: # pre-normalized merge, so fields removed from the schema (a legacy # model_routing block, auxiliary tasks/max_tokens/credentials_path) # can't linger on disk after runtime has dropped them. - config_path = Path("config.yml") + # The ACTIVE config, not whatever config.yml sits in the cwd: Odin can + # be launched with `python -m src /somewhere/odin.yml`, and writing to + # the wrong file meant a change appeared in live config and in the + # self-update preflight but vanished on re-exec — contradicting the + # preflight's contract that it validates what the restarted process + # will use (PR #239 round-10 review). llm_admin already does this. + config_path = active_config_path() or Path("config.yml") if config_path.exists(): try: await asyncio.to_thread(_write_config, config_path, new_config.model_dump()) diff --git a/src/web/api/self_update.py b/src/web/api/self_update.py index 56f05be4..795b78a7 100644 --- a/src/web/api/self_update.py +++ b/src/web/api/self_update.py @@ -7,6 +7,9 @@ from __future__ import annotations import asyncio +import os +import subprocess +from pathlib import Path from aiohttp import web @@ -27,7 +30,6 @@ def _repo_root() -> str: preservation joined the wrong directory, and the bot kept reporting the pre-update version from stale package metadata. """ - import os path = os.path.dirname(os.path.abspath(__file__)) for _ in range(8): @@ -79,10 +81,8 @@ async def check_update(_request: web.Request) -> web.Response: @routes.post("/api/update/apply") async def apply_update(request: web.Request) -> web.Response: - import os import re as _re import shutil - import subprocess try: data = await request.json() except Exception: @@ -151,6 +151,24 @@ async def apply_update(request: web.Request) -> web.Response: ), }, status=409) + # PREFLIGHT: the incoming code runs local commands in a validated + # workspace outside the install and REFUSES to run without one. + # This updater re-execs in place, so systemd never starts the + # service and never applies StateDirectory= — meaning an update + # could succeed and leave Odin unable to run any local command + # (PR #239 round-4 review, verified against the live install). + # Provision it here, BEFORE committing to the update, and refuse + # the update rather than transition to code that cannot work. + ws_error = _ensure_local_workspace_for_update(bot, base) + if ws_error: + return web.json_response({ + "error": ( + "update refused: the local command workspace could not be " + f"provisioned, so the updated Odin would be unable to run " + f"local commands. {ws_error}" + ), + }, status=409) + # Reset only the preserved config files for clean pull for fname in _preserve: subprocess.run( @@ -236,3 +254,81 @@ def _rollback(reason: str) -> web.Response: async def stop_all_loops(_request: web.Request) -> web.Response: result = bot.loop_manager.stop_loop("all") return web.json_response({"result": result}) + + +def _ensure_local_workspace_for_update(bot=None, base: str | None = None) -> str | None: + """Provision the local command workspace ahead of an in-place update. + + Returns None on success, or an operator-actionable message on failure. + + Delegates to the SINGLE authoritative implementation rather than + reimplementing a weaker contract here. An independent copy accepted + workspaces the runtime then rejected — a relative path, a symlink, one + inside the install — and could create a directory the restarted executor + would refuse, or provision a different directory from the one it actually + uses (PR #239 round-5 review reproduced four such mismatches). + + The configuration comes from the LIVE bot when available, so the path + checked here is the path the restarted process will use, including any + alternate config file or environment substitution already applied. + """ + from ...tools.workspace import ( + WorkspaceError, + provision_workspace, + provisioning_hint, + ) + + configured = _live_workspace_setting(bot) + try: + provision_workspace(configured, protected_roots=_live_protected_roots(bot, base)) + return None + except WorkspaceError as exc: + return f"{exc} {provisioning_hint(configured)}" + except Exception as exc: # pragma: no cover - defensive + return f"{exc} {provisioning_hint(configured)}" + + +def _live_workspace_setting(bot) -> str: + """The workspace path the RESTARTED process will actually use. + + Returned VERBATIM from the live config, never substituted. Treating a + present-but-blank value as "nothing configured" and validating the default + instead let the preflight approve an update whose restarted process then + failed closed on every local command (PR #239 round-7 review). The schema + normalizes blank to the default, so a blank value here means the live + config is not a validated ToolsConfig — which is exactly when guessing is + least safe. The default is used ONLY when there is no live config at all. + """ + try: + configured = bot.config.tools.local_working_dir + if isinstance(configured, str): + return configured + except Exception: + pass + from ...config.schema import ToolsConfig + + return ToolsConfig().local_working_dir + + +def _live_protected_roots(bot, base: str | None) -> list[str]: + """Install root plus canonical live-data roots, from the LIVE config. + + Delegates to the ONE shared derivation so the preflight cannot approve a + workspace the restarted executor refuses. Deriving them here separately + omitted live memory.json entirely, so with audit/trajectory paths relocated + the updater created a workspace beside memory.json, reported success, and + handed over to an executor that rejected every local command (PR #239 + round-6 review, reproduced). + + The live memory path is read from the running executor when reachable — + that is the value wiring actually supplied — and falls back to the shared + default otherwise. + """ + from src.tools.workspace import DEFAULT_MEMORY_PATH, command_protected_roots + + memory_path = getattr(getattr(bot, "tool_executor", None), "_memory_path", None) + return command_protected_roots( + Path(base).absolute() if base else Path(__file__).absolute().parents[3], + getattr(bot, "config", None), + memory_path=memory_path or DEFAULT_MEMORY_PATH, + ) diff --git a/tests/conftest.py b/tests/conftest.py index 8a7fe5a6..b17dfc2a 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -208,3 +208,27 @@ def _reset_restart_intent(): yield restart.reset() + + +@pytest.fixture(autouse=True, scope="session") +def _test_local_workspace(tmp_path_factory): + """Provision a valid local command workspace for the whole test session. + + ToolExecutor resolves ``tools.local_working_dir`` fail-closed before running + any local command — there is deliberately no fallback to the inherited cwd, + since that inheritance is what let a bare `rm -rf data` delete the live + install on 2026-07-27. Production provisions the directory at deploy time; + tests provision this one, so executor construction never depends on the + host having been deployed to. + """ + from src.config.schema import ToolsConfig + + workspace = tmp_path_factory.mktemp("odin-test-workspace") + workspace.chmod(0o700) + field = ToolsConfig.model_fields["local_working_dir"] + original = field.default + field.default = str(workspace) + ToolsConfig.model_rebuild(force=True) + yield workspace + field.default = original + ToolsConfig.model_rebuild(force=True) diff --git a/tests/test_branch_freshness.py b/tests/test_branch_freshness.py index dcf5dfb2..e99e9533 100644 --- a/tests/test_branch_freshness.py +++ b/tests/test_branch_freshness.py @@ -549,7 +549,7 @@ def executor(self): @pytest.mark.asyncio async def test_annotates_stale_branch_on_test_failure(self, executor): """Test failure on stale branch gets a staleness warning appended.""" - async def mock_exec_command(address, command, ssh_user, timeout=None, on_output=None): + async def mock_exec_command(address, command, ssh_user, timeout=None, on_output=None, **kw): if "pytest" in command: return (1, "3 failed, 10 passed") if "rev-parse" in command: @@ -574,7 +574,7 @@ async def mock_exec_command(address, command, ssh_user, timeout=None, on_output= @pytest.mark.asyncio async def test_no_annotation_on_fresh_branch(self, executor): """Test failure on fresh branch gets no warning.""" - async def mock_exec_command(address, command, ssh_user, timeout=None, on_output=None): + async def mock_exec_command(address, command, ssh_user, timeout=None, on_output=None, **kw): if "pytest" in command: return (1, "3 failed, 10 passed") if "rev-parse" in command: @@ -642,7 +642,7 @@ async def mock_run_on_host(alias, command): @pytest.mark.asyncio async def test_freshness_check_exception_is_safe(self, executor): """If freshness check crashes, the original result is returned.""" - async def mock_exec_command(address, command, ssh_user, timeout=None, on_output=None): + async def mock_exec_command(address, command, ssh_user, timeout=None, on_output=None, **kw): if "pytest" in command: return (1, "3 failed, 10 passed") raise RuntimeError("freshness check boom") @@ -659,7 +659,7 @@ async def mock_exec_command(address, command, ssh_user, timeout=None, on_output= @pytest.mark.asyncio async def test_fetch_failure_tracked(self, executor): """Fetch failures are counted in stats.""" - async def mock_exec_command(address, command, ssh_user, timeout=None, on_output=None): + async def mock_exec_command(address, command, ssh_user, timeout=None, on_output=None, **kw): if "pytest" in command: return (1, "3 failed") if "rev-parse" in command: @@ -690,7 +690,7 @@ def executor(self): @pytest.mark.asyncio async def test_script_with_test_annotated(self, executor): """run_script with test commands gets freshness annotation.""" - async def mock_exec_command(address, command, ssh_user, timeout=None, on_output=None): + async def mock_exec_command(address, command, ssh_user, timeout=None, on_output=None, **kw): if "rev-parse" in command: return (0, "master\n") if "fetch" in command: @@ -715,7 +715,7 @@ async def mock_exec_command(address, command, ssh_user, timeout=None, on_output= @pytest.mark.asyncio async def test_script_non_test_not_annotated(self, executor): """run_script without test commands doesn't get freshness annotation.""" - async def mock_exec_command(address, command, ssh_user, timeout=None, on_output=None): + async def mock_exec_command(address, command, ssh_user, timeout=None, on_output=None, **kw): return (1, "file not found") executor._exec_command = mock_exec_command diff --git a/tests/test_executor_integration_smoke.py b/tests/test_executor_integration_smoke.py index 2a951bf2..070fb7d3 100644 --- a/tests/test_executor_integration_smoke.py +++ b/tests/test_executor_integration_smoke.py @@ -812,7 +812,12 @@ async def test_run_command_multi_per_host_governor(self): assert "ok" in text assert "strict" in text assert exit_code == 1 # a host was denied → not ok - exe._run_on_host.assert_called_once_with("dev", "systemctl restart nginx") + # use_workspace=True is load-bearing: run_command_multi is a raw + # user-command route and must land in the workspace, not the install + # (PR #239). Unrelated tools deliberately omit it. + exe._run_on_host.assert_called_once_with( + "dev", "systemctl restart nginx", use_workspace=True + ) @pytest.mark.asyncio async def test_memory_manage_get_action(self, tmp_path): diff --git a/tests/test_local_workspace.py b/tests/test_local_workspace.py new file mode 100644 index 00000000..228708c0 --- /dev/null +++ b/tests/test_local_workspace.py @@ -0,0 +1,2869 @@ +"""Local command workspace: the 2026-07-27 wipe mechanism, closed. + +The incident: an AE2 jar whose internal layout is ``data/`` was extracted and +then cleaned up with ``rm -rf data``. Local user commands inherited the +service's working directory (the install root), so that relative path resolved +to ``/opt/odin/data`` and deleted Odin's live state. + +Aaron's acceptance bar is explicit: the *exact* three-command workflow must +still succeed — extract, read, clean up — while the install is untouched. So +the headline test replays those three commands for real, in a pytest-owned +temporary fixture. + +Safety of that replay is structural, not conventional. The fixture arranges +things so a REGRESSION is what gets destroyed: the test process's own cwd is +set to the fake install, so if the plumbing ever stopped passing ``cwd=``, the +relative ``rm -rf data`` would delete the fixture's sentinel and the assertion +would fail loudly. Every path involved is under ``tmp_path``, and the test +asserts that before any deletion runs. +""" + +from __future__ import annotations + +import json +import os +import stat +import subprocess +import sys +import threading +import time +import zipfile +from pathlib import Path +from types import SimpleNamespace +from unittest.mock import patch + +import pytest + +from src.config.schema import ToolsConfig +from src.tools.executor import ToolExecutor +from src.tools.process_manager import ProcessRegistry +from src.tools.ssh import run_local_command +from src.tools.workspace import ( + WorkspaceError, + provision_workspace, + resolve_workspace, + workspace_env, +) + + +@pytest.fixture +def fake_install(tmp_path: Path) -> Path: + """A stand-in for /opt/odin, complete with the data dir that got wiped.""" + install = tmp_path / "fake-install" + (install / "data").mkdir(parents=True) + (install / "data" / "sentinel").write_text("live odin state", encoding="utf-8") + return install + + +@pytest.fixture +def workspace(tmp_path: Path) -> Path: + ws = tmp_path / "workspace" + ws.mkdir(mode=0o700) + return ws + + +@pytest.fixture +def ae2_jar(tmp_path: Path) -> Path: + """A jar shaped like the real one: its internal layout starts with data/.""" + jar = tmp_path / "appliedenergistics2-19.2.17.jar" + with zipfile.ZipFile(jar, "w") as zf: + zf.writestr( + "data/ae2/recipe/network/blocks/pattern_providers_interface.json", + '{"type": "ae2:shaped"}', + ) + zf.writestr("data/ae2/recipe/network/crafting/cpu_crafting_unit.json", "{}") + return jar + + +# --- validation: fail closed ------------------------------------------------- + + +def test_valid_workspace_resolves(workspace: Path, fake_install: Path) -> None: + resolved = resolve_workspace(str(workspace), protected_roots=[str(fake_install)]) + assert resolved == workspace.resolve() + + +@pytest.mark.parametrize("value", ["", " ", "relative/path", "./also/relative"]) +def test_rejects_non_absolute_or_empty(value: str, fake_install: Path) -> None: + with pytest.raises(WorkspaceError): + resolve_workspace(value, protected_roots=[str(fake_install)]) + + +def test_rejects_uncreatable_directory(tmp_path: Path, fake_install: Path) -> None: + """A missing workspace whose PARENT also does not exist cannot be + self-provisioned, so it fails closed. (A missing workspace with a writable + parent is created instead — see the upgrade-seamlessness tests.)""" + with pytest.raises(WorkspaceError, match="could not be created"): + resolve_workspace( + str(tmp_path / "no-parent" / "nope"), protected_roots=[str(fake_install)] + ) + + +def test_rejects_file_masquerading_as_directory(tmp_path: Path, fake_install: Path) -> None: + target = tmp_path / "afile" + target.write_text("x", encoding="utf-8") + with pytest.raises(WorkspaceError, match="not a directory"): + resolve_workspace(str(target), protected_roots=[str(fake_install)]) + + +def test_rejects_symlink(tmp_path: Path, workspace: Path, fake_install: Path) -> None: + """A symlink could be repointed later, silently moving every command's cwd.""" + link = tmp_path / "link-to-workspace" + link.symlink_to(workspace) + with pytest.raises(WorkspaceError, match="symlink"): + resolve_workspace(str(link), protected_roots=[str(fake_install)]) + + +def test_rejects_workspace_inside_install(fake_install: Path) -> None: + """The whole point is to be outside the install — inside would reopen it.""" + inside = fake_install / "scratch" + inside.mkdir(mode=0o700) + with pytest.raises(WorkspaceError, match="overlap"): + resolve_workspace(str(inside), protected_roots=[str(fake_install)]) + + +def test_rejects_workspace_inside_data_root(tmp_path: Path, fake_install: Path) -> None: + data_ws = fake_install / "data" / "ws" + data_ws.mkdir(mode=0o700) + with pytest.raises(WorkspaceError, match="overlap"): + resolve_workspace( + str(data_ws), protected_roots=[str(fake_install), str(fake_install / "data")] + ) + + +@pytest.mark.parametrize("mode", [0o755, 0o750, 0o770, 0o300, 0o500, 0o600]) +def test_rejects_any_mode_other_than_0700(mode: int, tmp_path: Path, fake_install: Path) -> None: + """Group/world bits leak a workspace that holds command output; an + owner-only mode like 0300 is private but not readable, which breaks + ordinary use. The contract is exactly 0700 (PR #239 review).""" + target = tmp_path / f"mode-{mode:o}" + target.mkdir(mode=mode) + try: + with pytest.raises(WorkspaceError, match="mode"): + resolve_workspace(str(target), protected_roots=[str(fake_install)]) + finally: + target.chmod(0o700) + + +def test_env_normalizes_pwd_and_oldpwd(workspace: Path) -> None: + """cwd= alone leaves an inherited OLDPWD pointing at the install, so a bare + `cd -` walks straight back in. Both must be normalized.""" + env = workspace_env(workspace, base={"OLDPWD": "/opt/odin", "PWD": "/opt/odin"}) + assert env["PWD"] == str(workspace) + assert env["OLDPWD"] == str(workspace) + + +# --- the acceptance bar: Aaron's exact workflow ------------------------------ + + +@pytest.mark.parametrize("streamed", [False, True], ids=["buffered", "streaming"]) +async def test_incident_workflow_succeeds_without_touching_install( + fake_install: Path, + workspace: Path, + ae2_jar: Path, + tmp_path: Path, + monkeypatch: pytest.MonkeyPatch, + streamed: bool, +) -> None: + """The 2026-07-27 command sequence, replayed as three separate commands. + + Extract, read, clean up — all must SUCCEED (Aaron's bar: don't prevent him + from doing what he was trying to do), while the install's data survives. + """ + # The test process stands in the fake install: if cwd= plumbing regressed, + # the relative rm below could only destroy this fixture, never real data. + monkeypatch.chdir(fake_install) + assert Path.cwd() == fake_install.resolve() + assert tmp_path in fake_install.parents or fake_install.is_relative_to(tmp_path) + + ws = str(resolve_workspace(str(workspace), protected_roots=[str(fake_install)])) + collected: list[str] = [] + cb = (lambda line: collected.append(line)) if streamed else None + + # 1. extract, exactly as he did — relative `data/...` out of the jar + code, out = await run_local_command( + f"jar xf {ae2_jar} data/ae2/recipe/network/blocks/pattern_providers_interface.json" + f" || unzip -o {ae2_jar} 'data/ae2/recipe/network/blocks/*' > /dev/null", + timeout=30, + on_output=cb, + cwd=ws, + ) + assert code == 0, out + extracted = workspace / "data/ae2/recipe/network/blocks/pattern_providers_interface.json" + assert extracted.exists(), "extraction must land in the workspace" + assert not (fake_install / "data" / "ae2").exists(), "must not touch the install" + + # 2. read it back — the actual research task + code, out = await run_local_command( + "cat data/ae2/recipe/network/blocks/pattern_providers_interface.json", + timeout=30, + cwd=ws, + ) + assert code == 0 + assert "ae2:shaped" in out + + # 3. clean up after himself — THE command that caused the incident. + # Bounded by construction: cwd is asserted inside tmp_path above. + assert Path(ws).is_relative_to(tmp_path) + code, _ = await run_local_command("rm -rf data", timeout=30, cwd=ws) + assert code == 0 + + assert not (workspace / "data").exists(), "cleanup must work in the workspace" + # The whole point: + assert (fake_install / "data" / "sentinel").read_text(encoding="utf-8") == "live odin state" + + +async def test_explicit_cd_into_install_still_works( + fake_install: Path, + workspace: Path, +) -> None: + """Aaron's second bar: deliberately operating inside his own install — for + reviews, tests, greps — must remain possible. The default moves; explicit + intent is untouched.""" + ws = str(resolve_workspace(str(workspace), protected_roots=[str(fake_install)])) + + code, out = await run_local_command(f"cd {fake_install} && pwd", timeout=30, cwd=ws) + assert code == 0 + assert out.strip() == str(fake_install) + + # and reading install files by relative path after an explicit cd + code, out = await run_local_command( + f"cd {fake_install} && cat data/sentinel", timeout=30, cwd=ws + ) + assert code == 0 + assert "live odin state" in out + + +async def test_absolute_paths_are_unaffected(fake_install: Path, workspace: Path) -> None: + ws = str(resolve_workspace(str(workspace), protected_roots=[str(fake_install)])) + code, out = await run_local_command(f"cat {fake_install}/data/sentinel", timeout=30, cwd=ws) + assert code == 0 + assert "live odin state" in out + + +async def test_relative_paths_persist_across_commands( + fake_install: Path, + workspace: Path, +) -> None: + """The workspace is STABLE, not per-command: a file written relatively in + one command must still be there for the next. A fresh temp dir per command + would silently break two-step workflows.""" + ws = str(resolve_workspace(str(workspace), protected_roots=[str(fake_install)])) + code, _ = await run_local_command("echo carried > note.txt", timeout=30, cwd=ws) + assert code == 0 + code, out = await run_local_command("cat note.txt", timeout=30, cwd=ws) + assert code == 0 + assert "carried" in out + + +async def test_cd_dash_cannot_return_to_the_install( + fake_install: Path, + workspace: Path, + monkeypatch: pytest.MonkeyPatch, +) -> None: + """OLDPWD normalization: with an inherited OLDPWD the shell's `cd -` would + hop straight back into the install.""" + monkeypatch.setenv("OLDPWD", str(fake_install)) + ws = str(resolve_workspace(str(workspace), protected_roots=[str(fake_install)])) + code, out = await run_local_command("cd - > /dev/null 2>&1; pwd", timeout=30, cwd=ws) + assert code == 0 + assert str(fake_install) not in out + + +# --- background processes: the alternate route ------------------------------- + + +async def test_background_process_uses_the_workspace( + fake_install: Path, + workspace: Path, +) -> None: + """`manage_process start` must not remain a second path to the incident.""" + ws = str(resolve_workspace(str(workspace), protected_roots=[str(fake_install)])) + registry = ProcessRegistry(workspace=ws) + result = await registry.start("localhost", "pwd; sleep 0.2") + assert "Started" in result or "PID" in result + import asyncio + + await asyncio.sleep(0.6) + output = "".join("".join(info.output_buffer) for info in registry._processes.values()) + assert str(workspace) in output + assert str(fake_install) not in output + for info in registry._processes.values(): + if info.status == "running": + await registry.kill(info.pid) + + +def test_registry_without_workspace_inherits_cwd() -> None: + """Internal callers that legitimately need the application directory keep + the old behaviour; the scoping is explicit rather than accidental.""" + assert ProcessRegistry()._workspace is None + + +# --- scoping: remote execution is untouched ---------------------------------- + + +def test_remote_execution_signature_has_no_workspace() -> None: + """The workspace is a LOCAL user-command concern. Remote SSH runs in the + remote account's own home and must not be rewritten by this change.""" + import inspect + + from src.tools.ssh import run_ssh_command + + assert "cwd" not in inspect.signature(run_ssh_command).parameters + + +def test_discord_execution_path_excludes_the_legacy_dag_surfaces( + workspace: Path, fake_install: Path +) -> None: + """The legacy `src/odin/tools/{shell,process}.py` surfaces spawn subprocesses + that inherit the application cwd, so if the Discord path adopted them the + 2026-07-27 mechanism reopens behind this fix. + + Round-2 rewrote this from a grep; round 3 showed it was still vacuous — + `__new__` creates no handler owners, so the loop inspected nothing, and it + reloaded the executor rather than the Discord tool-loop path. This builds a + REAL executor and inspects every resolved owner's module, then imports the + Discord path in a CLEAN interpreter and proves neither legacy module loads. + """ + import subprocess + import sys + + from src.tools.executor import EXECUTOR_HANDLERS + + executor = _executor_with_workspace(workspace, fake_install) + + inspected: list[str] = [] + for tool_name, (owner_key, attr) in EXECUTOR_HANDLERS.items(): + # _handler_owners is the real late-bound registry (RFC-004); owner_key + # is a registry key like "system", not an attribute name. + owner = executor._handler_owners.get(owner_key) + assert owner is not None, f"{tool_name}: handler owner {owner_key!r} did not resolve" + module = type(owner).__module__ + inspected.append(module) + assert not module.startswith("src.odin.tools"), ( + f"{tool_name} is served by legacy module {module}.{attr}" + ) + assert inspected, "no handler owners were inspected — the check would be vacuous" + + # A clean interpreter: importing the Discord execution path must not pull + # the legacy surfaces in, transitively or otherwise. + probe = ( + "import sys; import src.discord.tool_loop; " + "leaked=[m for m in ('src.odin.tools.shell','src.odin.tools.process') " + "if m in sys.modules]; print(','.join(leaked))" + ) + result = subprocess.run( + [sys.executable, "-c", probe], + capture_output=True, text=True, timeout=120, + cwd=str(Path(__file__).resolve().parents[1]), + ) + assert result.returncode == 0, result.stderr[-500:] + leaked = result.stdout.strip() + assert leaked == "", ( + f"the Discord tool-loop path imports {leaked}; those surfaces inherit " + "the application cwd and would reopen the wipe mechanism" + ) + + + +def _executor_with_workspace(workspace: Path, protected: Path): + """A ToolExecutor whose local commands run in ``workspace``.""" + from src.config.schema import ToolHost, ToolsConfig + from src.tools.executor import ToolExecutor + + config = ToolsConfig( + local_working_dir=str(workspace), + # A resolvable localhost so tests drive the PRODUCTION dispatch route + # rather than the shared _exec_command primitive. + hosts={"localhost": ToolHost(address="127.0.0.1")}, + ) + executor = ToolExecutor(config=config) + # Protected roots are normally derived from the running app; point them at + # the fixture so the test exercises real validation against a fake install. + executor._protected_roots = lambda: [str(protected)] # type: ignore[method-assign] + return executor + + +async def _run_command(executor, command: str) -> tuple[int, str]: + """Drive the REAL run_command tool, end to end. + + Deliberately not `_exec_command`: since round 8 the workspace is opt-in at + the call site, because that shared primitive also backs git_ops, docker, + terraform, kubectl, claude_code and PDF host reads, whose cwd semantics + must not change. Testing the primitive would therefore no longer prove + that the tool Odin actually calls gets the workspace — removing + `use_workspace=True` from the run_command handler has to fail these tests. + """ + result = await executor.execute("run_command", {"command": command, "host": "localhost"}) + return (0 if result.ok else 1), str(result.output) + + +async def test_executor_replays_the_incident_without_touching_the_install( + fake_install: Path, + workspace: Path, + ae2_jar: Path, + tmp_path: Path, + monkeypatch: pytest.MonkeyPatch, +) -> None: + """The 2026-07-27 sequence through the EXECUTOR — the seam that failed. + + Deleting the executor's cwd argument must break this test, which the + low-level replay could not detect. + """ + monkeypatch.chdir(fake_install) # a regression can only destroy the fixture + assert Path.cwd() == fake_install.resolve() + executor = _executor_with_workspace(workspace, fake_install) + + code, out = await _run_command( + executor, + f"jar xf {ae2_jar} data/ae2/recipe/network/blocks/pattern_providers_interface.json" + f" || unzip -o {ae2_jar} 'data/ae2/recipe/network/blocks/*' > /dev/null", + ) + assert code == 0, out + assert (workspace / "data/ae2/recipe/network/blocks").exists() + assert not (fake_install / "data" / "ae2").exists() + + code, out = await _run_command( + executor, "cat data/ae2/recipe/network/blocks/pattern_providers_interface.json" + ) + assert code == 0 and "ae2:shaped" in out + + assert Path(str(workspace)).is_relative_to(tmp_path) # bounded before deleting + code, _ = await _run_command(executor, "rm -rf data") + assert code == 0 + assert not (workspace / "data").exists(), "cleanup must work" + assert (fake_install / "data" / "sentinel").read_text(encoding="utf-8") == "live odin state" + + +async def test_executor_pwd_is_the_workspace(fake_install: Path, workspace: Path) -> None: + executor = _executor_with_workspace(workspace, fake_install) + code, out = await _run_command(executor, "pwd") + assert code == 0 + assert out.strip() == str(workspace.resolve()) + + +async def test_executor_explicit_cd_into_install_still_works( + fake_install: Path, workspace: Path +) -> None: + """Aaron's bar: deliberately working inside his own install is untouched.""" + executor = _executor_with_workspace(workspace, fake_install) + code, out = await _run_command(executor, f"cd {fake_install} && cat data/sentinel") + assert code == 0 and "live odin state" in out + + +async def test_executor_background_process_uses_the_workspace( + fake_install: Path, workspace: Path +) -> None: + """manage_process must not remain an alternate route to the incident, and + the registry must get its workspace FROM the executor.""" + executor = _executor_with_workspace(workspace, fake_install) + registry = executor._ensure_process_registry() + # A RESOLVER, not a frozen string: each spawn re-verifies the workspace's + # mutable filesystem invariants (PR #239 round-3). + assert callable(registry._workspace) + assert registry._resolve_workspace() == str(workspace.resolve()) + + await registry.start("localhost", "pwd; sleep 0.2") + import asyncio + + await asyncio.sleep(0.6) + output = "".join("".join(i.output_buffer) for i in registry._processes.values()) + assert str(workspace.resolve()) in output + assert str(fake_install) not in output + for info in registry._processes.values(): + if info.status == "running": + await registry.kill(info.pid) + + +async def test_executor_refuses_to_run_with_an_invalid_workspace( + fake_install: Path, tmp_path: Path +) -> None: + """Fail closed at the point of use: no subprocess may run against an + unvalidated cwd, and there is deliberately no fallback to the inherited + directory — that fallback is the hazard.""" + executor = _executor_with_workspace(tmp_path / "no-parent" / "ws", fake_install) + with pytest.raises(WorkspaceError): + await executor._exec_command( + "localhost", "echo should-not-run", timeout=10, use_workspace=True + ) + + +# --- protected roots: real deployment shapes -------------------------------- + + +def test_rejects_workspace_inside_symlinked_live_data(tmp_path: Path) -> None: + """Packaged installs symlink /opt/odin/data -> /var/lib/odin. A string-joined + check would accept a workspace sitting inside the REAL data directory.""" + install = tmp_path / "opt-odin" + install.mkdir() + real_data = tmp_path / "var-lib-odin" + real_data.mkdir() + (install / "data").symlink_to(real_data) + ws = real_data / "workspace" + ws.mkdir(mode=0o700) + with pytest.raises(WorkspaceError, match="overlap"): + resolve_workspace(str(ws), protected_roots=[str(install), str(install / "data")]) + + +def test_rejects_workspace_inside_non_opt_install_root(tmp_path: Path) -> None: + """Docker's install root is /app and source checkouts are arbitrary — the + validator must use the roots it is given, not a hardcoded /opt/odin.""" + app = tmp_path / "app" + (app / "sub").mkdir(parents=True) + (app / "sub").chmod(0o700) + with pytest.raises(WorkspaceError, match="overlap"): + resolve_workspace(str(app / "sub"), protected_roots=[str(app)]) + + +def test_rejects_workspace_that_contains_a_protected_root(tmp_path: Path) -> None: + """Overlap is bidirectional: a workspace that CONTAINS the install is just + as unusable as one nested inside it.""" + parent = tmp_path / "parent" + install = parent / "opt-odin" + install.mkdir(parents=True) + parent.chmod(0o700) + with pytest.raises(WorkspaceError, match="overlap"): + resolve_workspace(str(parent), protected_roots=[str(install)]) + + +def test_declared_state_path_protects_lexical_alias_and_canonical_target( + tmp_path: Path, +) -> None: + """A live-state path may contain a symlinked directory component. Protect + both the alias traversed by runtime code and the canonical target it reaches. + """ + from src.config.schema import Config + from src.tools.workspace import command_protected_roots + + install = tmp_path / "install" + install.mkdir() + workspace = tmp_path / "workspace" + workspace.mkdir(mode=0o700) + live = tmp_path / "live" + (live / "sessions").mkdir(parents=True) + (workspace / "state").symlink_to(live, target_is_directory=True) + config = Config( + discord={"token": "fake"}, + sessions={"persist_directory": str(workspace / "state" / "sessions")}, + ) + + roots = command_protected_roots( + install, + config, + memory_path=tmp_path / "memory" / "memory.json", + ) + assert str(workspace / "state" / "sessions") in roots + assert str((live / "sessions").resolve()) in roots + with pytest.raises(WorkspaceError, match="overlap"): + resolve_workspace(str(workspace), protected_roots=roots) + + +def test_install_root_protects_lexical_alias_and_canonical_checkout(tmp_path: Path) -> None: + """In-place re-exec needs the checkout's launch alias as well as its target.""" + from src.tools.workspace import command_protected_roots + + workspace = tmp_path / "workspace" + workspace.mkdir(mode=0o700) + real_install = tmp_path / "real-install" + real_install.mkdir() + (workspace / "checkout").symlink_to(real_install, target_is_directory=True) + + roots = command_protected_roots( + workspace / "checkout", + memory_path=tmp_path / "memory" / "memory.json", + ) + assert str(workspace / "checkout") in roots + assert str(real_install.resolve()) in roots + with pytest.raises(WorkspaceError, match="overlap"): + resolve_workspace(str(workspace), protected_roots=roots) + + +def _symlinked_checkout_layout(tmp_path: Path) -> tuple[Path, Path]: + workspace = tmp_path / "workspace" + workspace.mkdir(mode=0o700) + real_install = tmp_path / "real-install" + real_install.mkdir() + checkout = workspace / "checkout" + checkout.symlink_to(real_install, target_is_directory=True) + return workspace, checkout + + +def test_executor_passes_lexical_install_root_to_shared_derivation( + tmp_path: Path, monkeypatch: pytest.MonkeyPatch +) -> None: + import src.tools.executor as executor_module + + workspace, checkout = _symlinked_checkout_layout(tmp_path) + monkeypatch.setattr(executor_module, "__file__", str(checkout / "src/tools/executor.py")) + executor = ToolExecutor( + ToolsConfig(local_working_dir=str(workspace)), + memory_path=str(tmp_path / "memory/memory.json"), + ) + with pytest.raises(WorkspaceError, match="overlap"): + executor._ensure_local_workspace() + + +def test_startup_passes_lexical_install_root_to_shared_derivation( + tmp_path: Path, monkeypatch: pytest.MonkeyPatch +) -> None: + import src.__main__ as entrypoint + from src.config.schema import Config + + workspace, checkout = _symlinked_checkout_layout(tmp_path) + monkeypatch.setattr(entrypoint, "__file__", str(checkout / "src/__main__.py")) + config = Config( + discord={"token": "fake"}, + tools={"local_working_dir": str(workspace)}, + ) + roots = entrypoint._command_protected_roots(config) + with pytest.raises(WorkspaceError, match="overlap"): + resolve_workspace(str(workspace), protected_roots=roots) + + +def test_updater_passes_lexical_install_root_to_shared_derivation(tmp_path: Path) -> None: + from src.config.schema import Config + from src.web.api.self_update import _live_protected_roots + + workspace, checkout = _symlinked_checkout_layout(tmp_path) + bot = SimpleNamespace( + config=Config( + discord={"token": "fake"}, + tools={"local_working_dir": str(workspace)}, + ), + tool_executor=SimpleNamespace( + _memory_path=tmp_path / "memory/memory.json", + ), + ) + roots = _live_protected_roots(bot, str(checkout)) + with pytest.raises(WorkspaceError, match="overlap"): + resolve_workspace(str(workspace), protected_roots=roots) + + +def test_rejects_wrong_owner(tmp_path: Path, fake_install: Path) -> None: + """The contract is a directory owned by the execution identity.""" + ws = tmp_path / "otherowner" + ws.mkdir(mode=0o700) + with pytest.raises(WorkspaceError, match="owned by uid"): + resolve_workspace( + str(ws), protected_roots=[str(fake_install)], owner_uid=os.getuid() + 12345 + ) + + +# --- provisioning: the repo must ship installable --------------------------- + + +def test_every_install_path_provisions_the_workspace() -> None: + """PR #239 review, blocker 2: the default fails closed when absent, so a + deployment that never creates it loses local-command capability on first + use. Each supported install path must provision it.""" + repo = Path(__file__).resolve().parents[1] + default = "/var/lib/odin-workspace" + + postinstall = (repo / "packaging/postinstall.sh").read_text(encoding="utf-8") + assert default in postinstall + assert "chmod 0700" in postinstall + + dockerfile = (repo / "Dockerfile").read_text(encoding="utf-8") + assert default in dockerfile and "0700" in dockerfile + + compose = (repo / "docker-compose.yml").read_text(encoding="utf-8") + assert default in compose, "the workspace must persist across container restarts" + + +async def test_executor_enforces_its_protected_roots(fake_install: Path) -> None: + """The executor must PASS its derived roots to the validator, not merely + own them. Without this pin, dropping `protected_roots=` from the executor + leaves every other test green while an inside-the-install workspace is + silently accepted.""" + inside = fake_install / "scratch" + inside.mkdir(mode=0o700) + executor = _executor_with_workspace(inside, fake_install) + with pytest.raises(WorkspaceError, match="overlap"): + await executor._exec_command( + "localhost", "echo should-not-run", timeout=10, use_workspace=True + ) + + +def test_executor_derives_real_roots_from_the_running_app() -> None: + """Roots come from the running application, not a hardcoded /opt/odin — + otherwise Docker (/app) and source checkouts get no protection at all.""" + from src.config.schema import ToolsConfig + from src.tools.executor import ToolExecutor + + executor = ToolExecutor(config=ToolsConfig()) + roots = [Path(r).resolve() for r in executor._protected_roots()] + repo = Path(__file__).resolve().parents[1] + assert repo.resolve() in roots, "the actual install/package root must be protected" + assert any("data" in str(r) for r in roots), "the live-data root must be protected" + + +def test_memory_path_directory_is_protected_when_other_paths_relocate(tmp_path: Path) -> None: + """PR #239 round-2 blocker 1, reproduced by Odin as UNSAFE_ACCEPTED. + + Deriving data roots from audit/trajectory alone means that if those are + configured elsewhere, a workspace sitting beside the live memory.json is + accepted. memory.json is the most valuable file in the tree. + """ + from src.config.schema import ToolsConfig + from src.tools.executor import ToolExecutor + + live_data = tmp_path / "var-lib-odin" + live_data.mkdir() + (live_data / "memory.json").write_text("{}", encoding="utf-8") + workspace = live_data / "workspace" # beside the live memory file + workspace.mkdir(mode=0o700) + + # audit + trajectories deliberately relocated away from the data root + elsewhere = tmp_path / "elsewhere" + elsewhere.mkdir() + config = ToolsConfig( + local_working_dir=str(workspace), + audit_log_path=str(elsewhere / "audit.jsonl"), + trajectory_path=str(elsewhere / "trajectories"), + ) + executor = ToolExecutor(config=config, memory_path=str(live_data / "memory.json")) + + roots = [Path(r).resolve() for r in executor._protected_roots()] + assert live_data.resolve() in roots, "the live memory directory must be protected" + with pytest.raises(WorkspaceError, match="overlap"): + executor._ensure_local_workspace() + + +def test_path_classification_is_declared_not_guessed(tmp_path: Path) -> None: + """A `Path.suffix` heuristic misreads dotted directories and extensionless + files; classification comes from declared semantics instead.""" + from src.config.schema import ToolsConfig + from src.tools.executor import ToolExecutor + + dotted_dir = tmp_path / "traj" / "trajectories.d" # a DIRECTORY with a suffix + dotted_dir.mkdir(parents=True) + audit_dir = tmp_path / "audit" + audit_dir.mkdir() + executor = ToolExecutor( + config=ToolsConfig( + audit_log_path=str(audit_dir / "audit.jsonl"), # declared FILE + trajectory_path=str(dotted_dir), # declared DIRECTORY + ) + ) + roots = [Path(r).resolve() for r in executor._protected_roots()] + # The dotted directory is protected as ITSELF; a suffix heuristic would + # have mistaken it for a file and protected its parent instead. + assert dotted_dir.resolve() in roots + assert dotted_dir.parent.resolve() not in roots, "suffix heuristic would over-protect" + # The audit FILE contributes its parent directory, which is correct. + assert audit_dir.resolve() in roots + + +async def test_workspace_is_revalidated_before_every_command( + fake_install: Path, workspace: Path, tmp_path: Path, monkeypatch: pytest.MonkeyPatch +) -> None: + """PR #239 round-3 blocker 1, reproduced by Odin: caching the validated + path meant fail-closed applied only to the FIRST command. Replacing the + directory with a symlink into the install afterwards was accepted, and the + second command read live data.""" + monkeypatch.chdir(fake_install) + executor = _executor_with_workspace(workspace, fake_install) + + code, out = await _run_command(executor, "pwd") + assert code == 0 and out.strip() == str(workspace.resolve()) + + # Swap the validated directory for a symlink pointing into the install. + workspace.rmdir() + workspace.symlink_to(fake_install) + with pytest.raises(WorkspaceError): + await executor._exec_command( + "localhost", "cat data/sentinel", timeout=30, use_workspace=True + ) + + +async def test_mode_change_after_first_command_is_caught( + fake_install: Path, workspace: Path +) -> None: + """A post-validation chmod must not be ignored either.""" + executor = _executor_with_workspace(workspace, fake_install) + assert (await _run_command(executor, "pwd"))[0] == 0 + workspace.chmod(0o755) + try: + with pytest.raises(WorkspaceError, match="mode"): + await executor._exec_command( + "localhost", "pwd", timeout=30, use_workspace=True + ) + finally: + workspace.chmod(0o700) + + +async def test_background_spawn_revalidates_too( + fake_install: Path, workspace: Path +) -> None: + """Verification must bind to background spawns as well, or manage_process + becomes the surviving route past a swapped workspace.""" + executor = _executor_with_workspace(workspace, fake_install) + registry = executor._ensure_process_registry() + assert registry._resolve_workspace() == str(workspace.resolve()) + workspace.chmod(0o750) + try: + with pytest.raises(WorkspaceError): + registry._resolve_workspace() + finally: + workspace.chmod(0o700) + + +def test_leaf_symlinked_data_paths_protect_the_target(tmp_path: Path) -> None: + """PR #239 round-3 blocker 2, reproduced as UNSAFE_ACCEPTED: taking .parent + BEFORE canonicalization protects the alias directory rather than the real + data directory, so a workspace beside the true memory.json was accepted.""" + from src.config.schema import ToolsConfig + from src.tools.executor import ToolExecutor + + live = tmp_path / "live-data" + live.mkdir() + (live / "memory.json").write_text("{}", encoding="utf-8") + (live / "audit.jsonl").write_text("", encoding="utf-8") + aliases = tmp_path / "aliases" + aliases.mkdir() + (aliases / "memory.json").symlink_to(live / "memory.json") + (aliases / "audit.jsonl").symlink_to(live / "audit.jsonl") + + workspace = live / "workspace" # beside the REAL data + workspace.mkdir(mode=0o700) + + executor = ToolExecutor( + config=ToolsConfig( + local_working_dir=str(workspace), + audit_log_path=str(aliases / "audit.jsonl"), + trajectory_path=str(tmp_path / "elsewhere"), + ), + memory_path=str(aliases / "memory.json"), + ) + roots = [Path(r).resolve() for r in executor._protected_roots()] + assert live.resolve() in roots, "the symlink TARGET's directory must be protected" + with pytest.raises(WorkspaceError, match="overlap"): + executor._ensure_local_workspace() + + +# --- workspace growth metrics (the operational half of "no auto-prune") ----- + + +def _metrics_after_refresh(executor, timeout: float = 5.0) -> dict[str, float]: + """Scrape, wait for the off-thread usage walk, scrape again. + + Usage is refreshed in the background since round 10 — /metrics is served on + the event loop and this directory never prunes, so the walk must not run on + the calling thread. Tests that assert on usage therefore have to let the + refresh land. + """ + executor.get_workspace_metrics() + deadline = time.monotonic() + timeout + while executor._workspace_usage_cache is None and time.monotonic() < deadline: + time.sleep(0.02) + return executor.get_workspace_metrics() + + +def test_workspace_metrics_report_usage(workspace: Path, fake_install: Path) -> None: + """No automatic pruning was always paired with observability, so growth is + alertable and cleanup stays an explicit operator action.""" + executor = _executor_with_workspace(workspace, fake_install) + (workspace / "artifact.bin").write_bytes(b"x" * 2048) + (workspace / "nested").mkdir() + (workspace / "nested" / "more.txt").write_text("hello", encoding="utf-8") + + metrics = _metrics_after_refresh(executor) + assert metrics["files"] == 2 + assert metrics["bytes"] >= 2048 + assert metrics["free_bytes"] > 0 + assert metrics["free_inodes"] > 0 + + +def test_workspace_metrics_never_raise_on_an_invalid_workspace( + tmp_path: Path, fake_install: Path +) -> None: + """Metrics collection must not be able to break a command path.""" + executor = _executor_with_workspace(tmp_path / "no-parent" / "missing", fake_install) + assert executor.get_workspace_metrics() == {} + + +def test_workspace_gauges_render_for_prometheus( + workspace: Path, fake_install: Path +) -> None: + from src.health.metrics import MetricsCollector + + executor = _executor_with_workspace(workspace, fake_install) + (workspace / "f").write_bytes(b"12345") + _metrics_after_refresh(executor) # let the background usage walk land + collector = MetricsCollector() + collector.register_source("workspace", executor.get_workspace_metrics) + rendered = collector.render() + for name in ( + "odin_workspace_bytes", + "odin_workspace_files", + "odin_workspace_free_bytes", + "odin_workspace_free_inodes", + ): + assert f"# TYPE {name} gauge" in rendered + assert any( + line.startswith(f"{name} ") for line in rendered.splitlines() + ), f"{name} value line missing" + + +def test_workspace_gauges_tolerate_partial_and_failing_sources() -> None: + """A partial dict renders what it has; a raising source is swallowed so the + metrics endpoint cannot be taken down by workspace trouble.""" + from src.health.metrics import MetricsCollector + + partial = MetricsCollector() + partial.register_source("workspace", lambda: {"bytes": 10.0}) + rendered = partial.render() + assert "odin_workspace_bytes 10" in rendered + assert "odin_workspace_free_inodes" not in rendered + + def _boom() -> dict[str, float]: + raise OSError("filesystem unavailable") + + failing = MetricsCollector() + failing.register_source("workspace", _boom) + assert "odin_workspace_bytes" not in failing.render() + + empty = MetricsCollector() + empty.register_source("workspace", dict) + assert "odin_workspace_bytes" not in empty.render() + + +def test_blank_configured_data_paths_are_skipped(tmp_path: Path, workspace: Path) -> None: + """A blank/whitespace data path contributes no protected root rather than + protecting the process's current directory by accident.""" + from src.config.schema import ToolsConfig + from src.tools.executor import ToolExecutor + + executor = ToolExecutor( + config=ToolsConfig( + local_working_dir=str(workspace), + audit_log_path=" ", + trajectory_path="", + ) + ) + roots = [Path(r).resolve() for r in executor._protected_roots()] + assert Path.cwd().resolve() not in roots or roots.count(Path.cwd().resolve()) <= 1 + assert roots, "the install root must still be protected" + + +def test_workspace_metrics_degrade_when_filesystem_stats_fail( + workspace: Path, fake_install: Path, monkeypatch: pytest.MonkeyPatch +) -> None: + """Free-space/inode probes are best-effort: if the filesystem refuses them + the usage figures still report, because breaking metrics must never break + a command path.""" + import shutil as _shutil + + executor = _executor_with_workspace(workspace, fake_install) + (workspace / "f").write_bytes(b"abc") + + def _fail_usage(_path: object) -> object: + raise OSError("statfs unavailable") + + def _fail_statvfs(_path: object) -> object: + raise OSError("statvfs unavailable") + + metrics = _metrics_after_refresh(executor) # usage lands before we break statfs + assert metrics["bytes"] == 3 and metrics["files"] == 1 + + monkeypatch.setattr(_shutil, "disk_usage", _fail_usage) + monkeypatch.setattr(os, "statvfs", _fail_statvfs) + + metrics = executor.get_workspace_metrics() + assert metrics["bytes"] == 3 + assert metrics["files"] == 1 + assert "free_bytes" not in metrics + assert "free_inodes" not in metrics + + +# --- upgrade seamlessness: fresh installs AND updates ------------------------ + + +def test_workspace_is_self_provisioned_when_the_parent_is_writable( + tmp_path: Path, fake_install: Path +) -> None: + """Upgrades must be seamless. A source checkout or a git-based self-update + lands on new code whose systemd unit was never refreshed, so the runtime + creates the workspace itself when it can — with the same 0700 contract a + deployment-provisioned one satisfies.""" + target = tmp_path / "fresh-workspace" + assert not target.exists() + resolved = resolve_workspace(str(target), protected_roots=[str(fake_install)]) + assert resolved == target.resolve() + assert target.is_dir() + assert stat.S_IMODE(target.stat().st_mode) == 0o700, "must not inherit a loose umask" + + + + + +def test_unwritable_parent_still_fails_closed_with_actionable_guidance( + tmp_path: Path, fake_install: Path +) -> None: + """When self-provisioning cannot work (root-owned /var/lib for a non-root + service account), the error names the exact command to run rather than + silently degrading.""" + with pytest.raises(WorkspaceError) as excinfo: + resolve_workspace( + str(tmp_path / "missing-parent" / "ws"), + protected_roots=[str(fake_install)], + ) + message = str(excinfo.value) + assert "could not be created" in message + assert "install -d -m 0700" in message, "the error must be actionable" + + +def test_creation_can_be_disabled_for_strict_callers( + tmp_path: Path, fake_install: Path +) -> None: + target = tmp_path / "never-created" + with pytest.raises(WorkspaceError, match="could not be created"): + resolve_workspace( + str(target), protected_roots=[str(fake_install)], create_if_missing=False + ) + assert not target.exists() + + +def test_every_upgrade_path_provisions_the_workspace() -> None: + """Fresh installs AND updates must both land working, with no manual step: + + - .deb install/upgrade -> postinstall creates it + - any systemd start -> StateDirectory= recreates it (covers restarts + after a self-update, even if postinstall never ran) + - Docker -> image directory + named volume + - source / git self-update -> runtime self-provisioning (tested above) + """ + repo = Path(__file__).resolve().parents[1] + + unit = (repo / "packaging/odin.service").read_text(encoding="utf-8") + assert "StateDirectory=odin-workspace" in unit + assert "StateDirectoryMode=0700" in unit + + postinstall = (repo / "packaging/postinstall.sh").read_text(encoding="utf-8") + assert "/var/lib/odin-workspace" in postinstall and "chmod 0700" in postinstall + + dockerfile = (repo / "Dockerfile").read_text(encoding="utf-8") + assert "/var/lib/odin-workspace" in dockerfile and "0700" in dockerfile + + compose = (repo / "docker-compose.yml").read_text(encoding="utf-8") + assert "odin-workspace:/var/lib/odin-workspace" in compose, "named volume, not a bind" + + +# --- round 4: the paths that would have broken YOUR live install ------------ + + +def test_self_provisioning_never_creates_inside_a_protected_root( + fake_install: Path, +) -> None: + """PR #239 round-4 blocker 4, reproduced by Odin: the mkdir ran BEFORE the + overlap check, so an invalid workspace was created inside the very tree + this protects and only then rejected. Fail-closed must not mean 'reject + after modifying the place we promised not to touch'. + + (The previous assertion here was a tautology that passed either way.) + """ + target = fake_install / "never-create-me" + with pytest.raises(WorkspaceError, match="overlap"): + resolve_workspace(str(target), protected_roots=[str(fake_install)]) + assert not target.exists(), "the directory must NOT have been created" + + +async def test_background_workspace_refusal_is_visible_as_an_error( + fake_install: Path, workspace: Path +) -> None: + """PR #239 round-4 blocker 3: a workspace refusal came back as a plain + string that did not match the error-prefix contract, so the tool loop + classified a refusal as a SUCCESSFUL start.""" + from src.tools.tool_text import _ERROR_RESULT_PREFIXES + + executor = _executor_with_workspace(workspace, fake_install) + registry = executor._ensure_process_registry() + workspace.chmod(0o755) + try: + result = await registry.start("localhost", "echo should-not-run") + finally: + workspace.chmod(0o700) + assert result.startswith(_ERROR_RESULT_PREFIXES), ( + f"refusal must be visible as a failure, got: {result!r}" + ) + assert "mode" in result, "the operator still needs the reason" + + +def test_incus_deployment_path_provisions_the_workspace() -> None: + """PR #239 round-4 blocker 2: Incus is an executable deployment path and + was missed — its unprivileged odin user cannot create /var/lib/odin-workspace.""" + repo = Path(__file__).resolve().parents[1] + script = (repo / "scripts/incus-deploy.sh").read_text(encoding="utf-8") + assert "/var/lib/odin-workspace" in script + assert "chmod 0700 /var/lib/odin-workspace" in script + assert "StateDirectory=odin-workspace" in script + assert "StateDirectoryMode=0700" in script + + +# --- round 5: one authoritative provisioner, and the bootstrap it must survive + + +def _fake_bot(workspace: Path, install: Path): + """A bot-shaped object exposing only what the preflight reads.""" + class _Tools: + local_working_dir = str(workspace) + audit_log_path = str(install / "data" / "audit.jsonl") + trajectory_path = str(install / "data" / "trajectories") + + class _Config: + tools = _Tools() + + class _Bot: + config = _Config() + + return _Bot() + + +def test_provision_workspace_is_the_single_contract(tmp_path: Path, fake_install: Path) -> None: + """provision_workspace creates AND fully validates, so no caller can accept + a workspace the runtime would reject.""" + from src.tools.workspace import provision_workspace + + target = tmp_path / "ws" + result = provision_workspace(str(target), protected_roots=[str(fake_install)]) + assert result == target.resolve() + assert stat.S_IMODE(target.stat().st_mode) == 0o700 + + +@pytest.mark.parametrize( + "case", + ["relative", "symlink", "inside_install", "wrong_owner"], + ids=["relative-path", "workspace-symlink", "inside-install", "wrong-owner"], +) +def test_provisioner_rejects_everything_the_runtime_rejects( + case: str, tmp_path: Path, fake_install: Path, monkeypatch: pytest.MonkeyPatch +) -> None: + """PR #239 round-5 blocker 2: the updater had a SECOND, weaker contract and + accepted four things the runtime refuses. One implementation now serves + both, so these cannot diverge again.""" + from src.tools.workspace import provision_workspace + + if case == "relative": + monkeypatch.chdir(tmp_path) + configured = "relative-ws" + elif case == "symlink": + real = tmp_path / "real" + real.mkdir(mode=0o700) + link = tmp_path / "link" + link.symlink_to(real) + configured = str(link) + elif case == "inside_install": + configured = str(fake_install / "ws") + else: + target = tmp_path / "owned-elsewhere" + target.mkdir(mode=0o700) + configured = str(target) + + kwargs = {"protected_roots": [str(fake_install)], "allow_sudo": False} + if case == "wrong_owner": + kwargs["owner_uid"] = os.getuid() + 4242 + + with pytest.raises(WorkspaceError): + provision_workspace(configured, **kwargs) + + if case == "inside_install": + assert not (fake_install / "ws").exists(), "must not create inside the install" + if case == "relative": + assert not (tmp_path / "relative-ws").exists(), "must not create a relative workspace" + + +def test_legacy_source_config_falls_back_when_var_lib_cannot_be_provisioned( + tmp_path: Path, fake_install: Path, monkeypatch: pytest.MonkeyPatch +) -> None: + """The first update to this feature is executed by old updater code. + + A pre-PR source config has no workspace field, and its unprivileged account + cannot normally create /var/lib/odin-workspace or use sudo. Preserve local + command capability with a stable HOME fallback, but only for that absent + legacy field. + """ + import src.tools.workspace as ws_module + from src.config.schema import DEFAULT_LOCAL_WORKING_DIR + + home = tmp_path / "home" + home.mkdir() + monkeypatch.setenv("HOME", str(home)) + # Simulate the validated shape produced from a pre-feature config file. + # The session fixture changes ToolsConfig's default for unrelated tests, so + # model this migration boundary explicitly rather than accidentally testing + # the fixture's temporary value. + tools = SimpleNamespace( + local_working_dir=DEFAULT_LOCAL_WORKING_DIR, + model_fields_set=set(), + ) + real_provision = ws_module.provision_workspace + + def _default_unavailable(configured: str, **kwargs): + if configured == DEFAULT_LOCAL_WORKING_DIR: + raise WorkspaceError("simulated root-owned /var/lib and denied sudo") + return real_provision(configured, **kwargs) + + monkeypatch.setattr(ws_module, "provision_workspace", _default_unavailable) + result = ws_module.provision_startup_workspace( + tools, protected_roots=[str(fake_install)] + ) + + assert result == (home / ".odin-workspace").resolve() + assert stat.S_IMODE(result.stat().st_mode) == 0o700 + assert tools.local_working_dir == str(result), "all runtime consumers must see the migration" + + +def test_explicit_workspace_never_uses_the_legacy_source_fallback( + tmp_path: Path, fake_install: Path, monkeypatch: pytest.MonkeyPatch +) -> None: + """Only a missing pre-feature field migrates; an explicit operator value is + authoritative even when unusable.""" + import src.tools.workspace as ws_module + home = tmp_path / "home" + home.mkdir() + monkeypatch.setenv("HOME", str(home)) + explicit = tmp_path / "missing-parent" / "operator-workspace" + tools = SimpleNamespace( + local_working_dir=str(explicit), + model_fields_set={"local_working_dir"}, + ) + + def _unavailable(_configured: str, **_kwargs): + raise WorkspaceError("operator workspace unavailable") + + monkeypatch.setattr(ws_module, "provision_workspace", _unavailable) + with pytest.raises(WorkspaceError, match="operator workspace unavailable"): + ws_module.provision_startup_workspace( + tools, protected_roots=[str(fake_install)] + ) + assert tools.local_working_dir == str(explicit) + assert not (home / ".odin-workspace").exists() + + +def test_startup_migration_provisions_before_commands_are_served() -> None: + """PR #239 round-5 blocker 1 — the bootstrap paradox. + + The self-update preflight is NEW code, so the update that installs it is + executed by the PREVIOUS release's handler, which has no preflight. Only a + startup migration in the incoming code can bootstrap the workspace, because + that runs after re-exec however the update arrived. + """ + main_src = (Path(__file__).resolve().parents[1] / "src/__main__.py").read_text( + encoding="utf-8" + ) + assert "provision_startup_workspace(" in main_src, "startup must provision the workspace" + provision_at = main_src.index("provision_startup_workspace(") + bot_at = main_src.index("bot = OdinBot(config)") + config_at = main_src.index("config = load_config(config_path)") + assert config_at < provision_at < bot_at, ( + "provisioning must run after the real config loads and before the bot " + "(and therefore command service) is constructed" + ) + # Failure must not prevent Odin from starting and answering on Discord. + assert "never block startup" in main_src or "not fatal" in main_src + + +def test_preflight_uses_the_live_config_not_a_reparsed_file( + tmp_path: Path, fake_install: Path +) -> None: + """The preflight must validate the path the RESTARTED process will use. + Re-reading config.yml missed alternate config paths and environment + substitution, and could provision a different directory entirely.""" + from src.web.api.self_update import _live_workspace_setting + + workspace = tmp_path / "live-ws" + workspace.mkdir(mode=0o700) + bot = _fake_bot(workspace, fake_install) + assert _live_workspace_setting(bot) == str(workspace) + + +async def test_preflight_refuses_a_workspace_the_runtime_would_reject( + fake_install: Path, +) -> None: + """End-to-end: a workspace inside the install must be refused by the + preflight, and nothing may be created.""" + from src.web.api.self_update import _ensure_local_workspace_for_update + + target = fake_install / "ws-inside" + bot = _fake_bot(target, fake_install) + message = _ensure_local_workspace_for_update(bot, str(fake_install)) + assert message is not None + assert "overlap" in message + assert "install -d -m 0700" in message + assert not target.exists(), "a refused preflight must not create anything" + + +def test_preflight_falls_back_to_the_schema_default_without_a_bot( + tmp_path: Path, +) -> None: + """Called without a live bot (or with one that cannot answer), the setting + still resolves to the schema default rather than guessing or crashing.""" + from src.config.schema import ToolsConfig + from src.web.api.self_update import _live_workspace_setting + + class _Broken: + @property + def config(self): # noqa: ANN201 - deliberately raises + raise RuntimeError("bot not ready") + + assert _live_workspace_setting(None) == ToolsConfig().local_working_dir + assert _live_workspace_setting(_Broken()) == ToolsConfig().local_working_dir + + +@pytest.mark.parametrize("blank", ["", " ", "\t\n"]) +def test_blank_workspace_normalizes_to_the_default_at_the_boundary(blank: str) -> None: + """The field accepts free strings and can be blanked through PUT /api/config. + + Round-7 regression. Left un-normalized, a blank value made the self-update + preflight validate the DEFAULT and approve, while the restarted process + loaded the blank value and failed closed on every local command. Normalizing + here means every consumer — preflight, startup migration, executor — reads + the identical path. + """ + assert ToolsConfig(local_working_dir=blank).local_working_dir == "/var/lib/odin-workspace" + + +def test_configured_workspace_is_stripped_not_reinterpreted() -> None: + """Normalization must not silently relocate a real configured path.""" + assert ToolsConfig(local_working_dir=" /srv/ws ").local_working_dir == "/srv/ws" + + +def test_preflight_validates_the_exact_live_value_never_a_substitute() -> None: + """A present-but-blank live value must be REFUSED, not replaced. + + The schema normalizes blank away, so reaching the preflight with one means + the live config is not a validated ToolsConfig — precisely when substituting + a plausible default is least safe, because the restarted process will use + the real value and fail closed (PR #239 round-7 review, reproduced). + """ + from src.web.api.self_update import _ensure_local_workspace_for_update + + bot = SimpleNamespace(config=SimpleNamespace(tools=SimpleNamespace(local_working_dir=" "))) + error = _ensure_local_workspace_for_update(bot, None) + assert error is not None and "empty" in error + + +def test_preflight_uses_the_schema_default_only_when_there_is_no_live_config( + tmp_path: Path, monkeypatch: pytest.MonkeyPatch +) -> None: + """No reachable bot is the ONE case where the default is the honest answer.""" + import src.web.api.self_update as su + from src.config.schema import ToolsConfig as _ToolsConfig + + class _Unreachable: + @property + def config(self): # noqa: ANN201 - deliberately raises + raise RuntimeError("bot not ready") + + assert su._live_workspace_setting(None) == _ToolsConfig().local_working_dir + assert su._live_workspace_setting(_Unreachable()) == _ToolsConfig().local_working_dir + + +# --- Round 6: the entrypoint and the protected-root contract ------------------ + +_ENTRYPOINT_DRIVER = r""" +import json, os, re, runpy, sys +from pathlib import Path + +repo, tmp, ws = sys.argv[1], Path(sys.argv[2]), Path(sys.argv[3]) + +# Env vars the shipped template substitutes; values are irrelevant, presence is not. +template = (Path(repo) / "config.yml").read_text() +for name in set(re.findall(r"\$\{([A-Za-z_][A-Za-z0-9_]*)\}", template)): + os.environ.setdefault(name, "test-value") + +import yaml +# The config lives in its own directory: since round 9 the ACTIVE config +# file's directory is protected, and production keeps config.yml inside the +# install root (already protected) with the workspace elsewhere. +(tmp / "etc").mkdir(exist_ok=True) +cfg = tmp / "etc" / "config.yml" +parsed = yaml.safe_load(template) +parsed.setdefault("tools", {})["local_working_dir"] = str(ws) +cfg.write_text(yaml.safe_dump(parsed)) + +os.chdir(tmp) +observed = {} + +class _StopStartup(Exception): + pass + +import src.discord.client as client_module + +class _RecordingBot: + def __init__(self, *a, **k): + # THE assertion point: by the time anything that can run a command + # exists, the workspace must already be there. + observed["workspace_at_bot_construction"] = ws.is_dir() + raise _StopStartup() + +client_module.OdinBot = _RecordingBot +sys.argv = ["src", str(cfg)] +try: + runpy.run_module("src", run_name="__main__") +except _StopStartup: + observed["reached_bot_construction"] = True +except BaseException as exc: + observed["error"] = f"{type(exc).__name__}: {exc}" + +print("RESULT " + json.dumps(observed)) +""" + + +def test_python_m_src_provisions_the_workspace_before_the_bot_exists( + tmp_path: Path, +) -> None: + """Executes the REAL entrypoint, as `python -m src` does. + + Round-6 regression. The startup migration sat above + ``_command_protected_roots``'s ``def``, so running the module as + ``__main__`` raised NameError mid-module; the migration's own nonfatal + handler swallowed it and the workspace was never created — silently + restoring the first-update bootstrap failure the migration exists to fix. + + A source-order comparison cannot see this: it is Python's execution order + that matters, so this test runs the module for real in a subprocess and + asserts the workspace exists at the moment the bot is constructed. + """ + repo_root = Path(__file__).resolve().parents[1] + workspace = tmp_path / "workspace" + + env = dict(os.environ) + env["PYTHONPATH"] = str(repo_root) + proc = subprocess.run( + [sys.executable, "-c", _ENTRYPOINT_DRIVER, str(repo_root), str(tmp_path), str(workspace)], + capture_output=True, + text=True, + cwd=str(tmp_path), + env=env, + timeout=180, + ) + result_lines = [ln for ln in proc.stdout.splitlines() if ln.startswith("RESULT ")] + assert result_lines, ( + f"driver produced no result.\nstdout:\n{proc.stdout}\nstderr:\n{proc.stderr}" + ) + observed = json.loads(result_lines[-1][len("RESULT ") :]) + + assert observed.get("error") is None, observed["error"] + assert observed.get("reached_bot_construction") is True, observed + assert observed.get("workspace_at_bot_construction") is True, ( + "the workspace did not exist when the bot was constructed — the startup " + f"migration did not run: {observed}" + ) + assert workspace.is_dir() + assert stat.S_IMODE(workspace.stat().st_mode) == 0o700 + + +def _relocated_data_layout(tmp_path: Path) -> dict[str, Path]: + """The packaged shape that defeated the per-caller root derivations. + + install/data is a SYMLINK to the live-data directory (as /opt/odin/data -> + /var/lib/odin is), audit and trajectories are configured somewhere else + entirely, and the proposed workspace sits beside live memory.json. + """ + install = tmp_path / "install" + live = tmp_path / "var-lib-odin" + elsewhere = tmp_path / "elsewhere" + for directory in (install, live, elsewhere): + directory.mkdir() + (live / "memory.json").write_text("{}") + (install / "data").symlink_to(live) + return { + "install": install, + "live": live, + "memory": live / "memory.json", + "audit": elsewhere / "audit.jsonl", + "trajectory": elsewhere / "trajectories", + "workspace": live / "workspace", + } + + +def test_executor_rejects_a_workspace_beside_relocated_live_memory( + tmp_path: Path, +) -> None: + """Executor arm of the shared-contract scenario.""" + layout = _relocated_data_layout(tmp_path) + executor = ToolExecutor( + ToolsConfig( + local_working_dir=str(layout["workspace"]), + audit_log_path=str(layout["audit"]), + trajectory_path=str(layout["trajectory"]), + ), + memory_path=str(layout["memory"]), + ) + with pytest.raises(WorkspaceError): + executor._ensure_local_workspace() + assert not layout["workspace"].exists() + + +def test_startup_migration_rejects_a_workspace_beside_relocated_live_memory( + tmp_path: Path, monkeypatch: pytest.MonkeyPatch +) -> None: + """Startup arm. Startup runs before wiring, so it protects the shared + default memory path — which, from the install directory, resolves through + the data symlink onto the real live-data root.""" + import src.__main__ as entrypoint + + layout = _relocated_data_layout(tmp_path) + monkeypatch.chdir(layout["install"]) + + config = SimpleNamespace( + tools=SimpleNamespace( + local_working_dir=str(layout["workspace"]), + audit_log_path=str(layout["audit"]), + trajectory_path=str(layout["trajectory"]), + ) + ) + roots = entrypoint._command_protected_roots(config) + assert str(layout["live"].resolve()) in roots + + with pytest.raises(WorkspaceError): + provision_workspace(str(layout["workspace"]), protected_roots=roots) + assert not layout["workspace"].exists() + + +def test_self_update_preflight_rejects_a_workspace_beside_relocated_live_memory( + tmp_path: Path, +) -> None: + """Updater arm — the one that previously said yes. + + It omitted live memory.json from its own root derivation, so with audit and + trajectory paths relocated it CREATED the workspace inside the live-data + directory, reported success, and handed over to an executor that refused + every local command (PR #239 round-6 review, reproduced). + """ + from src.web.api.self_update import _ensure_local_workspace_for_update + + layout = _relocated_data_layout(tmp_path) + bot = SimpleNamespace( + config=SimpleNamespace( + tools=SimpleNamespace( + local_working_dir=str(layout["workspace"]), + audit_log_path=str(layout["audit"]), + trajectory_path=str(layout["trajectory"]), + ) + ), + tool_executor=SimpleNamespace(_memory_path=Path(layout["memory"])), + ) + + error = _ensure_local_workspace_for_update(bot, str(layout["install"])) + assert error is not None + assert "overlap" in error + assert not layout["workspace"].exists(), "preflight created a directory inside live data" + + +def test_all_three_callers_share_one_protected_root_derivation( + tmp_path: Path, monkeypatch: pytest.MonkeyPatch +) -> None: + """The load-bearing property behind the three tests above: executor, + startup, and the updater must agree about the live-data root. When they + each derived their own, one accepted what another rejected.""" + import src.__main__ as entrypoint + from src.web.api.self_update import _live_protected_roots + + layout = _relocated_data_layout(tmp_path) + monkeypatch.chdir(layout["install"]) + live = str(layout["live"].resolve()) + + executor = ToolExecutor( + ToolsConfig( + local_working_dir=str(layout["workspace"]), + audit_log_path=str(layout["audit"]), + trajectory_path=str(layout["trajectory"]), + ), + memory_path=str(layout["memory"]), + ) + config = SimpleNamespace(tools=executor.config) + bot = SimpleNamespace( + config=config, tool_executor=SimpleNamespace(_memory_path=Path(layout["memory"])) + ) + + assert live in executor._protected_roots() + assert live in entrypoint._command_protected_roots(config) + assert live in _live_protected_roots(bot, str(layout["install"])) + + +# --- Round 8: the workspace must not leak into unrelated tools --------------- + + +async def test_git_ops_with_omitted_repo_keeps_process_cwd_semantics( + tmp_path: Path, workspace: Path, monkeypatch: pytest.MonkeyPatch +) -> None: + """Round-8 blocker 1, reproduced by Odin as `fatal: not a git repository`. + + git_ops documents an omitted ``repo`` as ``"."`` — which has always meant + the process cwd, i.e. Odin's own install repo. Applying the workspace + unconditionally in the shared _exec_command primitive silently repointed + that at a scratch directory and broke `git_ops status`. The same class hits + docker build ``"."``, compose's implicit project directory, and terraform + without ``working_dir``. + """ + repo = tmp_path / "a-real-repo" + repo.mkdir() + for cmd in (["git", "init", "-q"], ["git", "config", "user.email", "t@t"], + ["git", "config", "user.name", "t"]): + subprocess.run(cmd, cwd=repo, check=True, capture_output=True) + (repo / "tracked.txt").write_text("x", encoding="utf-8") + monkeypatch.chdir(repo) + + executor = _executor_with_workspace(workspace, tmp_path / "unrelated-install") + result = await executor.execute("git_ops", {"action": "status", "host": "localhost"}) + + assert result.ok, result.output + assert "not a git repository" not in str(result.output) + assert "tracked.txt" in str(result.output) + + +async def test_an_unusable_workspace_does_not_disable_unrelated_tools( + tmp_path: Path, monkeypatch: pytest.MonkeyPatch +) -> None: + """The sharp version of the same contract. + + A workspace that fails validation must take down raw user commands ONLY. + If it also took down git_ops/docker/terraform/kubectl, one bad directory + would cost most of Odin's capability — far beyond the accepted mechanism. + """ + repo = tmp_path / "repo" + repo.mkdir() + for cmd in (["git", "init", "-q"], ["git", "config", "user.email", "t@t"], + ["git", "config", "user.name", "t"]): + subprocess.run(cmd, cwd=repo, check=True, capture_output=True) + monkeypatch.chdir(repo) + + protected = tmp_path / "install" + protected.mkdir() + # Overlaps the protected root: unusable by construction. + executor = _executor_with_workspace(protected / "inside", protected) + + user_command = await executor.execute( + "run_command", {"command": "echo should-not-run", "host": "localhost"} + ) + assert not user_command.ok + assert "should-not-run" not in str(user_command.output) + + git_status = await executor.execute("git_ops", {"action": "status", "host": "localhost"}) + assert git_status.ok, "an unusable workspace must not disable unrelated tools" + + +# --- Round 8: protected roots come from the FULL live configuration --------- + + +def _config_with(**overrides): + """A real Config, so the derivation is exercised against production shape.""" + from src.config.schema import Config + + return Config(discord={"token": "test-token"}, **overrides) + + +def test_relocated_state_directory_is_protected(tmp_path: Path) -> None: + """Round-8 blocker 2, reproduced by Odin with sessions.persist_directory + EQUAL to the configured workspace and accepted by every caller.""" + from src.tools.workspace import command_protected_roots + + relocated = tmp_path / "relocated-sessions" + config = _config_with( + sessions={"persist_directory": str(relocated)}, + tools={"local_working_dir": str(relocated)}, + ) + roots = command_protected_roots(tmp_path / "install", config) + assert str(relocated.resolve()) in roots + + with pytest.raises(WorkspaceError, match="overlap"): + provision_workspace(str(relocated), protected_roots=roots) + assert not relocated.exists() + + +def test_relocated_state_file_protects_its_directory(tmp_path: Path) -> None: + """A relocated FILE protects the directory holding it — the workspace must + not sit beside live permissions/credential state either.""" + from src.tools.workspace import command_protected_roots + + live = tmp_path / "relocated-state" + live.mkdir() + config = _config_with(permissions={"overrides_path": str(live / "permissions.json")}) + roots = command_protected_roots(tmp_path / "install", config) + assert str(live.resolve()) in roots + + with pytest.raises(WorkspaceError, match="overlap"): + provision_workspace(str(live / "workspace"), protected_roots=roots) + assert not (live / "workspace").exists() + + +def test_overlap_with_relocated_state_is_rejected_in_both_directions( + tmp_path: Path, +) -> None: + """A workspace that CONTAINS live state is as unusable as one inside it.""" + from src.tools.workspace import command_protected_roots + + parent = tmp_path / "parent" + (parent / "live-context").mkdir(parents=True) + config = _config_with(context={"directory": str(parent / "live-context")}) + roots = command_protected_roots(tmp_path / "install", config) + + with pytest.raises(WorkspaceError, match="overlap"): + provision_workspace(str(parent), protected_roots=roots) + + +def test_every_declared_state_path_is_covered(tmp_path: Path) -> None: + """Each declared live-state path contributes a root, so relocating any one + of them cannot silently drop it from protection.""" + from src.tools.workspace import _DECLARED_STATE_PATHS, command_protected_roots + + relocations = { + "tools.audit_log_path": (tmp_path / "s-audit" / "audit.jsonl", tmp_path / "s-audit"), + "tools.trajectory_path": (tmp_path / "s-traj", tmp_path / "s-traj"), + "tools.ssh_key_path": (tmp_path / "s-ssh" / "id", tmp_path / "s-ssh"), + "tools.ssh_known_hosts_path": (tmp_path / "s-kh" / "known", tmp_path / "s-kh"), + "tools.ssh_pool.socket_dir": (tmp_path / "s-sock", tmp_path / "s-sock"), + "context.directory": (tmp_path / "s-ctx", tmp_path / "s-ctx"), + "sessions.persist_directory": (tmp_path / "s-sess", tmp_path / "s-sess"), + "logging.directory": (tmp_path / "s-log", tmp_path / "s-log"), + "usage.directory": (tmp_path / "s-usage", tmp_path / "s-usage"), + "search.search_db_path": (tmp_path / "s-search" / "db", tmp_path / "s-search"), + "permissions.overrides_path": (tmp_path / "s-perm" / "p.json", tmp_path / "s-perm"), + "openai_codex.credentials_path": (tmp_path / "s-codex" / "c.json", tmp_path / "s-codex"), + "attachments.temp_directory": (tmp_path / "s-att", tmp_path / "s-att"), + } + assert set(relocations) == {dotted for dotted, _ in _DECLARED_STATE_PATHS}, ( + "a declared state path has no relocation case — add one so protection " + "cannot be dropped silently" + ) + + overrides: dict[str, dict] = {} + for dotted, (value, _expected) in relocations.items(): + section, _, leaf = dotted.partition(".") + node = overrides.setdefault(section, {}) + while "." in leaf: # nested section, e.g. tools.ssh_pool.socket_dir + head, _, leaf = leaf.partition(".") + node = node.setdefault(head, {}) + node[leaf] = str(value) + + roots = command_protected_roots(tmp_path / "install", _config_with(**overrides)) + for dotted, (_value, expected) in relocations.items(): + assert str(expected.resolve()) in roots, f"{dotted} is not protected" + + +def test_reduced_derivation_is_a_subset_never_a_different_answer(tmp_path: Path) -> None: + """Callers holding only a ToolsConfig (the __new__ patch seam, unit tests) + must not disagree with the full derivation — they may only know less.""" + from src.config.schema import ToolsConfig + from src.tools.workspace import command_protected_roots + + tools = ToolsConfig( + audit_log_path=str(tmp_path / "a" / "audit.jsonl"), + trajectory_path=str(tmp_path / "t"), + ) + config = _config_with(tools=tools.model_dump()) + full = command_protected_roots(tmp_path / "install", config) + reduced = command_protected_roots(tmp_path / "install", tools=tools) + assert set(reduced) <= set(full) + + +def test_wiring_supplies_the_full_config_to_the_executor() -> None: + """The reduced derivation must never be what production runs on.""" + import inspect + + from src.discord import wiring + + source = inspect.getsource(wiring.build_services) + assert "app_config=config" in source, ( + "wiring must pass the full config to ToolExecutor, or production falls " + "back to the reduced protected-root derivation" + ) + + +# --- Round 9: the last arbitrary-command route, and the live config file ----- + + +async def test_skill_run_on_host_replays_the_incident_safely( + fake_install: Path, workspace: Path, ae2_jar: Path, monkeypatch: pytest.MonkeyPatch +) -> None: + """Round-9 blocker 1, reproduced by Odin: SkillContext.run_on_host is + arbitrary command execution exposed to user-created skills, and it was + still inheriting the process cwd — an alternate route straight back into + the wipe. Remote hosts are unaffected; the workspace applies only after + local-address resolution. + """ + from src.tools.skill_context import SkillContext + + monkeypatch.chdir(fake_install) # a regression can only destroy the fixture + executor = _executor_with_workspace(workspace, fake_install) + ctx = SkillContext.__new__(SkillContext) + ctx._executor = executor + + await ctx.run_on_host("localhost", "mkdir -p data && touch data/from-skill") + assert (workspace / "data" / "from-skill").exists() + + await ctx.run_on_host("localhost", "rm -rf data") + assert not (workspace / "data").exists(), "the skill's own cleanup must work" + assert (fake_install / "data" / "sentinel").read_text(encoding="utf-8") == "live odin state" + + +async def test_skill_run_on_host_fails_closed_on_an_invalid_workspace( + fake_install: Path, tmp_path: Path +) -> None: + """Same fail-closed contract as the other raw command routes.""" + from src.tools.skill_context import SkillContext + + executor = _executor_with_workspace(tmp_path / "no-parent" / "ws", fake_install) + ctx = SkillContext.__new__(SkillContext) + ctx._executor = executor + with pytest.raises(WorkspaceError): + await ctx.run_on_host("localhost", "echo should-not-run") + + +def test_active_config_file_directory_is_protected( + tmp_path: Path, monkeypatch: pytest.MonkeyPatch +) -> None: + """Round-9 blocker 2, reproduced by Odin with an alternate config whose + parent WAS the configured workspace. + + Odin accepts `python -m src /arbitrary/path/odin.yml`. That file is runtime + state, not a Config field, so the exhaustive-declaration test cannot cover + it — and a bare relative command could delete the file needed to restart. + """ + from src.config.schema import active_config_path, set_active_config_path + from src.tools.workspace import command_protected_roots + + live = tmp_path / "live-config-and-workspace" + live.mkdir() + config_file = live / "odin-custom.yml" + config_file.write_text("discord:\n token: fake\n", encoding="utf-8") + + previous = active_config_path() + set_active_config_path(config_file) + try: + roots = command_protected_roots(tmp_path / "install") + assert str(live.resolve()) in roots + + with pytest.raises(WorkspaceError, match="overlap"): + provision_workspace(str(live), protected_roots=roots) + # ...and in the other direction: a workspace CONTAINING the config file. + with pytest.raises(WorkspaceError, match="overlap"): + provision_workspace(str(tmp_path), protected_roots=roots) + finally: + set_active_config_path(previous) + + +def test_active_config_symlink_protects_the_target_directory( + tmp_path: Path, +) -> None: + """The complete file path is resolved before taking .parent, so an aliased + config cannot protect the alias directory instead of the real one.""" + from src.config.schema import active_config_path, set_active_config_path + from src.tools.workspace import command_protected_roots + + real_dir = tmp_path / "real-config-dir" + real_dir.mkdir() + real_file = real_dir / "odin.yml" + real_file.write_text("discord:\n token: fake\n", encoding="utf-8") + alias_dir = tmp_path / "aliases" + alias_dir.mkdir() + (alias_dir / "odin.yml").symlink_to(real_file) + + previous = active_config_path() + set_active_config_path(alias_dir / "odin.yml") + try: + roots = command_protected_roots(tmp_path / "install") + assert str(real_dir.resolve()) in roots + with pytest.raises(WorkspaceError, match="overlap"): + provision_workspace(str(real_dir / "ws"), protected_roots=roots) + finally: + set_active_config_path(previous) + + +def test_no_active_config_protects_nothing_extra(tmp_path: Path) -> None: + """A process that never loaded a config has nothing to protect, and must + not guess a path — guessing would reject legitimate workspaces.""" + from src.config.schema import active_config_path, set_active_config_path + from src.tools.workspace import command_protected_roots + + previous = active_config_path() + set_active_config_path(None) + try: + roots = command_protected_roots(tmp_path / "install") + assert roots == [str((tmp_path / "install").resolve()), str(Path("./data").resolve())] + finally: + set_active_config_path(previous) + + +def test_repeated_scrapes_inside_the_ttl_do_not_re_walk( + fake_install: Path, workspace: Path +) -> None: + """A scrape loop must not walk continuously. /metrics is unauthenticated, + and the workspace deliberately never prunes.""" + executor = _executor_with_workspace(workspace, fake_install) + (workspace / "one").write_text("x" * 10, encoding="utf-8") + + walks = 0 + real_walk = os.walk + + def counting_walk(*args, **kwargs): + nonlocal walks + walks += 1 + return real_walk(*args, **kwargs) + + with patch("src.tools.executor.os.walk", counting_walk): + _metrics_after_refresh(executor) + assert walks == 1 + for _ in range(5): + executor.get_workspace_metrics() + assert walks == 1, "scrapes inside the TTL must reuse the cached walk" + + + +# --- The complete set of local-execution routes, held closed ----------------- + +# Every module in src/ that spawns a local process, and why it does or does not +# use the command workspace. Odin's reviews found bypasses one at a time +# (round 9: SkillContext.run_on_host); this holds the whole surface closed, so +# a NEW spawn site has to be classified rather than silently inheriting the +# process cwd. +_SPAWN_PRIMITIVES = { + "create_subprocess_shell", + "create_subprocess_exec", + "run", # subprocess.run + "Popen", + "system", # os.system + # Process REPLACEMENT counts too: it is how the self-update restarts, and + # it carries the cwd forward into the new image. + "execv", + "execve", + "execvp", + "execvpe", + "execl", + "execle", + "execlp", +} + +_CLASSIFIED_SPAWN_SITES: dict[str, str] = { + # --- uses the workspace ------------------------------------------------- + "src/tools/ssh.py": "run_local_command — takes cwd from the caller; THE seam", + "src/tools/process_manager.py": "background manage_process — resolves workspace per spawn", + # --- deliberately does not ---------------------------------------------- + "src/discord/native_tools/media.py": "argv-form ssh to a REMOTE host; local cwd is irrelevant", + "src/tools/ssh_pool.py": "argv-form ssh control-socket management, no user command text", + "src/tools/mcp_client.py": "operator-configured MCP server process, not a user command", + "src/tools/skill_manager.py": "argv-form `pip install `, no user-supplied relative path", + "src/tools/workspace.py": "`sudo -n install -d` provisioning the workspace itself", + "src/web/api/self_update.py": "argv-form git/gh during self-update, inside the install", + "src/packaging/validate.py": "build-time packaging check, not a runtime path", + "src/restart.py": "os.execve re-exec of Odin himself", + # --- legacy CLI surface, unreachable from Discord (pinned separately) ---- + "src/odin/tools/shell.py": "legacy CLI ShellTool; excluded from the Discord tool loop", + "src/odin/tools/process.py": "legacy CLI ProcessTool; excluded from the Discord tool loop", +} + + +def _modules_that_spawn_processes() -> set[str]: + """Every src/ module calling a process-spawning primitive, by AST. + + AST rather than grep so comments, docstrings and this test's own tables + cannot register as spawn sites. + """ + import ast + + repo = Path(__file__).resolve().parents[1] + found: set[str] = set() + for path in sorted((repo / "src").rglob("*.py")): + try: + tree = ast.parse(path.read_text(encoding="utf-8")) + except SyntaxError: # pragma: no cover - src must always parse + continue + for node in ast.walk(tree): + if not isinstance(node, ast.Call): + continue + func = node.func + if not isinstance(func, ast.Attribute) or func.attr not in _SPAWN_PRIMITIVES: + continue + # `subprocess.run` / `os.system` / `asyncio.create_subprocess_*` + # only — not arbitrary `.run(...)` methods on unrelated objects. + owner = func.value + owner_name = getattr(owner, "id", None) or getattr(owner, "attr", None) + if func.attr in {"run", "Popen"} and owner_name != "subprocess": + continue + if func.attr == "system" and owner_name != "os": + continue + if func.attr.startswith("exec") and owner_name != "os": + continue + found.add(str(path.relative_to(repo))) + return found + + +def test_every_local_execution_route_is_classified() -> None: + """No unclassified way to run a local process may exist in src/. + + A new spawn site added later inherits Odin's process cwd by default — + which is exactly the 2026-07-27 mechanism. This fails until it is either + routed through the workspace or explicitly justified here. + """ + actual = _modules_that_spawn_processes() + declared = set(_CLASSIFIED_SPAWN_SITES) + + unclassified = actual - declared + assert not unclassified, ( + "these modules spawn local processes but are not classified: " + f"{sorted(unclassified)} — route them through the workspace or record why not" + ) + + stale = declared - actual + assert not stale, f"no longer spawn processes; drop from the table: {sorted(stale)}" + + +def test_wiring_hardcoded_state_paths_are_all_covered(tmp_path: Path) -> None: + """The third leg of the protected-root surface. + + Config-declared paths are held by the exhaustive table, and the active + config file is handled as runtime state — but wiring also hardcodes a set + of live-state paths (learned, channel config and logs, host access, skills, + schedules, audit, API tokens). They are covered today because they sit + beside memory.json under ./data, whose parent IS protected. This asserts + that rather than assuming it, so relocating one out of ./data without + declaring it fails here instead of silently losing protection. + """ + import ast + + from src.tools.workspace import command_protected_roots + + repo = Path(__file__).resolve().parents[1] + tree = ast.parse((repo / "src" / "discord" / "wiring.py").read_text(encoding="utf-8")) + hardcoded = { + node.value + for node in ast.walk(tree) + if isinstance(node, ast.Constant) + and isinstance(node.value, str) + and node.value.startswith("./data/") + } + assert len(hardcoded) >= 8, f"expected wiring's data paths, found {sorted(hardcoded)}" + + roots = [Path(r) for r in command_protected_roots(tmp_path / "install")] + for raw in sorted(hardcoded): + resolved = Path(raw).expanduser().resolve() + covered = any(resolved == root or root in resolved.parents for root in roots) + assert covered, ( + f"{raw} is live state that no protected root covers — declare it in " + "_DECLARED_STATE_PATHS or keep it under the protected data directory" + ) + + +# --- Every _exec_command / _run_on_host CALLER classified -------------------- + +# The route-classification test above covers where processes are SPAWNED. This +# covers who ASKS for one, which is the axis Odin's round-10 review found twice +# (validate_action and write_file both reached the install cwd through the +# shared helpers). A call site is either a raw user-command route that opts +# into the workspace, or it must say why it does not. +_CLASSIFIED_COMMAND_CALLERS: dict[tuple[str, str], str] = { + # --- opt in: arbitrary user-supplied command text ----------------------- + ("src/tools/handlers/system.py", "_handle_run_command"): "WORKSPACE", + ("src/tools/handlers/system.py", "_handle_run_script"): "WORKSPACE", + ("src/tools/handlers/system.py", "_run_one"): "WORKSPACE", # run_command_multi + # CONDITIONAL: forwards run_bundle's per-check decision — command + # checks opt in, fixed-shape probes keep process cwd (round 11). + ("src/tools/handlers/validation.py", "_exec"): "CONDITIONAL", + ("src/tools/skill_context.py", "run_on_host"): "WORKSPACE", + # --- do not: fixed command shapes with caller-supplied absolute paths ---- + ("src/tools/handlers/devops.py", "_handle_git_ops"): "documented repo default is the cwd", + ("src/tools/handlers/devops.py", "_handle_kubectl"): "fixed kubectl argv", + ("src/tools/handlers/devops.py", "_handle_docker_ops"): "docker build context is caller-given", + ("src/tools/handlers/devops.py", "_handle_terraform_ops"): "terraform working_dir is explicit", + ("src/tools/handlers/coding.py", "_handle_claude_code"): "claude_code has its own cwd config", + ("src/tools/handlers/browser_web.py", "_handle_http_probe"): "fixed curl argv", + ("src/tools/handlers/files_docs.py", "_handle_read_file"): "reads a caller-given path", + ("src/tools/handlers/files_docs.py", "_handle_write_file"): "absolute path enforced", + ("src/tools/handlers/files_docs.py", "_handle_analyze_pdf"): "reads a caller-given path", + ("src/discord/native_tools/media.py", "_handle_analyze_image"): "base64 of a given path", + ("src/audit/diff_tracker.py", "capture_before"): "cat of a governed absolute path", + # --- plumbing ----------------------------------------------------------- + ("src/tools/executor.py", "_run_on_host"): "CONDITIONAL", # forwards its caller's decision + ("src/tools/executor.py", "__init__"): "HandlerDeps lambdas forwarding **kwargs", +} + + +def _command_call_sites() -> dict[tuple[str, str], list[str]]: + """(file, enclosing function) -> category of EVERY call, in order. + + Per call, not per function: an OR over a function's calls let an opted-in + function silently gain a second, unopted call (PR #239 round-11 review). + Categories: "true" (constant opt-in), "dynamic" (forwards a decision made + upstream), "none" (no opt-in). + """ + import ast + + repo = Path(__file__).resolve().parents[1] + sites: dict[tuple[str, str], bool] = {} + for path in sorted((repo / "src").rglob("*.py")): + try: + tree = ast.parse(path.read_text(encoding="utf-8")) + except SyntaxError: # pragma: no cover + continue + parents: dict[ast.AST, ast.AST] = {} + for node in ast.walk(tree): + for child in ast.iter_child_nodes(node): + parents[child] = node + for node in ast.walk(tree): + if not isinstance(node, ast.Call): + continue + func = node.func + if not isinstance(func, ast.Attribute): + continue + if func.attr not in {"_exec_command", "_run_on_host"}: + continue + enclosing = "" + walker: ast.AST | None = node + while walker is not None: + if isinstance(walker, (ast.FunctionDef, ast.AsyncFunctionDef)): + enclosing = walker.name + break + walker = parents.get(walker) + category = "none" + for kw in node.keywords: + if kw.arg != "use_workspace": + continue + if isinstance(kw.value, ast.Constant): + category = "true" if kw.value.value is True else "none" + else: + # A forwarded variable/attribute: the decision is made + # upstream (run_bundle's per-check flag). + category = "dynamic" + key = (str(path.relative_to(repo)), enclosing) + sites.setdefault(key, []).append(category) + return sites + + +def test_every_command_caller_is_classified() -> None: + """Round-10 regression, both blockers of that shape at once. + + validate_action and write_file each reached Odin's install directory + through these shared helpers because nobody had decided what they were. + Adding a new call site now fails until it is classified, and flipping an + existing one's opt-in fails too. + """ + actual = _command_call_sites() + declared = set(_CLASSIFIED_COMMAND_CALLERS) + + unclassified = set(actual) - declared + assert not unclassified, ( + f"unclassified command call sites: {sorted(unclassified)} — decide whether " + "each is a raw user-command route (use_workspace=True) or record why not" + ) + stale = declared - set(actual) + assert not stale, f"no longer call the helpers; drop from the table: {sorted(stale)}" + + for key, expected in _CLASSIFIED_COMMAND_CALLERS.items(): + calls = actual[key] + if expected == "WORKSPACE": + assert calls and all(c == "true" for c in calls), ( + f"{key[0]}::{key[1]}: every call must opt in, got {calls} — a new " + "call in an opted-in function must not ride its neighbours' opt-in" + ) + elif expected == "CONDITIONAL": + assert calls and all(c == "dynamic" for c in calls), ( + f"{key[0]}::{key[1]}: every call must forward the upstream " + f"decision, got {calls}" + ) + else: + assert all(c == "none" for c in calls), ( + f"{key[0]}::{key[1]} is classified as NOT a command route " + f"({expected}) but a call passes use_workspace: {calls}" + ) + + +async def test_validate_action_command_check_runs_in_the_workspace( + fake_install: Path, workspace: Path, monkeypatch: pytest.MonkeyPatch +) -> None: + """Round-10 blocker 1, reproduced by Odin through the real dispatch: a + command check is user-supplied command text, so it can replay the same + relative-path mechanism.""" + monkeypatch.chdir(fake_install) + executor = _executor_with_workspace(workspace, fake_install) + + await executor.execute("validate_action", { + "host": "localhost", + "checks": [{"type": "command", "target": "touch from-validate"}], + }) + assert (workspace / "from-validate").exists(), "the check ran in the workspace" + assert not (fake_install / "from-validate").exists(), "and NOT in the install" + + +async def test_validate_action_fails_closed_on_an_invalid_workspace( + fake_install: Path, tmp_path: Path, monkeypatch: pytest.MonkeyPatch +) -> None: + """Same fail-closed contract as every other raw command route.""" + monkeypatch.chdir(fake_install) + executor = _executor_with_workspace(tmp_path / "no-parent" / "ws", fake_install) + + result = await executor.execute("validate_action", { + "host": "localhost", + "checks": [{"type": "command", "target": "touch should-not-run"}], + }) + assert not (fake_install / "should-not-run").exists() + assert not (workspace_leaked := (tmp_path / "should-not-run")).exists(), workspace_leaked + assert result is not None + + +@pytest.mark.parametrize("relative", ["notes.md", "./notes.md", "data/notes.md"]) +async def test_write_file_rejects_relative_paths( + fake_install: Path, workspace: Path, monkeypatch: pytest.MonkeyPatch, relative: str +) -> None: + """Round-10 blocker 4, reproduced by Odin: the schema documents an absolute + path but nothing enforced it, so a relative one wrote into the install.""" + monkeypatch.chdir(fake_install) + executor = _executor_with_workspace(workspace, fake_install) + + result = await executor.execute("write_file", { + "host": "localhost", "path": relative, "content": "x", + }) + assert not result.ok + assert "absolute path" in str(result.output) + assert not (fake_install / relative).exists(), "nothing may be written to the install" + + +async def test_write_file_still_accepts_absolute_paths( + fake_install: Path, + workspace: Path, + tmp_path: Path, + monkeypatch: pytest.MonkeyPatch, +) -> None: + """The documented capability is untouched, including spaces in the path. + + The old ``mkdir -p $(dirname )`` still word-split dirname's + output. An absolute ``.../intended parent/file`` therefore created a + relative ``parent`` directory in the inherited install cwd and then failed + the intended write — another fixed-shape route touching the install. + """ + monkeypatch.chdir(fake_install) + executor = _executor_with_workspace(workspace, fake_install) + target = tmp_path / "intended parent" / "written file.txt" + result = await executor.execute("write_file", { + "host": "localhost", "path": str(target), "content": "hello", + }) + assert result.ok, result.output + assert target.read_text(encoding="utf-8") == "hello" + assert not (fake_install / "parent").exists(), ( + "an absolute write must not create word-split relative directories in the install" + ) + + +def test_aliased_config_protects_both_the_alias_and_the_target(tmp_path: Path) -> None: + """Round-10 blocker 2, reproduced by Odin. + + set_active_config_path canonicalizes, but restart.reexec replays sys.argv — + so the launch path is what the next process opens. Deleting the alias + breaks the restart even though the target survives. + """ + from src.config.schema import active_config_path, set_active_config_path + from src.tools.workspace import command_protected_roots + + real_dir = tmp_path / "real" + real_dir.mkdir() + real_file = real_dir / "odin.yml" + real_file.write_text("discord:\n token: fake\n", encoding="utf-8") + alias_dir = tmp_path / "alias" + alias_dir.mkdir() + (alias_dir / "odin.yml").symlink_to(real_file) + + previous = active_config_path() + set_active_config_path(alias_dir / "odin.yml") + try: + roots = command_protected_roots(tmp_path / "install") + assert str(real_dir.resolve()) in roots, "canonical target" + assert str(alias_dir.resolve()) in roots, "launch path re-exec will reopen" + with pytest.raises(WorkspaceError, match="overlap"): + provision_workspace(str(alias_dir), protected_roots=roots) + finally: + set_active_config_path(previous) + + +def test_workspace_usage_is_never_walked_on_the_calling_thread( + fake_install: Path, workspace: Path +) -> None: + """Round-10 metrics correction: /metrics is served on the event loop, so + the walk must happen off-thread, and free space must stay live even when + usage is served from cache.""" + executor = _executor_with_workspace(workspace, fake_install) + (workspace / "one").write_text("x" * 10, encoding="utf-8") + + calling_thread = threading.get_ident() + walk_threads: list[int] = [] + real_walk = os.walk + + def recording_walk(*args, **kwargs): + walk_threads.append(threading.get_ident()) + return real_walk(*args, **kwargs) + + with patch("src.tools.executor.os.walk", recording_walk): + first = executor.get_workspace_metrics() + # Free space is live from the very first scrape, with no walk yet. + assert "free_bytes" in first and "free_inodes" in first + deadline = time.monotonic() + 5 + while executor._workspace_usage_cache is None and time.monotonic() < deadline: + time.sleep(0.02) + + assert walk_threads, "the refresh never ran" + assert calling_thread not in walk_threads, "the walk must not block the caller" + + second = executor.get_workspace_metrics() + assert second["files"] == 1 and second["bytes"] >= 10 + assert "free_bytes" in second and "free_inodes" in second, "cheap metrics stay live" + + +def test_workspace_usage_timestamp_is_recorded_on_completion( + fake_install: Path, workspace: Path +) -> None: + """Stamping at the START would make any walk longer than the TTL stale the + instant it finished, so every scrape would launch another one.""" + executor = _executor_with_workspace(workspace, fake_install) + started = time.monotonic() + + def slow_walk(*args, **kwargs): + time.sleep(0.3) + return iter(()) + + with patch("src.tools.executor.os.walk", slow_walk): + executor.get_workspace_metrics() + deadline = time.monotonic() + 5 + while executor._workspace_usage_cache is None and time.monotonic() < deadline: + time.sleep(0.02) + + stamped_at = executor._workspace_usage_cache[0] + assert stamped_at >= started + 0.3, "the stamp must be taken after the walk finished" + + +def test_usage_refresh_is_single_flight(fake_install: Path, workspace: Path) -> None: + """A slow walk must not have a second one piled on top by the next scrape.""" + executor = _executor_with_workspace(workspace, fake_install) + executor._workspace_usage_refreshing = True # a walk is already in flight + + started = [] + def _record(**kw): + started.append(kw) + return _NoThread() + + with patch("src.tools.executor.threading.Thread", _record): + executor.get_workspace_metrics() + assert not started, "a refresh was already running; a second must not start" + + +def test_waiting_usage_refresh_rechecks_a_newly_published_cache( + fake_install: Path, workspace: Path +) -> None: + """The unlocked TTL check is only a fast path. If this caller waits for + the lock while another refresh publishes, it must re-check freshness under + the lock instead of launching an immediate duplicate walk.""" + executor = _executor_with_workspace(workspace, fake_install) + executor._workspace_usage_cache = (0.0, 1.0, 1.0) # stale at first check + real_thread = threading.Thread + attempted = threading.Event() + inner = threading.Lock() + started: list[dict] = [] + + class _GateLock: + def __enter__(self): + attempted.set() + inner.acquire() + return self + + def __exit__(self, *_exc): + inner.release() + + class _RecordedThread: + def __init__(self, **kwargs): + started.append(kwargs) + + def start(self) -> None: + return None + + executor._workspace_usage_lock = _GateLock() + inner.acquire() + caller = real_thread(target=executor._refresh_workspace_usage, args=(workspace,)) + caller.start() + try: + assert attempted.wait(5), "refresh caller never reached the locked re-check" + # Simulate the refresh that completed while this caller waited. + executor._workspace_usage_cache = (time.monotonic(), 2.0, 2.0) + executor._workspace_usage_refreshing = False + with patch("src.tools.executor.threading.Thread", _RecordedThread): + inner.release() + caller.join(5) + finally: + # Never strand the test thread on a failing assertion. + if inner.locked(): + inner.release() + caller.join(5) + + assert not caller.is_alive() + assert started == [], "a newly fresh cache must suppress the duplicate walk" + + +def test_usage_cache_is_published_before_single_flight_clears( + fake_install: Path, workspace: Path +) -> None: + """No scrape may observe refreshing=False while the completed cache is + still absent/stale, or it can launch a duplicate usage walk.""" + executor = _executor_with_workspace(workspace, fake_install) + (workspace / "one").write_text("x", encoding="utf-8") + inner = threading.Lock() + invalid_transitions: list[str] = [] + + class _ObservingLock: + def __enter__(self): + inner.acquire() + return self + + def __exit__(self, *_exc): + if ( + executor._workspace_usage_refreshing is False + and executor._workspace_usage_cache is None + ): + invalid_transitions.append("refresh cleared before cache publish") + inner.release() + + executor._workspace_usage_lock = _ObservingLock() + executor.get_workspace_metrics() + + deadline = time.monotonic() + 5 + while executor._workspace_usage_refreshing and time.monotonic() < deadline: + time.sleep(0.01) + + assert invalid_transitions == [] + assert executor._workspace_usage_refreshing is False + assert executor._workspace_usage_cache is not None + + +class _NoThread: + def start(self) -> None: # pragma: no cover - never reached when single-flight holds + raise AssertionError("thread should not have been started") + + +def test_usage_refresh_is_inert_without_executor_state(workspace: Path) -> None: + """The sanctioned __new__ patch seam builds executors without __init__, so + the lock may not exist. Metrics must degrade, never raise.""" + from src.tools.executor import ToolExecutor as _Executor + + bare = _Executor.__new__(_Executor) + bare._refresh_workspace_usage(workspace) # must not raise + assert getattr(bare, "_workspace_usage_cache", None) is None + + +def test_usage_refresh_survives_a_failing_walk(fake_install: Path, workspace: Path) -> None: + """An unreadable workspace leaves usage unreported and, critically, clears + the in-flight flag so later scrapes can still refresh.""" + executor = _executor_with_workspace(workspace, fake_install) + + def _boom(*_a, **_kw): + raise OSError("walk refused") + + with patch("src.tools.executor.os.walk", _boom): + executor.get_workspace_metrics() + deadline = time.monotonic() + 5 + while executor._workspace_usage_refreshing and time.monotonic() < deadline: + time.sleep(0.02) + + assert executor._workspace_usage_cache is None, "no usage numbers to report" + assert executor._workspace_usage_refreshing is False, "the flag must not stick" + + # ...and a later scrape still refreshes normally. + (workspace / "f").write_text("xyz", encoding="utf-8") + assert _metrics_after_refresh(executor)["files"] == 1 + + +def test_usage_refresh_skips_files_it_cannot_stat( + fake_install: Path, workspace: Path +) -> None: + """A file that vanishes mid-walk is counted but not sized, rather than + aborting the whole scan.""" + executor = _executor_with_workspace(workspace, fake_install) + (workspace / "gone").write_text("x" * 5, encoding="utf-8") + + def _refuse(*_a, **_kw): + raise OSError("stat refused") + + with patch("src.tools.executor.os.lstat", _refuse): + metrics = _metrics_after_refresh(executor) + + assert metrics["files"] == 1 + assert metrics["bytes"] == 0 + + +# --- The operator-visible surface: startup diagnostics and the template ------ + + +def test_startup_diagnostic_reports_an_unusable_workspace(tmp_path: Path) -> None: + """Local commands fail closed on a bad workspace, so the startup report has + to SAY so. Without this the symptom is every run_command failing with + nothing in the diagnostics to explain it. + + This gap was mine, found by sweeping the operator-visible surfaces rather + than by review — the same sweep that should have preceded the change. + """ + from src.config.schema import ToolsConfig + from src.health.startup import check_local_workspace + + result = check_local_workspace(ToolsConfig(local_working_dir="relative/nope")) + assert result.passed is False + assert result.name == "local_workspace" + assert "absolute" in result.detail + assert "install -d -m 0700" in result.recommendation, "must name the fix" + + +def test_startup_diagnostic_passes_for_a_usable_workspace( + tmp_path: Path, workspace: Path +) -> None: + from src.config.schema import ToolsConfig + from src.health.startup import check_local_workspace + + result = check_local_workspace(ToolsConfig(local_working_dir=str(workspace))) + assert result.passed is True + assert str(workspace.resolve()) in result.detail + + +def test_startup_diagnostic_uses_full_config_for_relocated_state(tmp_path: Path) -> None: + """The real diagnostic registry must not say ready when runtime rejects the + same workspace for overlapping an independently relocated state directory. + """ + from src.config.schema import Config + from src.health.startup import run_startup_diagnostics + + workspace = tmp_path / "state" + workspace.mkdir(mode=0o700) + config = Config( + discord={"token": "fake"}, + tools={"local_working_dir": str(workspace)}, + sessions={"persist_directory": str(workspace)}, + ) + report = run_startup_diagnostics(yaml_config=config) + result = next(item for item in report.results if item.name == "local_workspace") + assert result.passed is False + assert "overlap" in result.detail + + +def test_startup_diagnostic_uses_the_shared_protected_roots() -> None: + """A diagnostic applying different rules than the executor would be worse + than no diagnostic: it would pass a workspace the runtime then refuses.""" + from src.health.startup import _workspace_protected_roots + from src.tools.workspace import command_protected_roots + + assert _workspace_protected_roots() == command_protected_roots( + Path(__file__).resolve().parents[1] + ) + + +def test_startup_report_includes_the_workspace_check() -> None: + """Registered in the real runner, not merely defined — an unregistered + check reports nothing, and this is the surface an operator reads when + local commands have stopped working.""" + from src.config.schema import Config + from src.health.startup import run_startup_diagnostics + + report = run_startup_diagnostics(yaml_config=Config(discord={"token": "fake"})) + names = [r.name for r in report.results] + assert "local_workspace" in names, f"check not run; got {names}" + + + +def test_tracked_config_template_documents_the_workspace() -> None: + """CONTRIBUTING is binding: config examples stay aligned with reality. An + undocumented knob is undiscoverable precisely when an operator needs it — + when fail-closed has taken local commands away.""" + repo = Path(__file__).resolve().parents[1] + template = (repo / "config.yml").read_text(encoding="utf-8") + assert "local_working_dir:" in template + + import re as _re + + import yaml + for name in set(_re.findall(r"\$\{([A-Za-z_][A-Za-z0-9_]*)\}", template)): + os.environ.setdefault(name, "test-value") + parsed = yaml.safe_load(template) + # Against the module CONSTANT, not ToolsConfig() — conftest patches the + # field default to a temp directory for the whole test session. + from src.config.schema import DEFAULT_LOCAL_WORKING_DIR + + assert parsed["tools"]["local_working_dir"] == DEFAULT_LOCAL_WORKING_DIR, ( + "the template must not drift from the schema default" + ) + + +# --- Round 11: per-check-type workspace, and symlinked launch ancestors ------ + + +async def test_fixed_shape_validation_checks_survive_an_unusable_workspace( + fake_install: Path, tmp_path: Path, monkeypatch: pytest.MonkeyPatch +) -> None: + """Round-11 blocker 1, reproduced by Odin: opting the WHOLE validate_action + exec callback into the workspace made every check type depend on it — + an unusable workspace stopped service/process/http/port probes that are + fixed command shapes, not raw user commands. The narrowed scope is: + an invalid workspace disables raw user-command routes ONLY. + """ + monkeypatch.chdir(fake_install) + executor = _executor_with_workspace(tmp_path / "no-parent" / "ws", fake_install) + + result = await executor.execute("validate_action", { + "host": "localhost", + "checks": [ + # Fixed-shape probes: must EXECUTE despite the broken workspace. + {"type": "process", "target": "init", "severity": "warn"}, + {"type": "port", "target": "1", "severity": "warn"}, + # Raw user command text: must fail closed, visibly. + {"type": "command", "target": "touch should-not-run"}, + ], + }) + report = str(result.output) + assert "local_working_dir" in report, "the command check must fail closed, visibly" + # The probes produced real pass/fail verdicts rather than workspace errors: + # exactly one check errored (the command one). + assert report.count("local_working_dir") == 1, report + assert not (fake_install / "should-not-run").exists() + + +async def test_command_checks_still_run_in_the_workspace_when_it_is_valid( + fake_install: Path, workspace: Path, monkeypatch: pytest.MonkeyPatch +) -> None: + """The other side of the round-11 narrowing: conditional must not mean + never — type=command checks keep the round-10 behaviour.""" + monkeypatch.chdir(fake_install) + executor = _executor_with_workspace(workspace, fake_install) + await executor.execute("validate_action", { + "host": "localhost", + "checks": [{"type": "command", "target": "touch from-conditional"}], + }) + assert (workspace / "from-conditional").exists() + assert not (fake_install / "from-conditional").exists() + + +async def test_port_probe_target_cannot_execute_in_the_inherited_cwd( + fake_install: Path, workspace: Path, monkeypatch: pytest.MonkeyPatch +) -> None: + """Port is a fixed-shape probe only when its host cannot escape the nested + shell. Before the fix this target created the marker in the install cwd. + """ + monkeypatch.chdir(fake_install) + executor = _executor_with_workspace(workspace, fake_install) + await executor.execute("validate_action", { + "host": "localhost", + "checks": [{"type": "port", "target": "$(touch injected-by-port):1"}], + }) + assert not (fake_install / "injected-by-port").exists() + assert not (workspace / "injected-by-port").exists() + + +def test_symlinked_ancestor_of_the_launch_config_is_protected(tmp_path: Path) -> None: + """Round-11 blocker 2, reproduced by Odin. + + Round 10 protected a symlinked config FILE; a symlinked DIRECTORY earlier + in the launch path (ws/cfg -> real/) still canonicalized onto the target, + so a workspace at ws/ was accepted and a relative `rm -rf cfg` would + remove the path restart.reexec() replays while the canonical target + survived. The launch-side parent is now kept lexical. + """ + from src.config.schema import active_config_path, set_active_config_path + from src.tools.workspace import command_protected_roots + + real = tmp_path / "real" + real.mkdir() + (real / "odin.yml").write_text("discord:\n token: fake\n", encoding="utf-8") + ws = tmp_path / "ws" + ws.mkdir(mode=0o700) + (ws / "cfg").symlink_to(real, target_is_directory=True) + + previous = active_config_path() + set_active_config_path(ws / "cfg" / "odin.yml") + try: + roots = command_protected_roots(tmp_path / "install") + assert str(real.resolve()) in roots, "canonical target still protected" + assert str(ws / "cfg") in roots, "the LEXICAL launch parent must survive" + + # The workspace containing the alias component is rejected... + with pytest.raises(WorkspaceError, match="overlap"): + provision_workspace(str(ws), protected_roots=roots) + # ...and so is a workspace that IS the alias target reached lexically. + with pytest.raises(WorkspaceError, match="overlap"): + provision_workspace(str(ws / "cfg" / "sub"), protected_roots=roots) + assert not (real / "sub").exists() + finally: + set_active_config_path(previous) + + +def test_usage_refresh_flag_never_survives_the_thread( + fake_install: Path, workspace: Path +) -> None: + """Cross-review of Odin's round-12 single-flight fix. + + Publishing the cache and clearing the flag as one locked transition is + correct — clearing first lets a scrape launch a duplicate walk before the + fresh cache is visible. But dropping the `finally` alongside it meant any + exception that is NOT OSError left the flag set forever, and every later + scrape then declined to refresh: usage metrics frozen permanently, with no + recovery. Both properties are required, so both are pinned. + """ + executor = _executor_with_workspace(workspace, fake_install) + + def _unexpected(*_a, **_kw): + raise RuntimeError("not an OSError") + + # Observe exceptions leaving the worker directly rather than letting + # pytest reduce them to a non-failing PytestUnhandledThreadExceptionWarning. + # The exact ee0d68d CI run was green while emitting that warning, so checking + # only the recovered flag is not enough. + real_thread = threading.Thread + escaped: list[BaseException] = [] + finished = threading.Event() + + class _ObservedThread: + def __init__(self, *, target, **_kwargs): + self._target = target + + def start(self) -> None: + def _run() -> None: + try: + self._target() + except BaseException as exc: + escaped.append(exc) + finally: + finished.set() + + real_thread(target=_run, daemon=True).start() + + with ( + patch("src.tools.executor.os.walk", _unexpected), + patch("src.tools.executor.threading.Thread", _ObservedThread), + ): + executor.get_workspace_metrics() + assert finished.wait(5), "usage worker did not finish" + + assert escaped == [], "metrics worker exceptions must not escape the daemon thread" + assert executor._workspace_usage_refreshing is False, ( + "the single-flight flag must not survive the thread on ANY exception" + ) + + # ...and metrics recover on the next scrape rather than staying frozen. + (workspace / "f").write_text("xyz", encoding="utf-8") + assert _metrics_after_refresh(executor)["files"] == 1 + + +def test_usage_cache_and_flag_are_published_atomically( + fake_install: Path, workspace: Path +) -> None: + """Odin's finding: a scrape landing between 'flag cleared' and 'cache + published' starts a redundant walk. The fresh cache must be visible to any + observer that sees the flag cleared.""" + executor = _executor_with_workspace(workspace, fake_install) + (workspace / "one").write_text("x" * 10, encoding="utf-8") + + _metrics_after_refresh(executor) + # Whenever the flag is clear, a cache is present — never the gap. + assert executor._workspace_usage_refreshing is False + assert executor._workspace_usage_cache is not None + + +def _unprovisionable(tmp_path: Path) -> Path: + """A path that CANNOT be created under any identity. + + Deliberately not "a missing parent": provision_workspace falls back to + `sudo -n install -d`, which creates parent chains, so on a runner with + passwordless sudo (GitHub Actions) that premise silently evaporates and the + test asserts nothing. A parent that is a regular FILE defeats both mkdir + and install -d, as root or otherwise — verified under both. + """ + blocker = tmp_path / "blocker-file" + blocker.write_text("not a directory", encoding="utf-8") + return blocker / "ws" + + +def test_legacy_fallback_is_visible_not_silent( + tmp_path: Path, monkeypatch: pytest.MonkeyPatch +) -> None: + """Cross-review of Odin's round-13 legacy fallback. + + The fallback itself is right — losing every local command on a first + upgrade is worse than using a private HOME directory. But its stated safety + property does not hold: the packaged unit sets User= and no + Environment=HOME, so HOME comes from the account record and is typically + OUTSIDE the install (/home/odin on a real deployment). A broken PACKAGED + default therefore falls back rather than rejecting, and with only an + INFO log naming the path, that is indistinguishable from normal operation — + an operator would never learn their packaging is broken. + + So the fallback must announce itself: a callback for the caller to warn on, + and process state the startup diagnostic reports. + """ + import src.tools.workspace as workspace_module + from src.config.schema import ToolsConfig + from src.tools.workspace import provision_startup_workspace + + monkeypatch.setattr(workspace_module, "_STARTUP_FALLBACK", None) + home = tmp_path / "home" + home.mkdir() + monkeypatch.setenv("HOME", str(home)) + + legacy = ToolsConfig() + object.__setattr__(legacy, "local_working_dir", str(_unprovisionable(tmp_path))) + legacy.model_fields_set.discard("local_working_dir") + + intended = str(_unprovisionable(tmp_path)) + seen: list[tuple[Path, str, str]] = [] + workspace = provision_startup_workspace( + legacy, + protected_roots=[str(tmp_path / "install")], + on_fallback=lambda path, configured, reason: seen.append( + (path, configured, str(reason)) + ), + ) + + assert workspace == (home / ".odin-workspace").resolve() + assert seen, "the caller must be told a fallback happened" + assert seen[0][1] == intended + assert "could not be created" in seen[0][2], "the reason must be carried" + + recorded = workspace_module.startup_fallback() + assert recorded is not None and recorded[0] == str(workspace) + assert recorded[1] == intended + + +def test_startup_diagnostic_announces_a_fallback_workspace( + tmp_path: Path, monkeypatch: pytest.MonkeyPatch +) -> None: + """The report an operator actually reads must say FALLBACK, not just show + a path they would have to recognise as unusual.""" + import src.tools.workspace as workspace_module + from src.config.schema import ToolsConfig + from src.health.startup import check_local_workspace + + active = tmp_path / "fallback-ws" + active.mkdir(mode=0o700) + monkeypatch.setattr( + workspace_module, + "_STARTUP_FALLBACK", + (str(active.resolve()), "/var/lib/odin-workspace", "permission denied"), + ) + + result = check_local_workspace(ToolsConfig(local_working_dir=str(active))) + assert result.passed is True, "a usable fallback is not a failure" + assert "FALLBACK" in result.detail + assert "permission denied" in result.detail + assert result.metadata.get("fallback") is True + assert result.metadata.get("configured") == "/var/lib/odin-workspace" + assert "/var/lib/odin-workspace" in result.recommendation + assert str(active) not in result.recommendation, ( + "remediation must repair the failed default, not the working fallback" + ) + + +def test_explicit_workspace_is_never_replaced_by_the_fallback( + tmp_path: Path, monkeypatch: pytest.MonkeyPatch +) -> None: + """Odin's property, re-verified independently: an operator who set the + path gets a hard error, never a silent substitution.""" + from src.config.schema import ToolsConfig + from src.tools.workspace import provision_startup_workspace + + home = tmp_path / "home" + home.mkdir() + monkeypatch.setenv("HOME", str(home)) + + explicit = ToolsConfig(local_working_dir=str(_unprovisionable(tmp_path))) + with pytest.raises(WorkspaceError): + provision_startup_workspace(explicit, protected_roots=[str(tmp_path / "install")]) + assert not (home / ".odin-workspace").exists(), "nothing may be provisioned" diff --git a/tests/test_next_level_pipeline_integration.py b/tests/test_next_level_pipeline_integration.py index 6b6d5cc1..6493d3ba 100644 --- a/tests/test_next_level_pipeline_integration.py +++ b/tests/test_next_level_pipeline_integration.py @@ -15,7 +15,7 @@ class TestValidateActionEndToEnd: @pytest.mark.asyncio async def test_full_pipeline_mixed_severity(self): - async def fake_exec(addr, cmd, user, *, timeout): + async def fake_exec(addr, cmd, user, *, timeout, use_workspace=False): if "curl" in cmd: return (0, "200") if "systemctl is-active" in cmd: diff --git a/tests/test_output_streamer.py b/tests/test_output_streamer.py index 5d069c2c..5b487548 100644 --- a/tests/test_output_streamer.py +++ b/tests/test_output_streamer.py @@ -11,6 +11,18 @@ _ActiveStream, ) + +def _session_workspace() -> str: + """A valid local-command workspace for tests that stub config with a mock. + + ToolExecutor refuses to run a local command against an unvalidated cwd — + deliberately, since inheriting the install directory is what allowed the + 2026-07-27 wipe — so a mocked config still needs a real directory here. + """ + from src.config.schema import ToolsConfig + + return ToolsConfig().local_working_dir + # --------------------------------------------------------------------------- # StreamChunk # --------------------------------------------------------------------------- @@ -578,6 +590,8 @@ async def test_local_passes_on_output(self): executor = ToolExecutor.__new__(ToolExecutor) executor.config = MagicMock() + # Real path: the workspace is validated fail-closed (no cwd fallback). + executor.config.local_working_dir = _session_workspace() executor.config.command_timeout_seconds = 30 executor.bulkheads = MagicMock() executor.bulkheads.get.return_value = None @@ -601,6 +615,8 @@ async def test_ssh_passes_on_output(self): executor = ToolExecutor.__new__(ToolExecutor) executor.config = MagicMock() + # Real path: the workspace is validated fail-closed (no cwd fallback). + executor.config.local_working_dir = _session_workspace() executor.config.command_timeout_seconds = 30 executor.config.ssh_key_path = "/key" executor.config.ssh_known_hosts_path = "/known" @@ -627,6 +643,8 @@ async def test_no_on_output_default(self): executor = ToolExecutor.__new__(ToolExecutor) executor.config = MagicMock() + # Real path: the workspace is validated fail-closed (no cwd fallback). + executor.config.local_working_dir = _session_workspace() executor.config.command_timeout_seconds = 30 executor.bulkheads = MagicMock() executor.bulkheads.get.return_value = None @@ -654,6 +672,8 @@ async def test_streaming_enabled_creates_callback(self): executor = ToolExecutor() executor.config = MagicMock() + # Real path: the workspace is validated fail-closed (no cwd fallback). + executor.config.local_working_dir = _session_workspace() executor.config.command_timeout_seconds = 30 executor.config.hosts = {"myhost": MagicMock( address="127.0.0.1", @@ -692,6 +712,8 @@ async def test_streaming_disabled_no_callback(self): executor = ToolExecutor() executor.config = MagicMock() + # Real path: the workspace is validated fail-closed (no cwd fallback). + executor.config.local_working_dir = _session_workspace() executor.config.command_timeout_seconds = 30 executor.config.hosts = {"myhost": MagicMock( address="127.0.0.1", @@ -726,6 +748,8 @@ async def test_no_streamer_works(self): executor = ToolExecutor() executor.config = MagicMock() + # Real path: the workspace is validated fail-closed (no cwd fallback). + executor.config.local_working_dir = _session_workspace() executor.config.command_timeout_seconds = 30 executor.config.hosts = {"myhost": MagicMock( address="127.0.0.1", @@ -756,6 +780,8 @@ async def test_unknown_host_calls_finish(self): executor = ToolExecutor() executor.config = MagicMock() + # Real path: the workspace is validated fail-closed (no cwd fallback). + executor.config.local_working_dir = _session_workspace() executor.config.hosts = {} executor._branch_freshness_enabled = False executor._host_access = None @@ -784,6 +810,8 @@ async def test_streaming_enabled(self): executor = ToolExecutor() executor.config = MagicMock() + # Real path: the workspace is validated fail-closed (no cwd fallback). + executor.config.local_working_dir = _session_workspace() executor.config.command_timeout_seconds = 30 executor.config.hosts = {"myhost": MagicMock( address="127.0.0.1", @@ -821,6 +849,8 @@ async def test_streaming_disabled(self): executor = ToolExecutor() executor.config = MagicMock() + # Real path: the workspace is validated fail-closed (no cwd fallback). + executor.config.local_working_dir = _session_workspace() executor.config.command_timeout_seconds = 30 executor.config.hosts = {"myhost": MagicMock( address="127.0.0.1", diff --git a/tests/test_post_validation.py b/tests/test_post_validation.py index 66fe3061..cb5401af 100644 --- a/tests/test_post_validation.py +++ b/tests/test_post_validation.py @@ -3,6 +3,8 @@ import asyncio import json +import subprocess +from pathlib import Path import pytest @@ -118,14 +120,17 @@ def test_http(self): def test_port_with_host(self): cmd = _build_command(Check(type="port", target="1.2.3.4:8080")) - assert "/dev/tcp/1.2.3.4/8080" in cmd + assert '"/dev/tcp/$1/$2"' in cmd + assert cmd.endswith("_ 1.2.3.4 8080 && echo OPEN || echo CLOSED") def test_port_bare(self): cmd = _build_command(Check(type="port", target="5432")) - assert "/dev/tcp/127.0.0.1/5432" in cmd + assert '"/dev/tcp/$1/$2"' in cmd + assert cmd.endswith("_ 127.0.0.1 5432 && echo OPEN || echo CLOSED") def test_port_invalid(self): assert _build_command(Check(type="port", target="notaport")) is None + assert _build_command(Check(type="port", target=":80")) is None def test_service(self): cmd = _build_command(Check(type="service", target="nginx")) @@ -144,6 +149,17 @@ def test_log_absent_bare(self): def test_command_passthrough(self): assert _build_command(Check(type="command", target="echo hi")) == "echo hi" + def test_port_host_cannot_escape_the_constant_inner_script(self, tmp_path: Path): + """A port probe is fixed-shape only if its target cannot become shell + syntax. This payload executed in Odin's inherited cwd before the fix. + """ + marker = tmp_path / "port-target-executed" + payload = f"$(touch {marker}):1" + cmd = _build_command(Check(type="port", target=payload, timeout_seconds=1)) + assert cmd is not None + subprocess.run(cmd, shell=True, cwd=tmp_path, capture_output=True, timeout=5) + assert not marker.exists() + class TestEvaluate: def test_http_pass_default_status(self): @@ -357,7 +373,7 @@ def test_all_errored_is_error(self): class TestRunBundleIntegration: @pytest.mark.asyncio async def test_full_bundle_mixed_results(self): - async def fake_exec(addr, cmd, user, *, timeout): + async def fake_exec(addr, cmd, user, *, timeout, use_workspace=False): if "curl" in cmd: return (0, "200") if "dev/tcp" in cmd: @@ -437,7 +453,7 @@ async def slow_exec(*a, **kw): async def test_host_resolution_order(self): seen_hosts: list[str] = [] - async def fake_exec(addr, cmd, user, *, timeout): + async def fake_exec(addr, cmd, user, *, timeout, use_workspace=False): seen_hosts.append(addr) return (0, "active") @@ -480,7 +496,7 @@ async def test_concurrent_checks_respect_independent_timeouts(self): """Round 2 review — no shared-state timeout race across concurrent checks.""" observed: list[tuple[int, float]] = [] - async def timed_exec(addr, cmd, user, *, timeout): + async def timed_exec(addr, cmd, user, *, timeout, use_workspace=False): t0 = asyncio.get_event_loop().time() # Fast/slow checks finish at different times; neither should see # the other's timeout bleed in. @@ -529,7 +545,7 @@ async def test_max_parallel_bounds_concurrency(self): max_in_flight = 0 lock = asyncio.Lock() - async def tracking_exec(addr, cmd, user, *, timeout): + async def tracking_exec(addr, cmd, user, *, timeout, use_workspace=False): nonlocal in_flight, max_in_flight async with lock: in_flight += 1 @@ -609,7 +625,7 @@ def check(self, command): gov = _FailingGovernor() - async def wrapped(addr, cmd, user, *, timeout): + async def wrapped(addr, cmd, user, *, timeout, use_workspace=False): try: gov.check(cmd) except Exception as ge: diff --git a/tests/test_startup_diagnostics.py b/tests/test_startup_diagnostics.py index c6543915..eb5bb8af 100644 --- a/tests/test_startup_diagnostics.py +++ b/tests/test_startup_diagnostics.py @@ -618,6 +618,17 @@ def test_model_empty(self): # --------------------------------------------------------------------------- + +def _usable_workspace(): + """A 0700 directory owned by this user, outside the repo and data roots.""" + import tempfile + from pathlib import Path + + path = Path(tempfile.mkdtemp(prefix="odin-diag-ws-")) + path.chmod(0o700) + return path + + class TestRunStartupDiagnostics: def test_no_config(self): report = run_startup_diagnostics() @@ -644,6 +655,9 @@ def test_with_yaml_config(self, tmp_path, monkeypatch): yaml_cfg.openai_codex = MagicMock() yaml_cfg.openai_codex.enabled = False yaml_cfg.tools = MagicMock() + # A real path: local_workspace is a registered check now, and a + # MagicMock attribute is not a workspace (PR #239). + yaml_cfg.tools.local_working_dir = str(_usable_workspace()) yaml_cfg.tools.hosts = {} yaml_cfg.sessions = MagicMock() yaml_cfg.sessions.persist_directory = str(tmp_path / "sessions") @@ -669,6 +683,9 @@ def test_with_both_configs(self, tmp_path, monkeypatch): yaml_cfg.openai_codex = MagicMock() yaml_cfg.openai_codex.enabled = False yaml_cfg.tools = MagicMock() + # A real path: local_workspace is a registered check now, and a + # MagicMock attribute is not a workspace (PR #239). + yaml_cfg.tools.local_working_dir = str(_usable_workspace()) yaml_cfg.tools.hosts = {} yaml_cfg.sessions = MagicMock() yaml_cfg.sessions.persist_directory = "" @@ -705,6 +722,9 @@ def test_check_crash_handled(self, tmp_path, monkeypatch): lambda self: (_ for _ in ()).throw(RuntimeError("boom")) ) yaml_cfg.tools = MagicMock() + # A real path: local_workspace is a registered check now, and a + # MagicMock attribute is not a workspace (PR #239). + yaml_cfg.tools.local_working_dir = str(_usable_workspace()) yaml_cfg.tools.hosts = {} yaml_cfg.sessions = MagicMock() yaml_cfg.sessions.persist_directory = "" @@ -746,7 +766,7 @@ def test_unique_names(self): assert len(names) == len(set(names)), "Duplicate check names" def test_check_count(self): - assert len(_CONFIG_CHECKS) == 7 + assert len(_CONFIG_CHECKS) == 8 # --------------------------------------------------------------------------- @@ -841,6 +861,9 @@ def test_full_production_config(self, tmp_path, monkeypatch): yaml_cfg.openai_codex.credentials_path = str(cred_file) yaml_cfg.openai_codex.model = "gpt-4o" yaml_cfg.tools = MagicMock() + # A real path: local_workspace is a registered check now, and a + # MagicMock attribute is not a workspace (PR #239). + yaml_cfg.tools.local_working_dir = str(_usable_workspace()) yaml_cfg.tools.hosts = {"web1": MagicMock(), "db1": MagicMock()} yaml_cfg.tools.ssh_key_path = str(ssh_key) yaml_cfg.tools.ssh_known_hosts_path = str(known_hosts) @@ -871,6 +894,9 @@ def test_minimal_dev_config(self, tmp_path, monkeypatch): yaml_cfg.openai_codex = MagicMock() yaml_cfg.openai_codex.enabled = False yaml_cfg.tools = MagicMock() + # A real path: local_workspace is a registered check now, and a + # MagicMock attribute is not a workspace (PR #239). + yaml_cfg.tools.local_working_dir = str(_usable_workspace()) yaml_cfg.tools.hosts = {} yaml_cfg.sessions = MagicMock() yaml_cfg.sessions.persist_directory = "" @@ -898,6 +924,9 @@ def test_misconfigured_everything(self, tmp_path, monkeypatch): yaml_cfg.openai_codex.credentials_path = "/nonexistent/creds.json" yaml_cfg.openai_codex.model = "" yaml_cfg.tools = MagicMock() + # A real path: local_workspace is a registered check now, and a + # MagicMock attribute is not a workspace (PR #239). + yaml_cfg.tools.local_working_dir = str(_usable_workspace()) yaml_cfg.tools.hosts = {"web1": MagicMock()} yaml_cfg.tools.ssh_key_path = "/nonexistent/key" yaml_cfg.tools.ssh_known_hosts_path = "/nonexistent/known" diff --git a/tests/test_ts_ledger_fixes.py b/tests/test_ts_ledger_fixes.py index 4a03d5eb..cc97a9af 100644 --- a/tests/test_ts_ledger_fixes.py +++ b/tests/test_ts_ledger_fixes.py @@ -22,7 +22,10 @@ def __init__(self, raw): self._raw = raw self.config = MagicMock() - async def _run_on_host(self, alias, command): + async def _run_on_host(self, alias, command, use_workspace=False): + # Recorded, not asserted: skills opt IN (arbitrary command execution), + # the audit diff tracker deliberately does NOT (PR #239 round 9). + self.last_use_workspace = use_workspace if isinstance(self._raw, Exception): raise self._raw return self._raw @@ -68,6 +71,15 @@ def _ctx(self, raw): ctx._executor = _FakeExecutor(raw) return ctx + @pytest.mark.asyncio + async def test_skill_commands_opt_into_the_workspace(self): + """PR #239 round 9: this is arbitrary command execution exposed to + user-created skills, so it is a raw command route and must land in the + workspace — omitting it left an alternate path back into the wipe.""" + ctx = self._ctx(("ok", 0)) + await ctx.run_on_host("localhost", "rm -rf data") + assert ctx._executor.last_use_workspace is True + @pytest.mark.asyncio async def test_resolved_host_returns_output_string(self): result = await self._ctx(("uptime output", 0)).run_on_host("server", "uptime") diff --git a/tests/test_web_api_config_admin.py b/tests/test_web_api_config_admin.py index 2cfbb678..8d635c20 100644 --- a/tests/test_web_api_config_admin.py +++ b/tests/test_web_api_config_admin.py @@ -274,16 +274,103 @@ async def test_update_config_persists_normalized_dropping_removed_keys(self): from pathlib import Path from ruamel.yaml import YAML + + from src.config.schema import active_config_path, set_active_config_path + Path("config.yml").write_text("discord:\n token: fake\n") - app, bot = _app(register_discord_config) - async with TestClient(TestServer(app)) as c: - r = await c.put("/api/config", json={ - "openai_codex": {"model_routing": {"enabled": True}}, - }) - assert r.status == 200 + previous = active_config_path() + set_active_config_path(Path("config.yml")) + try: + app, bot = _app(register_discord_config) + async with TestClient(TestServer(app)) as c: + r = await c.put("/api/config", json={ + "openai_codex": {"model_routing": {"enabled": True}}, + }) + assert r.status == 200 + finally: + set_active_config_path(previous) oc = YAML().load(Path("config.yml").read_text()).get("openai_codex", {}) assert "model_routing" not in oc + @pytest.mark.asyncio + async def test_blanking_the_workspace_normalizes_everywhere(self): + """PR #239 round-8 follow-up: the persisted-config path, for real. + + tools.local_working_dir accepts free strings and can be blanked through + this endpoint. Blank must normalize to the default in the RESPONSE, in + the runtime config, on disk, and on a fresh reload — otherwise the + self-update preflight and the restarted process disagree about which + directory they are validating, which is how a blank value used to + approve an update that then failed closed on every local command. + """ + from pathlib import Path + + from ruamel.yaml import YAML + + from src.config.schema import Config as _Config + from src.config.schema import active_config_path, set_active_config_path + + Path("config.yml").write_text("discord:\n token: fake\n") + # Explicit: persistence targets the ACTIVE config path, and any earlier + # test that called load_config leaves that module global set. + previous = active_config_path() + set_active_config_path(Path("config.yml")) + try: + app, bot = _app(register_discord_config) + async with TestClient(TestServer(app)) as c: + r = await c.put("/api/config", json={"tools": {"local_working_dir": " "}}) + assert r.status == 200 + finally: + set_active_config_path(previous) + + default = "/var/lib/odin-workspace" + assert bot.config.tools.local_working_dir == default, "runtime config" + on_disk = YAML().load(Path("config.yml").read_text())["tools"]["local_working_dir"] + assert on_disk == default, "persisted YAML" + reloaded = _Config(**YAML().load(Path("config.yml").read_text())) + assert reloaded.tools.local_working_dir == default, "fresh reload" + + @pytest.mark.asyncio + async def test_writes_to_the_active_config_not_cwd_config_yml(self): + """PR #239 round-10 blocker 3: alternate-config deployments. + + Odin can be launched with `python -m src /somewhere/odin.yml`, and + restart.reexec replays that argument. Persisting to a cwd-relative + config.yml meant a change lived in bot.config, was validated by the + self-update preflight, and then vanished on re-exec — contradicting the + preflight's contract that it validates what the restarted process will + use. llm_admin already wrote to the active path; this one did not. + """ + from pathlib import Path + + from ruamel.yaml import YAML + + from src.config.schema import active_config_path, set_active_config_path + + alternate = Path("odin-alternate.yml") + alternate.write_text("discord:\n token: fake\n") + decoy = Path("config.yml") + decoy.write_text("discord:\n token: fake\n") + + previous = active_config_path() + set_active_config_path(alternate) + try: + app, bot = _app(register_discord_config) + async with TestClient(TestServer(app)) as c: + r = await c.put( + "/api/config", json={"tools": {"local_working_dir": "/srv/ws"}} + ) + assert r.status == 200 + finally: + set_active_config_path(previous) + + written = YAML().load(alternate.read_text())["tools"]["local_working_dir"] + assert written == "/srv/ws", "the ACTIVE config must receive the change" + assert bot.config.tools.local_working_dir == "/srv/ws", "runtime agrees" + assert "tools" not in (YAML().load(decoy.read_text()) or {}), ( + "the cwd config.yml is a decoy here and must not be written" + ) + @pytest.mark.asyncio async def test_health_and_resource_and_streams(self): app, bot = _app(register_discord_config) diff --git a/tests/test_web_api_self_update.py b/tests/test_web_api_self_update.py index b81a930c..e15d28c3 100644 --- a/tests/test_web_api_self_update.py +++ b/tests/test_web_api_self_update.py @@ -10,6 +10,7 @@ from __future__ import annotations from subprocess import CompletedProcess +from types import SimpleNamespace from unittest.mock import MagicMock, patch from aiohttp import web @@ -157,6 +158,69 @@ def _stub_call_later(delay, cb, *a): # main() re-execs in place after shutdown only because this was set assert restart.restart_requested() is True + async def test_refuses_when_the_workspace_cannot_be_provisioned(self): + """PR #239 round-4 blocker 1: this updater re-execs IN PLACE, so systemd + never starts the service and never applies StateDirectory=. If the local + command workspace cannot be provisioned, the update must be REFUSED — + otherwise it completes and Odin cannot run a single local command + afterwards (verified against the live install during review).""" + with patch( + "src.web.api.self_update._ensure_local_workspace_for_update", + return_value=( + "Create it before updating: sudo install -d -m 0700 " + "-o odin -g odin /var/lib/odin-workspace" + ), + ), patch("subprocess.run", side_effect=_run_ok) as run: + async with TestClient(TestServer(_app())) as c: + r = await c.post("/api/update/apply", json={"version": "latest"}) + assert r.status == 409 + body = await r.json() + assert "update refused" in body["error"] + assert "install -d -m 0700" in body["error"], "must stay actionable" + # Refusal happens BEFORE the repository is touched. + assert not any( + "merge" in " ".join(map(str, call.args[0])) + for call in run.call_args_list + if call.args and isinstance(call.args[0], list) + ), "the update must not have been committed" + + async def test_refuses_a_blank_live_workspace_before_touching_the_repo(self): + """PR #239 round-7 blocker: a blank LIVE workspace, end to end. + + The persistence half — that PUT /api/config normalizes blank away — is + pinned separately in test_web_api_config_admin; this drives the update + route against a live config that already holds a raw blank value. + + local_working_dir accepts free strings and can be blanked through + PUT /api/config. The preflight used to treat a present-but-blank value + as "nothing configured" and validate the DEFAULT instead, so the update + was approved while the restarted process loaded the blank value and + failed closed on every local command. + + Nothing is stubbed here except the exec primitives: this drives the real + preflight from a live bot config. + """ + bot = SimpleNamespace( + config=SimpleNamespace( + tools=SimpleNamespace( + local_working_dir=" ", + audit_log_path=None, + trajectory_path=None, + ) + ) + ) + with patch("subprocess.run", side_effect=_run_ok) as run: + async with TestClient(TestServer(_app(bot))) as c: + r = await c.post("/api/update/apply", json={"version": "latest"}) + assert r.status == 409 + assert "update refused" in (await r.json())["error"] + assert not any( + "merge" in " ".join(map(str, call.args[0])) + for call in run.call_args_list + if call.args and isinstance(call.args[0], list) + ), "the update must not have been committed" + assert restart.restart_requested() is False + async def test_unexpected_exception_500(self): # subprocess raising mid-flow (inside the try) surfaces as a 500 def _boom(cmd, **kw):