diff --git a/cli/parser.py b/cli/parser.py index fcaebb5..72d5c78 100644 --- a/cli/parser.py +++ b/cli/parser.py @@ -54,6 +54,24 @@ from .stt import cmd_stt_transcribe +def _positive_task_count(raw: str) -> int: + """`--max-tasks`, refused rather than defaulted when it is not a positive whole number. + + ⚠️ **Deliberately NOT `GRID_MAX_TASKS`'s rule, and the difference is the point.** That variable + falls back to 1 and says so, because refusing would take task serving down for the life of a + running process — a far worse answer than running with the default. A flag is a different + situation: the operator is at the terminal, they typed it a second ago, and being told costs + them one retry. `argparse` turns this into exit 2, which is "ask again" rather than "done". + """ + try: + count = int(raw) + except ValueError: + raise argparse.ArgumentTypeError(f"{raw!r} is not a whole number of tasks") from None + if count < 1: + raise argparse.ArgumentTypeError(f"{raw!r} must be at least 1 task") + return count + + def build_parser() -> argparse.ArgumentParser: parser = argparse.ArgumentParser( prog="grid", @@ -278,6 +296,19 @@ def _add_engines(sub) -> None: remote_only.add_argument("--respawn", action="store_true", default=None, help="Stop the engine already serving this grid and start a fresh one, " "instead of no-opping an identical re-join (remote only).") + # Task serving (ADR 0032). Remote-only for the same structural reason as `--max-concurrency`: + # a task is claimed from the relay, and local mode has no relay. `default=None` throughout, per + # the group's comment above — `--tasks` in particular, because a `store_true` defaulting to + # False would make `_reject_remote_only_flags` refuse every LOCAL join. + remote_only.add_argument("--tasks", action="store_true", default=None, + help="Also claim distributed tasks for this grid, spending this box's " + "own Claude subscription (remote only). Off unless you ask.") + remote_only.add_argument("--max-tasks", type=_positive_task_count, default=None, metavar="N", + help="How many tasks this provider runs at once (default 1). " + "Wins over GRID_MAX_TASKS.") + remote_only.add_argument("--tasks-root", default=None, metavar="PATH", + help="Where task workspaces live. Keep it SHORT and outside your home " + "directory. Wins over GRID_TASK_ROOT.") join.set_defaults(handler=cmd_join) leave = sub.add_parser("leave", help="Stop and unregister engines from a grid") diff --git a/cli/provider.py b/cli/provider.py index 31302d9..dcc750d 100644 --- a/cli/provider.py +++ b/cli/provider.py @@ -40,6 +40,13 @@ ("pricing_output", "--pricing-output"), ("max_concurrency", "--max-concurrency"), ("respawn", "--respawn"), + # Task serving (ADR 0032, issue 61). A task is claimed from the relay, and local mode has no + # relay — the same structural reason `--max-concurrency` is here. ⚠️ All three default to + # `None`, `--tasks` included: the predicate below is `is not None`, so a `store_true` flag + # defaulting to False would refuse every LOCAL join. + ("tasks", "--tasks"), + ("max_tasks", "--max-tasks"), + ("tasks_root", "--tasks-root"), ) diff --git a/cli/remote_provider.py b/cli/remote_provider.py index 685696b..a694612 100644 --- a/cli/remote_provider.py +++ b/cli/remote_provider.py @@ -14,6 +14,7 @@ from __future__ import annotations import argparse +import contextlib import getpass import os import signal @@ -152,7 +153,110 @@ def allowed(self) -> bool: return self.requested and self.problem is None -def _decide_task_serving() -> _TaskServing: +def _task_env_from_flags(args: argparse.Namespace) -> dict[str, str]: + """What `--tasks`/`--max-tasks`/`--tasks-root` change in the serve child's environment. + + The flags SET the environment the child is handed rather than moving the reading into the run + record, and that is deliberate — `task_opt_in.serving_enabled`'s docstring records why the + opt-in is read at serve time. This adds a second way to set it, not a second place to read it. + + **The flag wins over an exported variable**, which is the ordinary expectation and is said in + `--help` so nobody has to discover it. `--tasks` is the exception that proves nothing: there is + no `--no-tasks`, so it can only ever turn serving ON — and turning it on is the one thing that + must be a person typing it, never something inferred. + """ + from remote import task_agent, task_opt_in + + overrides: dict[str, str] = {} + if getattr(args, "tasks", None): + overrides[task_opt_in.SERVING_ENV] = "1" + count = getattr(args, "max_tasks", None) + if count is not None: + overrides[task_opt_in.WORKERS_ENV] = str(count) + root = getattr(args, "tasks_root", None) + if root is not None: + overrides[task_agent.WORKSPACE_ROOT_ENV] = str(root) + return overrides + + +@contextlib.contextmanager +def _as_the_child_will_see_it(overrides: dict[str, str]): + """Run a check under the environment the serve child is about to be handed. + + The checks read `os.environ` because that is what the CHILD reads, so asking them about a + `--tasks-root` this process was never started with means putting the value where they look. + A scoped mutation with a `finally`, rather than threading every variable through every check: + the alternative is a second way to express the provider's configuration, and a second way to + express it is a second way for the two to disagree. + """ + saved = {name: os.environ.get(name) for name in overrides} + os.environ.update(overrides) + try: + yield + finally: + for name, value in saved.items(): + if value is None: + os.environ.pop(name, None) + else: + os.environ[name] = value + + +def _task_state_of_the_child(overrides: dict[str, str] | None) -> dict[str, object]: + """What the child about to be spawned will decide about task serving, asked its own way. + + Recorded on the run record so a LATER join can tell whether an opt-in it was handed will take + effect (issue 60). Read through `task_opt_in` rather than by inspecting `overrides`, so this + cannot drift from what the child itself concludes — the environment is the wire between the two + processes, and asking any other way is a second reading of it. + """ + from remote import task_opt_in + + with _as_the_child_will_see_it(overrides or {}): + return {"tasks": task_opt_in.serving_enabled(), "max_tasks": task_opt_in.worker_count()} + + +def _task_serving_drift(live: list[dict[str, object]], desired: dict[str, object]) -> str | None: + """Why the task configuration this join was handed will not take effect, or `None` (issue 60). + + Two paths turn `GRID_TASKS=1 grid join …` into nothing at all. The **no-op gate** declares a + join idempotent when no engine, model, display name, bundle or media changed — and the opt-in is + none of those, while the running child's environment was fixed when it was spawned. The + **hot reload** re-reads the run record, which cannot change a running process's environment + either. From outside, both are indistinguishable from "there is no work yet". + + ⚠️ **Absent is UNKNOWN, never off.** Every record written before this issue carries no such key, + and reading a missing one as `False` would tell every provider already serving tasks that its + task serving is not on. Hence `isinstance(..., bool)` rather than `.get(...)` — and `bool` is + checked *before* `int` for the count, because `True` is an `int` and would otherwise read as one + worker. + + Nothing respawns on the strength of this. A respawn stops the child, and the in-flight inference + requests that would drop are the operator's call, not this function's. + """ + recorded = _identity_field(live, "tasks") + if not isinstance(recorded, bool): + return None + if recorded != desired["tasks"]: + return ( + f"Task serving is {'on' if recorded else 'off'} for the engine already serving this " + f"grid, and it is read once at startup — so this join cannot turn it " + f"{'off' if recorded else 'on'}. Apply it with `grid join --respawn`." + ) + if not desired["tasks"]: + return None # both off: the worker count decides nothing at all + running = _identity_field(live, "max_tasks") + if isinstance(running, bool) or not isinstance(running, int): + return None + if running != desired["max_tasks"]: + return ( + f"The engine already serving this grid runs {running} task(s) at once, not " + f"{desired['max_tasks']}; that count is read once at startup too. Apply it with " + f"`grid join --respawn`." + ) + return None + + +def _decide_task_serving(*, may_make_the_root: bool = False) -> _TaskServing: """Ask, in the parent, whether the child about to be spawned could actually run a task. Every one of these answers exists already — and until this ran, `run_task` was the only place @@ -181,6 +285,11 @@ def _decide_task_serving() -> _TaskServing: from remote import task_agent try: + # ⚠️ Only `--tasks` may CREATE a directory, and only the default one. An operator who named + # a root — by flag or by variable — named a path they own, and issue 62's stricter rules are + # for the default nobody chose (`ensure_default_workspace_root` says why). + if may_make_the_root and not (os.getenv(task_agent.WORKSPACE_ROOT_ENV) or "").strip(): + task_agent.ensure_default_workspace_root() task_agent.preflight_before_serving() except (Exception, SystemExit) as exc: # noqa: BLE001 — inference must survive any of them return _TaskServing(requested=True, problem=str(exc) or exc.__class__.__name__) @@ -269,7 +378,11 @@ def cmd_remote_join(args: argparse.Namespace) -> int: # a second ago must still be serving it after a task check that failed — so this may not run # from inside the block that has already terminated the prior child on its way to finding out. # It is also the last point where nothing has been written: a pure read, then the mutation. - task_serving = _decide_task_serving() + # The flags first, so every check below asks about the configuration the child will actually + # get rather than the one this shell happens to export (issue 61). + task_flags = _task_env_from_flags(args) + with _as_the_child_will_see_it(task_flags): + task_serving = _decide_task_serving(may_make_the_root=bool(getattr(args, "tasks", None))) if task_serving.problem: print( f"Task serving is off for this join: {task_serving.problem}\n" @@ -277,8 +390,13 @@ def cmd_remote_join(args: argparse.Namespace) -> int: file=sys.stderr, ) # Withheld from the CHILD, never from this process: the opt-in travels in the environment the - # parent hands over, and the child reads it once at startup. - engine_env = _task_serving_override(task_serving) + # parent hands over, and the child reads it once at startup. The withholding is applied LAST so + # it outranks `--tasks` — a provider that cannot run a task may not be told to claim one by a + # flag, however explicitly it was typed. + engine_env = {**task_flags, **(_task_serving_override(task_serving) or {})} or None + # What a child spawned now WOULD conclude — recorded when one is spawned, and compared against + # the live record when one is not (issue 60). + task_state = _task_state_of_the_child(engine_env) # Remote has ONE identity per grid (the token pins the relay node_id), so `grid join` is additive: # merge this join's engines into whatever is already serving, then respawn the single detached engine. @@ -340,6 +458,11 @@ def cmd_remote_join(args: argparse.Namespace) -> int: print(adrift.detail, file=sys.stderr) return 0 print(f"Already serving on {label}; nothing to append.") + # Said HERE rather than folded into the gate above: a task-configuration change is not a + # reason to restart a serving provider behind the operator's back (issue 60). + drift = _task_serving_drift(live, task_state) + if drift: + print(drift, file=sys.stderr) # The serve process records a hot-reload that failed AFTER the CLI reported success (the # SIGHUP is fire-and-forget) — surface it here, or the no-op compounds the false success. stale = _identity_field(live, "last_reload_error") @@ -364,6 +487,10 @@ def cmd_remote_join(args: argparse.Namespace) -> int: # advertised, and the reload pins the advertised capacity to the actual live pool anyway). if getattr(args, "max_concurrency", None) is None and base: record["max_concurrency"] = _identity_field(base, "max_concurrency") + # What the child will actually conclude, not what was asked for — issue 58 withholds the + # opt-in from a provider that cannot run a task, and recording the request would make the + # next join read that as task serving being on. + record.update(task_state) # Zero-drop when we can: SIGHUP the live singleton to hot-reload the union in place — an appended # API engine reloads too now that its bearer is re-read from the key store (issue 05). Fall back to # stop-respawn for a first join, a legacy/pre-handler process, a launch, a media change, a @@ -378,6 +505,12 @@ def cmd_remote_join(args: argparse.Namespace) -> int: reloaded = (not rotated_live) and (not respawn) and _hot_reloadable(live, merged_specs, record) if reloaded: reloaded = _hot_reload_identity(network_id, record, live) # False if it fell back to a respawn + if reloaded: + # A reload re-reads the RECORD; it cannot hand a running process a new environment, + # so a task-configuration change goes the same way as on the no-op path (issue 60). + drift = _task_serving_drift(live, task_state) + if drift: + print(drift, file=sys.stderr) else: # stops prior process(es) then respawns; aborts on failure _respawn_identity(network_id, record, live, env_overrides=engine_env) diff --git a/docs/adr/0032-a-task-is-not-an-inference-transaction.md b/docs/adr/0032-a-task-is-not-an-inference-transaction.md index b2a86b7..2bd57ab 100644 --- a/docs/adr/0032-a-task-is-not-an-inference-transaction.md +++ b/docs/adr/0032-a-task-is-not-an-inference-transaction.md @@ -136,10 +136,28 @@ correlate replies. Publishing keeps one direction and one reader. - Providers gain a second claim loop and a supervisor process. Task capacity is configured **per provider** and is not `max_concurrency`: the real ceiling is the rate limit of the provider's own Claude subscription, shared by every child it spawns. -- Every provider must run tasks at an **identical absolute path** (`/var/grid/projects// +- ~~Every provider must run tasks at an **identical absolute path** (`/var/grid/projects// workspace`). Claude Code derives a session's transcript directory from the working directory (`~/.claude/projects//`), so a provider using a different prefix cannot - `--resume` a session another one started. + `--resume` a session another one started.~~ + **AMENDED 2026-08-24 (issue 62): providers do NOT have to agree on the path.** The rule was true + of the design it described, and **issue 06 replaced that design** in this same ADR — the + transcript now lives inside the git worktree (`.grid/agent//`) and travels in the + ordinary result commit, and `link_transcript` plants Claude Code's per-cwd symlink at whatever + name *this* provider's own workspace flattens to, pointing at what arrived through git. Every + provider computes its own name and reaches the same conversation. + **What measured it**: issue 35's measurement 5 exists to put exactly this question — the + transcript is pushed to `refs/grid/agent/`, fetched into a second bare repository, + materialized into a workspace **at a different absolute path**, and resumed there; *"the different + path is the whole point"* (`tests/measure_non_dev/agent_tier.py`). It resumed, compacted, with the + token planted before the compaction coming back. + **What replaces it**: three constraints, all local to one provider — the flattened name must stay + under `task_agent.TRANSCRIPT_NAME_MAX_CHARS`; the root must be outside `$HOME` and `GRID_HOME`, or + the sandbox refuses it; and the provider must be able to write there. The default is therefore + per-platform (`/var/grid` on Linux, `/Users/Shared/grid` on macOS, where `/var` is root-owned and + a provider without `sudo` fails every task on it). + Recorded as an amendment rather than a deletion: the record of *why it stopped being true* is what + stops it coming back. - `CLAUDE_CONFIG_DIR` stays **fixed per provider** and holds the provider's own credential. Per-user config directories are not a rejected preference — they are **measured to be broken**: pointing the variable at a fresh directory yields `Not logged in · Please run /login` even on macOS, where the diff --git a/docs/cli.md b/docs/cli.md index 066a839..0548acc 100644 --- a/docs/cli.md +++ b/docs/cli.md @@ -2188,20 +2188,33 @@ commands; check `bubblewrap` and `socat` first. **`grid join` checks all of this before it serves.** With `GRID_TASKS` on, the join asks the same questions a task would — is Claude Code installed and new enough, is the permission mode and -passthrough list valid, can the sandbox start, and can this account create a workspace under -`GRID_TASK_ROOT` — and it asks them *in the terminal you typed the command in*. A provider that +passthrough list valid, can the sandbox start, **are `bubblewrap` and `socat` both installed** (on +Linux, with the sandbox on), and can this account create a workspace under `GRID_TASK_ROOT` — and it +asks them *in the terminal you typed the command in*. A provider that fails any of them still **serves inference**: the join lands, the answer says what to change, and only task serving is withheld until you fix it and re-run `grid join --respawn`. Before this, every one of those was checked only after a task had been claimed, so the first person to find out was a member of your grid whose task died. +**Or say it on the command line.** `grid join --tasks` turns task serving on for that join, with +`--max-tasks N` and `--tasks-root PATH` beside it; each **wins over** the matching environment +variable. Every variable below keeps working exactly as it does today, with or without the flags. +A bad `--max-tasks` is refused rather than defaulted — you are at the terminal and can retype it, +which is the one difference from `GRID_MAX_TASKS`'s rule. + +⚠️ **Turning it on over an engine that is already serving does not take effect by itself.** Task +serving is read from the environment once, when the serve child starts, so a join that changes +nothing else — or one that hot-reloads — cannot hand the running process a new setting. `grid join` +now says so and points at `grid join --respawn`; it will not restart a serving provider on its own, +because the in-flight requests that would drop are yours to decide about. + The environment variables that tune a provider, all optional: | variable | default | what it does | |---|---|---| | `GRID_TASKS` | off | `1` to claim tasks at all | | `GRID_MAX_TASKS` | `1` | how many tasks this provider runs at once. **Anything above 1 is unverified** — two Claude Code children sharing one config directory has never been measured (see [How much a provider takes on](#how-much-a-provider-takes-on)). No upper limit is imposed: the ceiling that actually binds is the provider's own Claude subscription, which is read at runtime rather than guessed at. A value that is not a positive whole number falls back to `1` and says so | -| `GRID_TASK_ROOT` | `/var/grid` | root of the workspace tree (`/projects////workspace`). **Keep it short** — the whole path becomes one directory name and grid adds ~126 characters below the root. **Every provider in a grid must agree** — Claude Code derives a session's transcript directory from the working directory, so a provider using a different root cannot resume a session another one started | +| `GRID_TASK_ROOT` | `/var/grid` | root of the workspace tree (`/projects////workspace`). **Keep it short** — the whole path becomes one directory name and grid adds ~126 characters below the root. Providers **do not** have to agree on it: the conversation travels inside the project's repository, and each provider points Claude Code at its own copy (ADR 0032, amended). The default is `/var/grid` on Linux and `/Users/Shared/grid` on macOS, where `/var` is root-owned | | `GRID_TASK_MAX_WORKSPACES` | `8` | how many conversations keep a working directory on this provider. Past this, the least recently used are deleted at the start of the next task — never by refusing work, and never one a worker is using. An evicted conversation rebuilds itself on its next task at the cost of one fetch. A value that is not a positive whole number falls back to `8` and says so | | `GRID_TASK_MIN_FREE_GB` | off | a floor under free space on the filesystem holding `GRID_TASK_ROOT`. Set it and the provider keeps evicting workspaces while free space is below it. Off by default because it is a promise about a disk the provider does not own alone: a machine already below the line for reasons of its own would evict everything on every task and re-fetch it | | `GRID_TASK_TIMEOUT_SECONDS` | `3600` | how long one agent run may take before the provider gives up on it | diff --git a/remote/task_agent.py b/remote/task_agent.py index 3369e04..75eeb21 100644 --- a/remote/task_agent.py +++ b/remote/task_agent.py @@ -9,18 +9,38 @@ import json import os import re +import shutil +import stat +import sys from dataclasses import dataclass from pathlib import Path from . import task_repo, task_sandbox, task_worktree -# LOCKSTEP (PRD `.scratch/distributed-tasks/PRD.md`): **every provider must use the identical -# absolute path**, because Claude Code derives a session's transcript directory from the working -# directory (`~/.claude/projects//`). A provider using a different prefix cannot -# `--resume` a session another one started, which is the whole of issue 06. -DEFAULT_WORKSPACE_ROOT = "/var/grid" -# Overridable only so tests and dev boxes need not write under `/var`. An operator who changes this -# on one provider and not the others breaks cross-provider resume — the flag is not a preference. +def default_workspace_root(platform: str | None = None) -> str: + """Where task workspaces go when nobody said (issue 62). Per PLATFORM, and that is now allowed. + + ⚠️ **The rule this used to obey is retired** (ADR 0032, amended). It said every provider in a + grid must run tasks at an identical absolute path, because Claude Code derives a session's + transcript directory from the working directory. That was true of the design it was written + for, and **issue 06 replaced that design**: the transcript now lives inside the git worktree and + travels in the ordinary result commit, and `link_transcript` plants the per-cwd symlink at + whatever name *this* provider's own workspace flattens to. Every provider computes its own name + and reaches the same conversation. Issue 35's measurement 5 put exactly this question — a + transcript pushed to `refs/grid/agent/`, fetched into a second repository, materialized **at + a different absolute path**, and resumed there — and it resumed. + + `/var` is root-owned on macOS, so a provider without `sudo` cannot use `/var/grid` and fails + EVERY task on it. `/Users/Shared` is the short, outside-`$HOME` place a normal account can write + — see `ensure_default_workspace_root` for what that costs and how it is paid for. + """ + return "/Users/Shared/grid" if (platform or sys.platform) == "darwin" else "/var/grid" + + +DEFAULT_WORKSPACE_ROOT = default_workspace_root() +# Overridable, and a real choice rather than a lockstep value: what constrains a root is local to +# one provider — the flattened name must clear `TRANSCRIPT_NAME_MAX_CHARS`, the path must be outside +# `$HOME` and `GRID_HOME` for the sandbox, and this account must be able to write it. WORKSPACE_ROOT_ENV = "GRID_TASK_ROOT" @@ -361,9 +381,11 @@ def transcript_dir_name(cwd: Path) -> str: caller that trusted the string. A live two-task run is what found it — every unit test compared our own computation against itself and agreed. - It also sharpens the lockstep rule: providers must agree on the **resolved** absolute workspace - path. One provider reaching `/var/grid` through a symlink and another not is already two - different conversations as far as Claude Code is concerned. + ⚠️ **This used to say providers must agree on the resolved absolute workspace path.** They do + not (ADR 0032, amended by issue 62): the transcript travels in the git worktree since issue 06, + so each provider computes its own name and finds the same conversation there. What survives is + the RESOLVING — the name must come from the path the child reports, not from the string we + built, because a process's `getcwd` has already followed every symlink on the way in. """ return _TRANSCRIPT_NAME_REPLACED.sub("-", str(cwd.resolve(strict=False))) @@ -763,9 +785,46 @@ def preflight_before_serving() -> None: """ preflight() resolve_binary() + _require_the_sandbox_packages() _require_a_reachable_workspace_root() +# What the Linux sandbox needs on the box, and it is **two packages, not one** (issue 59). MEASURED +# on Ubuntu 24.04 against Claude Code 2.1.223: with `bwrap` present and `socat` absent the run still +# refuses, naming `socat`. A probe that checked only `bwrap` would report healthy for a provider +# that fails every task it claims. A stock provider VM has neither. +# Named the way the VENDOR names them in its own refusal — `bubblewrap (bwrap) not installed, +# socat not installed` — so an operator who meets both messages is reading about one thing. +_SANDBOX_PACKAGES = (("bwrap", "bubblewrap (bwrap)"), ("socat", "socat")) +_SANDBOX_PACKAGE_INSTALL = "apt install bubblewrap socat" + + +def _require_the_sandbox_packages() -> None: + """Refuse a Linux provider whose sandbox cannot start, before it claims anybody's task. + + Until this existed, the only enforcement was Claude Code's own `failIfUnavailable` — which fires + **inside the child**, after the claim and after the repository was fetched, and reports it on a + member's task. Nothing in this tree checked: `bwrap`, `bubblewrap` and `socat` appeared in the + source only inside comments. + + **Linux only, and only while the sandbox is on.** macOS needs neither package, so a probe that + fired there would be a refusal handed to a provider that would have worked; and the packages are + the sandbox's own requirement, so an operator who turned the sandbox off deliberately gets the + provider that existed before it — the same shape the version floor has, and for the same reason. + """ + if sys.platform != "linux" or not task_sandbox.enabled(): + return + missing = [package for binary, package in _SANDBOX_PACKAGES if shutil.which(binary) is None] + if not missing: + return + raise OSError( + f"the task sandbox needs {' and '.join(missing)} on this box, and " + f"{'they are' if len(missing) > 1 else 'it is'} not installed — every task would fail " + f"inside the agent with `sandbox required but unavailable`. Install both with: " + f"{_SANDBOX_PACKAGE_INSTALL}, or set {task_sandbox.SANDBOX_ENV}=0 to run agents unconfined " + f"deliberately.") + + def _require_a_reachable_workspace_root() -> None: """Refuse a workspace root this provider could not put a task under. @@ -786,7 +845,7 @@ def _require_a_reachable_workspace_root() -> None: if root.exists() and not root.is_dir(): raise OSError( f"the task workspace root {root} is not a directory, so no task can be checked out " - f"under it; point {WORKSPACE_ROOT_ENV} somewhere else") + f"under it; point {WORKSPACE_ROOT_ENV} somewhere else.") reachable = root while not reachable.exists(): @@ -797,7 +856,82 @@ def _require_a_reachable_workspace_root() -> None: return raise OSError( f"this provider cannot create a task workspace under {root}: {reachable} is not writable " - f"by it. Point {WORKSPACE_ROOT_ENV} at a short path this account can write") + f"by it. Point {WORKSPACE_ROOT_ENV} at a short path this account can write.") + + +def ensure_default_workspace_root() -> Path: + """Create the platform default root, privately — or refuse one this provider cannot trust. + + ⚠️ **A root the FLAG chose is not a root the operator chose, and it needs a stricter rule than + `ensure_workspace` applies.** That function deliberately leaves an existing directory's mode + alone ("anything that already existed is the operator's business") and deliberately permits a + symlink above the workspace level, because relocating storage that way is legitimate. Both are + right for a path a person named. Neither is right for a default nobody chose — which is why this + runs ONLY for the default, and never for `--tasks-root` or `GRID_TASK_ROOT`. + + MEASURED on macOS 26.6, 2026-08-24: `/Users/Shared` is `drwxrwxrwt` root:wheel — **1777, and + sticky**. The sticky bit stops one account deleting another's entries; it does **not** stop a + second local account creating the grid root there first. And a directory created there under the + default umask lands `0755` — every local account able to read every member's checked-out + repository and every `.grid/agent/` transcript on the machine. + + So the property delivered is: **this provider can write the root, and no other account can read + it.** Stated as a property rather than as "owned by another uid", because a `/var/grid` created + once with `sudo` and then handed to a non-root provider is a legitimate and likely Linux setup — + what disqualifies it is being readable by everyone, not who made it, and the fix is one `chmod`. + + The mode goes to `mkdir` rather than a following `chmod`, so no window exists in which the + directory is readable; and `mkdir` is NOT `exist_ok`, so a second account that wins the race + between the check and the create is caught rather than adopted. + """ + root = Path(DEFAULT_WORKSPACE_ROOT) + if root.is_symlink(): + raise OSError( + f"the default task workspace root {root} is a symlink, and this provider will not run " + f"agents under a path it did not place. Name one yourself with --tasks-root.") + if root.exists(): + _require_a_private_directory(root) + return root + if not root.parent.is_dir(): + raise OSError( + f"{root.parent} does not exist, so the default task workspace root cannot be created " + f"there; name one yourself with --tasks-root.") + try: + os.mkdir(root, 0o700) + except FileExistsError: + # Another account created it between the check above and this call — the race the sticky + # world-writable parent makes real. Judge what is there rather than assuming it is ours. + _require_a_private_directory(root) + except OSError as exc: + raise OSError( + f"could not create the default task workspace root {root} ({exc.strerror or exc}); " + f"name one yourself with --tasks-root.") from exc + return root + + +def _require_a_private_directory(root: Path) -> None: + """Refuse a pre-existing default root that another account owns or can read. + + Three refusals, each for its own reason. **Not a directory**: nothing can be checked out under + it, and saying "not writable" about it would send an operator to `chmod`. **Another account's**: + this provider would be running agents inside a directory somebody else controls. **Readable by + group or other**: the members' repositories and their conversations are in there. + """ + info = root.stat() + if not stat.S_ISDIR(info.st_mode): + raise OSError( + f"the default task workspace root {root} exists and is not a directory; name one " + f"yourself with --tasks-root.") + if info.st_uid != os.getuid(): + raise OSError( + f"the default task workspace root {root} belongs to another account (uid " + f"{info.st_uid}), and this provider will not run agents inside it. Give it to this " + f"account, or name a different one with --tasks-root.") + if info.st_mode & (stat.S_IRWXG | stat.S_IRWXO): + raise OSError( + f"the default task workspace root {root} can be read by other accounts on this machine, " + f"and members' repositories and conversations are kept under it. Close it with " + f"`chmod 700 {root}`, or name a different one with --tasks-root.") def _require_version_for_the_sandbox(binary: str) -> None: diff --git a/tests/e2e_agent_settings.py b/tests/e2e_agent_settings.py index 4e89a2e..91a2547 100644 --- a/tests/e2e_agent_settings.py +++ b/tests/e2e_agent_settings.py @@ -146,6 +146,12 @@ def _mcp_config(marker: Path) -> str: "command": "/bin/sh", "args": ["-c", f"/usr/bin/touch {marker}; sleep 30"]}}}) +# The same values `tests/test_task_agent.py` uses, so a claim built here and a claim built there +# are the same shape. A `member_key` is a hex digest in the field; a conversation id is a uuid4. +_MEMBER = "9f2b" * 8 +_CONVERSATION = "2f0b9b1e-7a4c-4d5e-9c31-0a1b2c3d4e5f" + + def _run(tmp_path: Path, files: dict[str, str], prompt: str, *, guarded: bool, project: str): """One task, through the real `run_task`, against a workspace carrying `files`.""" from remote import task_agent, tasks @@ -153,7 +159,15 @@ def _run(tmp_path: Path, files: dict[str, str], prompt: str, *, guarded: bool, p remote, commit = _remote_for(tmp_path, "task/T1", files) if not guarded: _strip_the_guard(task_agent) - job = {"task_id": "T1", "project_id": project, "prompt": prompt, "attempt": 1, + job = {"task_id": "T1", "project_id": project, "member_key": _MEMBER, + # ⚠️ Both of these are REFUSALS, not defaults, and both were added to the claim after + # this module was written — which is how it came to fail every check for a reason that + # had nothing to do with what it tests. `member_key` (ADR 0033 issue 11) is fail-closed + # by design: without it a provider cannot tell whose workspace a task belongs to and + # refuses rather than share one between members. `conversation_id` (ADR 0034 D-a) is the + # CONVERSATION, while `task_id` above is the TURN — two keys for two objects. + "conversation_id": _CONVERSATION, + "prompt": prompt, "attempt": 1, "input_commit": commit, "branch": "task/T1"} return tasks.run_task(job, remote=remote) diff --git a/tests/test_local_cli.py b/tests/test_local_cli.py index 1facd76..c05c036 100644 --- a/tests/test_local_cli.py +++ b/tests/test_local_cli.py @@ -38097,3 +38097,457 @@ def test_the_task_check_runs_before_the_join_stops_or_spawns_anything(monkeypatc f"the join acted before it asked whether this provider can run a task: {order}") assert "spawned" in order, "the join never spawned, so this proves nothing about the order" assert spawned is not None + + +def test_the_join_runs_the_REAL_check_not_a_stand_in(monkeypatch, tmp_path, capsys): + """The seam every other test in this group mocks away. + + ⚠️ The four tests above patch `preflight_before_serving`, and its own unit tests patch + `preflight` and `resolve_binary` — so `cmd_remote_join` → `preflight_before_serving` → + `preflight` had **never executed as one chain**. Mocking on both sides of a seam is how a + contract comes to be verified by nothing, and this repository has paid for that before. + + Nothing here is patched but the grid and the spawn. The refusal is reached through + `GRID_TASK_PERMISSION_MODE`, deliberately: it is the FIRST thing `preflight()` checks, it needs + no subprocess and no Claude Code on the box, so this test asserts the same thing on a developer's + Mac and on a Linux CI runner that has no agent installed at all. + """ + from remote import task_sandbox + + _seed_running_remote_grid(monkeypatch, tmp_path) + spawned = _mock_remote_spawn(monkeypatch) + monkeypatch.setenv("GRID_TASKS", "1") + monkeypatch.delenv(task_sandbox.SANDBOX_ENV, raising=False) # sandbox on: the mode is refused + monkeypatch.setenv("GRID_TASK_PERMISSION_MODE", "bypassPermissions") + + assert cli.main(["join", "--serve", "m"]) == 0, "a task misconfiguration failed the whole join" + + err = capsys.readouterr().err + assert "GRID_TASK_SANDBOX" in err, ( + f"the real check's own sentence did not reach the operator's terminal: {err!r}") + assert "respawn" in err, f"the refusal does not say how to retry it: {err!r}" + assert cli.provider._read_records("n1")["remote"]["models"] == ["m"], "inference did not join" + assert not _the_child_would_claim_tasks(spawned), ( + "the child was spawned still claiming tasks this provider cannot run") + + +# --- issue 61: `grid join --tasks` is the surface of task serving --------------------------------- + + +def _child_task_env(spawned: dict) -> dict[str, str]: + """The three task variables the child was actually handed, whatever set them.""" + from remote import task_agent, task_opt_in + + env = _child_env(spawned) + return {k: env.get(k) for k in + (task_opt_in.SERVING_ENV, task_opt_in.WORKERS_ENV, task_agent.WORKSPACE_ROOT_ENV)} + + +def test_join_help_says_this_provider_can_serve_tasks(capsys): + """Task serving was configured entirely through the environment, and `grid join --help` had no + word about it — a provider could not discover the feature from the CLI that has it.""" + from cli.parser import build_parser + + parser = build_parser() + joins = [a for a in parser._subparsers._group_actions[0].choices.items() if a[0] == "join"] + assert joins, "no `join` subparser" + text = joins[0][1].format_help() + + assert "--tasks" in text, "`grid join --help` still says nothing about task serving" + assert "--tasks-root" in text and "--max-tasks" in text + + +def test_tasks_flag_alone_starts_a_provider_that_claims_tasks(monkeypatch, tmp_path): + """No environment variable set anywhere — the flag is the whole opt-in.""" + from remote import task_agent + + _seed_running_remote_grid(monkeypatch, tmp_path) + spawned = _mock_remote_spawn(monkeypatch) + monkeypatch.delenv("GRID_TASKS", raising=False) + monkeypatch.delenv("GRID_TASK_ROOT", raising=False) + # ⚠️ The default root is a REAL path (`/Users/Shared/grid`, `/var/grid`), and `--tasks` creates + # it. Pointed at tmp_path so this test does not write to the developer's machine — and does not + # pass on macOS while failing on a Linux runner that cannot write `/var`. + monkeypatch.setattr(task_agent, "DEFAULT_WORKSPACE_ROOT", str(tmp_path / "default-root")) + monkeypatch.setattr(task_agent, "preflight_before_serving", lambda: None) + + assert cli.main(["join", "--serve", "m", "--tasks"]) == 0 + + assert _the_child_would_claim_tasks(spawned), "the flag did not reach the child" + + +def test_the_task_flags_reach_the_child(monkeypatch, tmp_path): + """`--max-tasks` sizes the pool and `--tasks-root` places the workspaces; both are read by the + CHILD from its environment, so this asserts what the child was handed.""" + from remote import task_agent + + _seed_running_remote_grid(monkeypatch, tmp_path) + spawned = _mock_remote_spawn(monkeypatch) + monkeypatch.setattr(task_agent, "preflight_before_serving", lambda: None) + root = tmp_path / "elsewhere" + + assert cli.main(["join", "--serve", "m", "--tasks", + "--max-tasks", "3", "--tasks-root", str(root)]) == 0 + + env = _child_task_env(spawned) + assert env["GRID_MAX_TASKS"] == "3" + assert env["GRID_TASK_ROOT"] == str(root) + + +def test_the_environment_still_works_with_no_flag(monkeypatch, tmp_path): + """Providers are running on these variables today; a rollout may not move them.""" + from remote import task_agent + + _seed_running_remote_grid(monkeypatch, tmp_path) + spawned = _mock_remote_spawn(monkeypatch) + monkeypatch.setenv("GRID_TASKS", "1") + monkeypatch.setenv("GRID_MAX_TASKS", "2") + monkeypatch.setattr(task_agent, "preflight_before_serving", lambda: None) + + assert cli.main(["join", "--serve", "m"]) == 0 + + assert _the_child_would_claim_tasks(spawned) + assert _child_task_env(spawned)["GRID_MAX_TASKS"] == "2" + + +@pytest.mark.parametrize("variable, flag, flag_value, expected", [ + ("GRID_MAX_TASKS", "--max-tasks", "5", "5"), + ("GRID_TASK_ROOT", "--tasks-root", "/Users/Shared/from-the-flag", "/Users/Shared/from-the-flag"), +]) +def test_the_flag_wins_over_the_environment( + monkeypatch, tmp_path, variable, flag, flag_value, expected): + """Stated in `--help` as well as tested: an operator who passes a flag AND has an old variable + exported in a shell profile must not have to work out which one is live.""" + from remote import task_agent + + _seed_running_remote_grid(monkeypatch, tmp_path) + spawned = _mock_remote_spawn(monkeypatch) + monkeypatch.setenv(variable, "/from/the/environment" if flag == "--tasks-root" else "9") + if variable != "GRID_TASK_ROOT": + monkeypatch.setenv("GRID_TASK_ROOT", str(tmp_path / "named")) # never the real default + monkeypatch.setattr(task_agent, "preflight_before_serving", lambda: None) + + assert cli.main(["join", "--serve", "m", "--tasks", flag, flag_value]) == 0 + + assert _child_task_env(spawned)[variable] == expected + + +@pytest.mark.parametrize("bad, because", [ + ("0", "at least 1"), + ("-1", "at least 1"), + ("three", "not a whole number"), +]) +def test_a_bad_max_tasks_is_refused_in_front_of_the_operator(monkeypatch, tmp_path, capsys, bad, because): + """⚠️ **Deliberately NOT `GRID_MAX_TASKS`'s rule.** That variable falls back and says so, + because refusing would take task serving down for the life of a running process. A flag is a + different situation: the operator is at the terminal, they just typed it, and a typo they are + told about costs one retry. `argparse` exits 2, which is "ask again" rather than "done".""" + _seed_running_remote_grid(monkeypatch, tmp_path) + _mock_remote_spawn(monkeypatch) + + with pytest.raises(SystemExit) as excinfo: + cli.main(["join", "--serve", "m", "--tasks", "--max-tasks", bad]) + + assert excinfo.value.code == 2, "a typo was accepted, or refused as though the join had run" + err = capsys.readouterr().err + assert "--max-tasks" in err + # ⚠️ The REASON, not just the flag name. Before the flag existed this assertion passed on + # argparse's "unrecognized arguments: --max-tasks" — a test green because its subject was + # missing is the shape this suite has been caught by before. + assert because in err, f"refused for the wrong reason: {err!r}" + + +def test_no_other_flag_turns_task_serving_on(monkeypatch, tmp_path): + """⚠️ Opt-in is not a convenience: a task loop spends the operator's own agent subscription, so + nothing in this feature may turn it on for them. `--tasks` is a person typing it.""" + _seed_running_remote_grid(monkeypatch, tmp_path) + spawned = _mock_remote_spawn(monkeypatch) + monkeypatch.delenv("GRID_TASKS", raising=False) + + assert cli.main(["join", "--serve", "m", "--max-tasks", "4", + "--tasks-root", str(tmp_path / "r"), "--respawn"]) == 0 + + assert not _the_child_would_claim_tasks(spawned), ( + "a flag other than --tasks turned task serving on") + + +def test_tasks_flag_runs_the_preflight(monkeypatch, tmp_path, capsys): + """The flag is a second door onto task serving, and issue 58's check guards the first. A door + that skips the guard is the failure this repository has recorded more than once.""" + from remote import task_agent + + _seed_running_remote_grid(monkeypatch, tmp_path) + spawned = _mock_remote_spawn(monkeypatch) + monkeypatch.delenv("GRID_TASKS", raising=False) + monkeypatch.delenv("GRID_TASK_ROOT", raising=False) + # ⚠️ The default root is a REAL path (`/Users/Shared/grid`, `/var/grid`), and `--tasks` creates + # it. Pointed at tmp_path so this test does not write to the developer's machine — and does not + # pass on macOS while failing on a Linux runner that cannot write `/var`. + monkeypatch.setattr(task_agent, "DEFAULT_WORKSPACE_ROOT", str(tmp_path / "default-root")) + monkeypatch.setattr(task_agent, "preflight_before_serving", + _raise(RuntimeError("Claude Code isn't installed on this provider"))) + + assert cli.main(["join", "--serve", "m", "--tasks"]) == 0 + + assert "isn't installed" in capsys.readouterr().err + assert not _the_child_would_claim_tasks(spawned) + + +@pytest.mark.parametrize("flag, extra", [ + ("--tasks", []), + ("--max-tasks", ["4"]), + ("--tasks-root", ["/Users/Shared/x"]), +]) +def test_the_task_flags_are_remote_only(monkeypatch, tmp_path, flag, extra): + """The task plane is the relay's; local mode has no equivalent of its poll loop. Refused the way + every other remote-only join flag is. + + ⚠️ This is also what pins `default=None` on all three. `provider._reject_remote_only_flags` + decides "was this flag used" with `is not None`, so a `store_true` defaulting to False would + refuse EVERY local `grid join` — the group's own comment says so, and this parametrised row for + `--tasks` is what would catch it. + """ + monkeypatch.setenv("GRID_HOME", str(tmp_path)) + runtime.init_grid_config(name="home", port=8090) + args = cli.build_parser().parse_args(["join", "home", "--serve", "m", flag, *extra]) + + with pytest.raises(SystemExit) as exc: + cli.cmd_join(args) + + assert flag in str(exc.value) and "remote" in str(exc.value).lower() + + +def test_a_plain_local_join_is_not_refused_by_the_task_flags(monkeypatch, tmp_path): + """The other half of the pair above, and the one that fails if `--tasks` ever defaults to False: + a local join that names no task flag must reach its own error, not the remote-only one.""" + monkeypatch.setenv("GRID_HOME", str(tmp_path)) + runtime.init_grid_config(name="home", port=8090) + args = cli.build_parser().parse_args(["join", "home", "--serve", "m"]) + + assert getattr(args, "tasks", None) is None, "--tasks defaults to False, which refuses every local join" + + +@pytest.mark.parametrize("env_root_ok, flag_root_ok, tasks_on", [ + (False, True, True), # the flag rescues a root the shell exported wrongly + (True, False, False), # and it can break one the shell had right — the flag is what is checked +]) +def test_the_preflight_checks_the_root_the_FLAG_names( + monkeypatch, tmp_path, capsys, env_root_ok, flag_root_ok, tasks_on): + """⚠️ The reason `_as_the_child_will_see_it` exists at all. + + The checks read `os.environ`, because that is what the CHILD reads. A `--tasks-root` this + process was never started with therefore reaches them only if the parent puts it where they + look — otherwise `grid join --tasks --tasks-root ` is refused over a stale variable in a + shell profile, and `--tasks-root ` sails through on a good one. Both directions, because + only the pair shows that the FLAG is what was consulted. + + `preflight` and `resolve_binary` are stubbed so this needs no Claude Code on the box; the + workspace-root check is the real one, and it is the whole subject here. + """ + from remote import task_agent + + _seed_running_remote_grid(monkeypatch, tmp_path) + spawned = _mock_remote_spawn(monkeypatch) + monkeypatch.setattr(task_agent, "preflight", lambda: None) + monkeypatch.setattr(task_agent, "resolve_binary", lambda: "claude") + # ⚠️ Issue 59's probe is Linux-only, so a test that skipped this passes on a Mac and fails on a + # Linux runner, where `bwrap` and `socat` really are absent. + monkeypatch.setattr(task_agent.shutil, "which", lambda name: f"/usr/bin/{name}") + denied = tmp_path / "denied" + denied.mkdir(mode=0o500) + good, bad = tmp_path / "fine" / "root", denied / "root" + monkeypatch.setenv("GRID_TASK_ROOT", str(good if env_root_ok else bad)) + + assert cli.main(["join", "--serve", "m", "--tasks", + "--tasks-root", str(good if flag_root_ok else bad)]) == 0 + + assert _the_child_would_claim_tasks(spawned) is tasks_on, ( + f"the check consulted the environment's root, not the flag's: {capsys.readouterr().err!r}") + + +# --- issue 60: turning task serving on must not be a silent no-op --------------------------------- + + +_LIVE_ENGINE = [{"endpoint_url": "http://h:11434/v1", "models": ["llama3"], "engine_label": "ollama"}] + + +def _rejoin_identically(): + """The idempotent re-join: same engine, same model, same display name — the no-op gate's case.""" + return ["join", "--at", "http://h:11434/v1", "-m", "llama3", "--name", "mybox"] + + +def test_turning_task_serving_on_over_a_live_identity_says_it_will_not_take( + monkeypatch, tmp_path, capsys): + """The shape nothing reported. Adding the opt-in changes no engine, model, name, bundle or + media, so the join is declared idempotent — and the running child's environment was fixed when + it was spawned, so task serving never turns on. From outside that is indistinguishable from + "there is no work yet".""" + from remote import task_agent + + _seed_running_remote_grid(monkeypatch, tmp_path) + _seed_live_identity_record(pid=4242, engines=_LIVE_ENGINE, tasks=False) + spawned = _mock_remote_spawn(monkeypatch) + monkeypatch.setenv("GRID_TASKS", "1") + monkeypatch.setattr(task_agent, "preflight_before_serving", lambda: None) + + assert cli.main(_rejoin_identically()) == 0 + + err = capsys.readouterr().err + assert "respawn" in err, f"the operator is not told how to make it take: {err!r}" + assert "cmd" not in spawned, "the join respawned implicitly" + # ⚠️ Signal **0** is `os.kill(pid, 0)`, the liveness probe the no-op gate makes — not a reload. + # Asserting on an empty signal list instead reads a healthy no-op as a hot reload. + assert [s for _, s in spawned["signals"] if s != 0] == [], "the join reloaded implicitly" + + +def test_a_join_that_respawns_records_what_the_child_was_spawned_with(monkeypatch, tmp_path): + """The recorded value is the CHILD's, so the comparison above has something true to read.""" + from remote import task_agent + + _seed_running_remote_grid(monkeypatch, tmp_path) + spawned = _mock_remote_spawn(monkeypatch) + monkeypatch.setenv("GRID_TASKS", "1") + monkeypatch.setattr(task_agent, "preflight_before_serving", lambda: None) + + assert cli.main(["join", "--serve", "m"]) == 0 + + assert cli.provider._read_records("n1")["remote"]["tasks"] is True + assert _the_child_would_claim_tasks(spawned) + + +def test_the_record_says_what_the_child_got_not_what_was_asked_for(monkeypatch, tmp_path): + """⚠️ Issue 58 spawns the child with the opt-in WITHHELD when preflight fails. Recording the + request rather than the outcome would report task serving as on for a provider deliberately + told not to claim — and the next join would then read that lie as the truth.""" + from remote import task_agent + + _seed_running_remote_grid(monkeypatch, tmp_path) + spawned = _mock_remote_spawn(monkeypatch) + monkeypatch.setenv("GRID_TASKS", "1") + monkeypatch.setattr(task_agent, "preflight_before_serving", + _raise(RuntimeError("Claude Code isn't installed on this provider"))) + + assert cli.main(["join", "--serve", "m"]) == 0 + + assert cli.provider._read_records("n1")["remote"]["tasks"] is False + assert not _the_child_would_claim_tasks(spawned) + + +@pytest.mark.parametrize("opt_in", ["1", None]) +def test_a_record_from_before_this_issue_makes_no_claim_either_way( + monkeypatch, tmp_path, capsys, opt_in): + """⚠️ Absent means UNKNOWN, never off. Every record written before this issue carries no such + key, and reading a missing key as `False` would tell every provider already serving tasks that + its task serving is not on — which for most of them is a lie. The same mistake five entries in + the lockstep register exist to record.""" + from remote import task_agent + + _seed_running_remote_grid(monkeypatch, tmp_path) + _seed_live_identity_record(pid=4242, engines=_LIVE_ENGINE) # no `tasks` key at all + _mock_remote_spawn(monkeypatch) + if opt_in is None: + monkeypatch.delenv("GRID_TASKS", raising=False) + else: + monkeypatch.setenv("GRID_TASKS", opt_in) + monkeypatch.setattr(task_agent, "preflight_before_serving", lambda: None) + + assert cli.main(_rejoin_identically()) == 0 + + assert "respawn" not in capsys.readouterr().err, ( + "a record that says nothing was read as saying the opt-in is off") + + +def test_the_worker_count_drift_is_reported_too(monkeypatch, tmp_path, capsys): + """`GRID_MAX_TASKS=4` against a live identity is exactly as inert as the opt-in, and answering + only half of that would leave the obvious next report to somebody else.""" + from remote import task_agent + + _seed_running_remote_grid(monkeypatch, tmp_path) + _seed_live_identity_record(pid=4242, engines=_LIVE_ENGINE, tasks=True, max_tasks=1) + _mock_remote_spawn(monkeypatch) + monkeypatch.setenv("GRID_TASKS", "1") + monkeypatch.setenv("GRID_MAX_TASKS", "4") + monkeypatch.setattr(task_agent, "preflight_before_serving", lambda: None) + + assert cli.main(_rejoin_identically()) == 0 + + err = capsys.readouterr().err + assert "respawn" in err and "4" in err, f"the worker count change is not reported: {err!r}" + + +def test_a_live_identity_already_serving_tasks_is_not_nagged(monkeypatch, tmp_path, capsys): + """The positive control. A note that appeared on every idempotent re-join would be furniture + within a day, and the tests above would all still pass.""" + from remote import task_agent + + _seed_running_remote_grid(monkeypatch, tmp_path) + _seed_live_identity_record(pid=4242, engines=_LIVE_ENGINE, tasks=True, max_tasks=1) + _mock_remote_spawn(monkeypatch) + monkeypatch.setenv("GRID_TASKS", "1") + monkeypatch.delenv("GRID_MAX_TASKS", raising=False) + monkeypatch.setattr(task_agent, "preflight_before_serving", lambda: None) + + assert cli.main(_rejoin_identically()) == 0 + + assert "respawn" not in capsys.readouterr().err + + +def test_the_hot_reload_path_says_it_too(monkeypatch, tmp_path, capsys): + """The second silent path, and the less obvious one. A join that DOES change the union may + SIGHUP the live child instead of respawning — zero dropped requests, which is the point — but a + reload re-reads the run RECORD. It cannot hand a running process a new environment, and the + opt-in is read from the environment once at startup.""" + from remote import task_agent + + _seed_running_remote_grid(monkeypatch, tmp_path) + _seed_live_identity_record(pid=4242, engines=_LIVE_ENGINE, tasks=False) + spawned = _mock_remote_spawn(monkeypatch) + monkeypatch.setenv("GRID_TASKS", "1") + monkeypatch.setattr(task_agent, "preflight_before_serving", lambda: None) + + # A NEW model on the same engine: the union changed, so this is not the no-op gate — and it is + # hot-reloadable, so no child is spawned and no environment is handed over. + assert cli.main(["join", "--at", "http://h:11434/v1", "-m", "llama3", "-m", "mistral", + "--name", "mybox"]) == 0 + + assert "cmd" not in spawned, "this went down the respawn path, so it proves nothing about reload" + assert [s for _, s in spawned["signals"] if s != 0], "no reload signal was sent" + assert "respawn" in capsys.readouterr().err, "a reload that cannot apply the opt-in said nothing" + + +@pytest.mark.parametrize("how", ["flag", "variable"]) +def test_a_root_the_operator_NAMED_is_not_judged_by_the_defaults_rules( + monkeypatch, tmp_path, capsys, how): + """⚠️ Issue 62's strict rules exist because **nobody chose** the default. A path a person named + is their business — the line `ensure_workspace` has always drawn — so a world-readable one they + pointed at is used, not refused. + + Without this the feature would refuse the setups it was written to serve: an operator who + already runs `/srv/grid` at 0755 would be told to `chmod` a directory they configured on purpose. + """ + from remote import task_agent + + _seed_running_remote_grid(monkeypatch, tmp_path) + spawned = _mock_remote_spawn(monkeypatch) + named = tmp_path / "named" + named.mkdir() + named.chmod(0o755) # readable by everyone, and deliberately so + # The real default would be created if the named root were ignored — this is what proves it is not. + monkeypatch.setattr(task_agent, "DEFAULT_WORKSPACE_ROOT", str(tmp_path / "never-made")) + monkeypatch.setattr(task_agent, "preflight", lambda: None) + monkeypatch.setattr(task_agent, "resolve_binary", lambda: "claude") + # ⚠️ Issue 59's probe is Linux-only, so a test that skipped this passes on a Mac and fails on a + # Linux runner, where `bwrap` and `socat` really are absent. + monkeypatch.setattr(task_agent.shutil, "which", lambda name: f"/usr/bin/{name}") + argv = ["join", "--serve", "m", "--tasks"] + if how == "flag": + monkeypatch.delenv("GRID_TASK_ROOT", raising=False) + argv += ["--tasks-root", str(named)] + else: + monkeypatch.setenv("GRID_TASK_ROOT", str(named)) + + assert cli.main(argv) == 0 + + assert _the_child_would_claim_tasks(spawned), ( + f"a root the operator named was judged by the default's rules: {capsys.readouterr().err!r}") + assert not (tmp_path / "never-made").exists(), "the default root was made despite a named one" diff --git a/tests/test_task_agent.py b/tests/test_task_agent.py index 7ad7743..a27581b 100644 --- a/tests/test_task_agent.py +++ b/tests/test_task_agent.py @@ -5,6 +5,8 @@ claim/run/report loop's own tests stay beside the rest of the task-loop suite. """ import json +import stat +import sys import subprocess from pathlib import Path @@ -5459,6 +5461,19 @@ def test_checkout_result_still_refuses_when_there_is_no_branch(tmp_path): # --- issue 58: the provider is asked before a member is waiting ----------------------------------- +def _sandbox_packages_present(monkeypatch): + """Answer issue 59's probe as a box that HAS both packages. + + ⚠️ Every test that drives `preflight_before_serving` for some other reason needs this, and the + need is invisible on macOS: the probe is Linux-only, so without it those tests pass on a + developer's Mac and fail on a Linux runner where the packages really are absent. That is exactly + the failure this suite has a memory about, and issue 59 introduced a fresh instance of it. + """ + from remote import task_agent + + monkeypatch.setattr(task_agent.shutil, "which", lambda name: f"/usr/bin/{name}") + + def test_preflight_before_serving_asks_what_a_claim_would_ask(monkeypatch, tmp_path): """The delegation, pinned. `grid join` must not grow a second opinion about any of this.""" from remote import task_agent @@ -5467,6 +5482,7 @@ def test_preflight_before_serving_asks_what_a_claim_would_ask(monkeypatch, tmp_p asked: list[str] = [] monkeypatch.setattr(task_agent, "preflight", lambda: asked.append("preflight")) monkeypatch.setattr(task_agent, "resolve_binary", lambda: asked.append("binary") or "claude") + _sandbox_packages_present(monkeypatch) task_agent.preflight_before_serving() @@ -5483,6 +5499,7 @@ def test_preflight_before_serving_refuses_a_workspace_root_it_cannot_write(monke monkeypatch.setenv("GRID_TASK_ROOT", str(denied / "grid" / "tasks")) monkeypatch.setattr(task_agent, "preflight", lambda: None) monkeypatch.setattr(task_agent, "resolve_binary", lambda: "claude") + _sandbox_packages_present(monkeypatch) with pytest.raises(OSError) as excinfo: task_agent.preflight_before_serving() @@ -5503,6 +5520,7 @@ def test_preflight_before_serving_accepts_a_root_that_does_not_exist_yet(monkeyp monkeypatch.setenv("GRID_TASK_ROOT", str(root)) monkeypatch.setattr(task_agent, "preflight", lambda: None) monkeypatch.setattr(task_agent, "resolve_binary", lambda: "claude") + _sandbox_packages_present(monkeypatch) task_agent.preflight_before_serving() @@ -5519,6 +5537,7 @@ def test_preflight_before_serving_refuses_a_root_that_is_a_file(monkeypatch, tmp monkeypatch.setenv("GRID_TASK_ROOT", str(root)) monkeypatch.setattr(task_agent, "preflight", lambda: None) monkeypatch.setattr(task_agent, "resolve_binary", lambda: "claude") + _sandbox_packages_present(monkeypatch) with pytest.raises(OSError) as excinfo: task_agent.preflight_before_serving() @@ -5547,3 +5566,230 @@ def test_preflight_before_serving_does_not_move_the_version_floor(monkeypatch, t monkeypatch.setattr(task_agent, "preflight", lambda: None) task_agent.preflight_before_serving() # an ancient binary, and the sandbox is off: allowed + + +# --- issue 62: the default root the flag makes, and the rule that retired ------------------------ + + +@pytest.mark.parametrize("platform, expected", [("darwin", "/Users/Shared/grid"), ("linux", "/var/grid")]) +def test_the_default_root_is_per_platform(platform, expected): + """⚠️ Allowed only because ADR 0032's identical-absolute-path rule is RETIRED. Issue 06 moved + the transcript into the git worktree; issue 35's measurement 5 resumed a compacted one at a + DIFFERENT absolute path. `/var` is root-owned on macOS, so `/var/grid` fails every task there + for a provider without sudo.""" + from remote import task_agent + + assert task_agent.default_workspace_root(platform) == expected + + +@pytest.mark.parametrize("platform", ["darwin", "linux"]) +def test_both_platform_defaults_leave_transcript_headroom(platform): + """Measured for BOTH, not just the one this test happens to run on — a default that flattened + past the limit would lose every conversation with every other signal healthy.""" + from remote import task_agent + + stock = Path(task_agent.default_workspace_root(platform)) / "projects" / ( + "2f0b9b1e-7a4c-4d5e-9c31-0a1b2c3d4e5f") / ("9f2b" * 8) / ( + "8d1a4c60-3b2e-4f7a-95d8-6e0f1a2b3c4d") / "workspace" + + assert len(task_agent.transcript_dir_name(stock)) < task_agent.TRANSCRIPT_NAME_MAX_CHARS + + +def _default_root_at(monkeypatch, path): + from remote import task_agent + monkeypatch.setattr(task_agent, "DEFAULT_WORKSPACE_ROOT", str(path)) + + +def test_the_created_default_root_is_private(monkeypatch, tmp_path): + """`/Users/Shared` is 1777 and a directory created there under the default umask lands 0755 — + every local account able to read every member's repository and every transcript.""" + from remote import task_agent + + root = tmp_path / "grid" + _default_root_at(monkeypatch, root) + + assert task_agent.ensure_default_workspace_root() == root + assert stat.S_IMODE(root.stat().st_mode) == 0o700 + + +def test_a_symlinked_default_root_is_refused(monkeypatch, tmp_path): + """`ensure_workspace` permits a symlink above the workspace, because relocating storage that way + is a legitimate thing an operator does. Nobody chose THIS path, so the same symlink is a second + account redirecting where this provider runs agents.""" + from remote import task_agent + + (tmp_path / "elsewhere").mkdir() + root = tmp_path / "grid" + root.symlink_to(tmp_path / "elsewhere") + _default_root_at(monkeypatch, root) + + with pytest.raises(OSError) as excinfo: + task_agent.ensure_default_workspace_root() + + assert "--tasks-root" in str(excinfo.value) + + +def test_a_world_readable_default_root_is_refused_whoever_owns_it(monkeypatch, tmp_path): + """⚠️ The Linux case this rule is phrased for: `/var/grid` created once with `sudo` and then + handed to a non-root provider is legitimate and likely. What disqualifies it is being readable + by every account, not who made it — so the refusal names `chmod 700`, which is the one command + that fixes it.""" + from remote import task_agent + + root = tmp_path / "grid" + root.mkdir(mode=0o755) + _default_root_at(monkeypatch, root) + + with pytest.raises(OSError) as excinfo: + task_agent.ensure_default_workspace_root() + + assert "chmod 700" in str(excinfo.value), str(excinfo.value) + + +def test_a_root_that_is_already_ours_and_private_is_adopted_silently(monkeypatch, tmp_path): + """The positive control, and the ordinary second run. A rule that refused this would make + `grid join --tasks` work once and fail every time after.""" + from remote import task_agent + + root = tmp_path / "grid" + root.mkdir(mode=0o700) + (root / "kept.txt").write_text("from the first join", encoding="utf-8") + _default_root_at(monkeypatch, root) + + assert task_agent.ensure_default_workspace_root() == root + assert (root / "kept.txt").read_text(encoding="utf-8") == "from the first join" + + +def test_a_default_root_that_is_a_file_says_so_rather_than_talking_about_permissions( + monkeypatch, tmp_path): + """Three refusals, three reasons. Calling this one "not writable" sends an operator to `chmod` + for a fault no mode can fix.""" + from remote import task_agent + + root = tmp_path / "grid" + root.write_text("not a directory", encoding="utf-8") + _default_root_at(monkeypatch, root) + + with pytest.raises(OSError) as excinfo: + task_agent.ensure_default_workspace_root() + + assert "not a directory" in str(excinfo.value) + + +def test_ensure_workspace_still_permits_a_symlink_ABOVE_the_workspace(monkeypatch, tmp_path): + """⚠️ Issue 62's stricter rules are for the DEFAULT root only. `ensure_workspace`'s two + allowances are load-bearing for a path an operator named, and a sweep that "tightened" them + would refuse the legitimate relocation its own docstring describes.""" + from remote import task_agent + + real = tmp_path / "real" + real.mkdir() + linked = tmp_path / "linked" + linked.symlink_to(real) + monkeypatch.setenv("GRID_TASK_ROOT", str(linked)) + + path = task_agent.workspace_for("proj-1", _MEMBER, _CONVERSATION) + task_agent.ensure_workspace(path) # must not raise: the symlink is ABOVE the workspace + + assert path.is_dir() + + +def test_ensure_workspace_still_leaves_an_existing_directorys_mode_alone(monkeypatch, tmp_path): + """The other allowance. A repair walk would happily `chmod` a system directory the first time + somebody points the root at one — `/tmp` is 1777 on purpose.""" + from remote import task_agent + + root = tmp_path / "root" + root.mkdir() + # ⚠️ `chmod` AFTER `mkdir`, not `mkdir(mode=…)`: the umask masks the mode argument, so the + # obvious spelling had this test asserting 0o777 against the 0o755 it actually produced. + root.chmod(0o777) + monkeypatch.setenv("GRID_TASK_ROOT", str(root)) + + task_agent.ensure_workspace(task_agent.workspace_for("proj-1", _MEMBER, _CONVERSATION)) + + assert stat.S_IMODE(root.stat().st_mode) == 0o777, "ensure_workspace repaired a mode it found" + + +# --- issue 59: a Linux provider knows it is missing bubblewrap or socat -------------------------- + + +def _packages(monkeypatch, present): + """Make `shutil.which` answer for exactly the sandbox packages in `present`.""" + import shutil as shutil_module + + from remote import task_agent + monkeypatch.setattr(task_agent.shutil, "which", + lambda name: f"/usr/bin/{name}" if name in present else None) + return shutil_module + + +@pytest.mark.parametrize("present, missing", [ + (("bwrap",), "socat"), # ⚠️ bwrap alone is NOT enough — measured on Ubuntu 24.04 / 2.1.223 + (("socat",), "bwrap"), + ((), "bwrap"), +]) +def test_a_linux_provider_missing_a_sandbox_package_is_told_at_join( + monkeypatch, tmp_path, present, missing): + """The only enforcement before this was Claude Code's own `failIfUnavailable`, which fires + inside the child — after the claim, after the repository was fetched, on a member's task.""" + from remote import task_agent + + monkeypatch.setattr(sys, "platform", "linux") + monkeypatch.delenv("GRID_TASK_SANDBOX", raising=False) + monkeypatch.setenv("GRID_TASK_ROOT", str(tmp_path / "root")) + _packages(monkeypatch, present) + monkeypatch.setattr(task_agent, "preflight", lambda: None) + monkeypatch.setattr(task_agent, "resolve_binary", lambda: "claude") + + with pytest.raises(OSError) as excinfo: + task_agent.preflight_before_serving() + + assert missing in str(excinfo.value), str(excinfo.value) + assert "apt install bubblewrap socat" in str(excinfo.value) + + +def test_a_linux_provider_with_both_packages_is_allowed(monkeypatch, tmp_path): + """⚠️ The positive control. Every assertion above is "it refused", which is exactly what a probe + that refuses everything also reports — and that probe would take task serving off every Linux + provider in the fleet.""" + from remote import task_agent + + monkeypatch.setattr(sys, "platform", "linux") + monkeypatch.delenv("GRID_TASK_SANDBOX", raising=False) + monkeypatch.setenv("GRID_TASK_ROOT", str(tmp_path / "root")) + _packages(monkeypatch, ("bwrap", "socat")) + monkeypatch.setattr(task_agent, "preflight", lambda: None) + monkeypatch.setattr(task_agent, "resolve_binary", lambda: "claude") + + task_agent.preflight_before_serving() + + +def test_the_sandbox_package_probe_does_not_run_off_linux(monkeypatch, tmp_path): + """macOS needs neither package. A probe that fired there is a refusal handed to a provider that + would have worked — the opposite of what this issue is for.""" + from remote import task_agent + + monkeypatch.setattr(sys, "platform", "darwin") + monkeypatch.delenv("GRID_TASK_SANDBOX", raising=False) + monkeypatch.setenv("GRID_TASK_ROOT", str(tmp_path / "root")) + _packages(monkeypatch, ()) # neither present, and it must not matter + monkeypatch.setattr(task_agent, "preflight", lambda: None) + monkeypatch.setattr(task_agent, "resolve_binary", lambda: "claude") + + task_agent.preflight_before_serving() + + +def test_the_sandbox_package_probe_does_not_run_with_the_sandbox_off(monkeypatch, tmp_path): + """The packages are the SANDBOX's requirement. An operator who turned the sandbox off + deliberately gets the provider that existed before it — the same shape as the version floor.""" + from remote import task_agent, task_sandbox + + monkeypatch.setattr(sys, "platform", "linux") + monkeypatch.setenv(task_sandbox.SANDBOX_ENV, "0") + monkeypatch.setenv("GRID_TASK_ROOT", str(tmp_path / "root")) + _packages(monkeypatch, ()) + monkeypatch.setattr(task_agent, "preflight", lambda: None) + monkeypatch.setattr(task_agent, "resolve_binary", lambda: "claude") + + task_agent.preflight_before_serving()