diff --git a/cli/parser.py b/cli/parser.py index 238c86c..fcaebb5 100644 --- a/cli/parser.py +++ b/cli/parser.py @@ -1200,8 +1200,9 @@ def _add_task(sub) -> None: "within about half a minute, on the provider's next lease renewal — and on a provider " "that has not been updated yet it runs to completion, harmlessly, with nothing waiting " "on it.\n\n" - "Nothing is rewound: whatever the agent had already done is kept, so " - "`grid task fetch` still works on it."), + "Nothing is undone. What the agent had reached is another matter: it is stopped " + "part-way and may never have published anything, so `grid task fetch` gives you " + "what the grid has recorded — which can be only what you sent in. It says which."), formatter_class=argparse.RawDescriptionHelpFormatter) cancel.add_argument( "task_id", diff --git a/cli/remote_task.py b/cli/remote_task.py index a754463..bb2ce46 100644 --- a/cli/remote_task.py +++ b/cli/remote_task.py @@ -540,7 +540,13 @@ def _no_trunk_message(args: argparse.Namespace, project_id: str) -> str: """ import shlex - head = (f"Project {project_id} has no main yet, so there is nothing to cut a task from.") + # ⚠️ No git word (ADR 0034 D-m, issue 46). This said "has no main yet" and was the FIRST wall a + # new user meets — `project create` then `task create` — while `tests/test_application_surface` + # reported green, because the literal lives in this local and the scan's walker only followed + # constants handed straight to a sink (ND-06). The walker follows locals now; the wording is + # the other half. "Ready to work in" is `_project_ready`'s own phrase for the state this one is + # the absence of, so the two sides of that coin read as one thing. + head = (f"Project {project_id} has no files yet, so there is nothing to start a task from.") if getattr(args, "init_project", False): # The caller ALREADY asked for a trunk and the relay still says there is none, so offering # `--init-project` would hand back the command that just failed — the "advice that is @@ -1333,9 +1339,9 @@ def _task_fetch(args: argparse.Namespace) -> int: f"Task {args.task_id} finished as {state} but recorded no result to fetch. " f"`grid task get {args.task_id}` shows what it did report.") # A missing `result_commit` used to be refused here too, and that made `grid task cancel` a - # liar: it prints "Its branch is left where the agent got to: grid task fetch ", and this - # command then answered "recorded no result to fetch" — while the branch was on the relay all - # along, holding at least the task's input. + # liar: cancel pointed the user straight at this command, and this command then answered + # "recorded no result to fetch" — while the branch was on the relay all along, holding at + # least the task's input. (Cancel's own wording was the other half, fixed under ND-02.) # # The promise cannot be made conditional at its own end: cancel returns immediately and the # agent does not die until the next lease beat, so at the moment the sentence is printed nobody @@ -1630,10 +1636,25 @@ def _task_cancel(args: argparse.Namespace) -> int: reason = answer.get("error") print(f"task {args.task_id} cancelled — it is now {state}" + (f" ({reason})" if reason else "")) - # Nothing is rewound, so whatever the agent had done is still fetchable. Said out loud, - # because "cancelled" reads as "undone" and here it is not. No git word (ADR 0034 D-m, - # issue 46) — the parser's own description was reworded with it. - print(f"Whatever it had already done is kept: grid task fetch {args.task_id}") + # Nothing is REWOUND, and that is all this command can honestly claim. Said out loud, because + # "cancelled" reads as "undone" and here it is not. No git word (ADR 0034 D-m, issue 46) — the + # parser's own description was reworded with it. + # + # ⚠️ This used to say "Whatever it had already done is kept", and that was a promise this line + # is in no position to make (ND-02). `cancel` returns as soon as the relay records it while the + # agent runs on until the next lease beat, so at the moment these words are printed nobody + # knows whether a result will ever land — and measured on a live grid, the tree a subsequent + # `fetch` returned was the task's INPUT, with none of the agent's edits in it. `_task_fetch` + # already says which of the two it served ("recorded no result … it may hold only the task's + # input"); this end now stops contradicting it in advance. + # + # ⚠️ The command goes LAST on its own line, which is a house rule with a test behind it: + # `test_task_lease.test_every_command_this_cli_tells_you_to_run_actually_parses` reads a + # printed hint as `grid (task|project) …` to the end of the LINE and retypes it, so prose + # trailing the command becomes argv and the parser refuses what this CLI just recommended. + print(f"Nothing is undone. Whether the agent published anything before it stopped is another " + f"matter — this gives you what the grid has recorded, and says which it is:\n" + f" grid task fetch {args.task_id}") return 0 diff --git a/docs/cli.md b/docs/cli.md index 9f074e3..df4e360 100644 --- a/docs/cli.md +++ b/docs/cli.md @@ -1201,6 +1201,17 @@ exactly the documents the relay sent: {"error": {"code": "project_has_no_trunk", "message": "…", "status": 409}} ``` +⚠️ **stderr is not only that document.** The envelope is written on its own line and the same +message then follows as a plain sentence, because a person watching the terminal must still be +told what happened — the envelope is an addition to stderr, not a replacement for it: + +``` +{"error": {"code": null, "message": "`grid task` is a remote-mode command. …", "status": null}} +`grid task` is a remote-mode command. … +``` + +So read the **first line** of stderr, rather than parsing the whole stream as one document. + `code` is the relay's own machine-readable slug, and is `null` for a refusal this CLI raised itself or one from a relay too old to send one — which is ordinary, not an error: branch on `code` when it is there and show `message` when it is not. `status` is the HTTP status when the relay answered and diff --git a/remote/relay.py b/remote/relay.py index 9906e16..854b566 100644 --- a/remote/relay.py +++ b/remote/relay.py @@ -880,6 +880,28 @@ def _task_error_message(resp: httpx.Response) -> str: return f"Task request failed ({resp.status_code}): {resp.text[:400]}" +# A bare framework 404 from `POST /relay/v1/tasks` (ND-13). +# +# ⚠️ **Its own sentence, and pointedly NOT `_OLD_RELAY`'s diagnosis.** Every other missing-route +# hint in this module says "your relay predates this feature", because those routes really did +# arrive with a release. This one did not: `/relay/v1/tasks` exists on every relay that has ever +# had a task plane at all — an out-of-date relay answers **201** here and quietly files the task in +# the caller's own `default`, which is the failure `create_task`'s echoed-`project_id` guard +# catches. So a 404 from THIS route cannot mean an old relay, and saying so would send somebody to +# upgrade a server that is answering correctly. +# +# What it does mean is that whatever is on the other end of this address is not the relay: a proxy +# or gateway in front of it that does not forward the path, or a base URL pointing somewhere else +# entirely. Left untranslated the user's whole diagnosis was the words "Not Found" — the one +# command out of fourteen in the old-relay drill that leaked the framework's own string. +_NOT_THE_RELAY = ( + "This grid's relay address answered 'not found' for the route every relay has, so what is " + "answering there is probably not the relay — usually a proxy in front of it that does not " + "pass the path on, or a base address pointing somewhere else. No task was created. " + "`grid status` shows the address in use." +) + + def create_task( signaling_url: str, access_token: str, @@ -907,7 +929,8 @@ def create_task( body: dict[str, Any] = {"prompt": prompt, "project_id": project_id} if files: body["files"] = files - task = _task_oneshot(signaling_url, access_token, "POST", "/relay/v1/tasks", json=body) + task = _task_oneshot(signaling_url, access_token, "POST", "/relay/v1/tasks", json=body, + missing_route_hint=_NOT_THE_RELAY) # The ONE way this feature can fail silently, and it is the exact bug it exists to kill. # `/relay/v1/tasks` exists on a relay that predates project membership too, so it answers 201 diff --git a/remote/task_agent.py b/remote/task_agent.py index 83336bd..d39df11 100644 --- a/remote/task_agent.py +++ b/remote/task_agent.py @@ -217,12 +217,12 @@ def _safe_segment(kind: str, value: str) -> str: minted — so "not a safe path segment" on its own would leave an operator with three things to check and no way to tell which. - ⚠️ **One NAME is refused as well as one shape** (ADR 0034 D-c, issue 50). The object store lives - at `/store.git`, and `_SAFE_PROJECT_ID` admits a dot — so a conversation id spelled - exactly that is legal by every other rule here and would put a workspace on top of the member's - entire history. Refused in all three positions rather than only the one where the collision is - reachable: one rule is checkable, and "which levels does the store sit between" is the kind of - thing a later layout change moves. + ⚠️ **One NAME — in any casing — is refused as well as one shape** (ADR 0034 D-c, issue 50; the + casing is ND-11). The object store lives at `/store.git`, and `_SAFE_PROJECT_ID` + admits a dot — so a conversation id spelled exactly that is legal by every other rule here and + would put a workspace on top of the member's entire history. Refused in all three positions + rather than only the one where the collision is reachable: one rule is checkable, and "which + levels does the store sit between" is the kind of thing a later layout change moves. """ if not isinstance(value, str): raise ValueError(f"{kind} must be a string, got {type(value).__name__}") @@ -230,7 +230,14 @@ def _safe_segment(kind: str, value: str) -> str: raise ValueError(f"{kind} must be 1-{_MAX_PROJECT_ID_CHARS} characters, got {len(value)}") if value in (".", "..") or not _SAFE_PROJECT_ID.match(value): raise ValueError(f"{kind} {value!r} is not a single safe path segment") - if value == task_worktree.STORE_DIR_NAME: + if value.casefold() == task_worktree.STORE_DIR_NAME.casefold(): + # ⚠️ **`casefold`, never `==`** (ND-11). APFS and NTFS are case-INsensitive by default, so + # `STORE.GIT` and `store.git` are ONE directory on the machines a provider actually runs + # on — measured on macOS: writing `store.git/marker` and reading `STORE.GIT/marker` gives + # the store's own content back. An `==` here therefore admitted the exact collision this + # guard exists to refuse, spelled differently, in all three positions. + # `Path.resolve()` does not normalise case either, so comparing resolved paths would miss + # it too; the comparison has to be on the NAME. raise ValueError( f"{kind} {value!r} is the object store's own directory name, so a workspace built from " f"it would sit on top of this member's whole git history") @@ -733,7 +740,9 @@ def preflight() -> None: permission_mode() _passthrough_env_names() if task_sandbox.enabled(): - task_sandbox.preflight() + # The root is handed over rather than read there: `task_sandbox` cannot import this module + # (it would close a cycle), and this is the one caller that already knows the answer. + task_sandbox.preflight(task_root=workspace_root()) def _require_version_for_the_sandbox(binary: str) -> None: diff --git a/remote/task_evict.py b/remote/task_evict.py index afdc0c8..c4ed573 100644 --- a/remote/task_evict.py +++ b/remote/task_evict.py @@ -147,8 +147,21 @@ def _conversations(root: Path) -> list[tuple[tuple[str, str, str], Path]]: if not projects.is_dir(): return found try: - project_dirs = sorted(projects.iterdir()) + # ⚠️ **Filtered to DIRECTORIES here, not left for `_subdirectories` to trip over** (ND-18). + # A non-directory under `projects/` is not a project and can never hold a candidate, but + # handing one down produced `NotADirectoryError` — a real listing failure as far as that + # function can tell — and with it a warning that this provider's workspace bound "is not + # being fully enforced". On macOS the file is `.DS_Store`, Finder writes it the moment + # anybody opens the folder, and the sweep went on enforcing the cap perfectly over every + # genuine project while saying once per sweep that it had stopped. The impact was nil and + # the sentence was alarming, which is the pair that teaches an operator to ignore it. + # + # Same predicate as `_subdirectories`, symlinks included: a symlinked project directory + # would let the sweep walk — and delete — through a link out of the tree entirely. + project_dirs = sorted(entry for entry in projects.iterdir() + if entry.is_dir() and not entry.is_symlink()) except OSError as exc: + # A failure to list `projects/` ITSELF is still a real one, and still says so. _warn(f"could not list {projects} to bound the provider's workspaces ({exc})") return found for project in project_dirs: diff --git a/remote/task_sandbox.py b/remote/task_sandbox.py index 42bda9a..15e97be 100644 --- a/remote/task_sandbox.py +++ b/remote/task_sandbox.py @@ -236,14 +236,26 @@ def home_directory() -> Path: return home -def preflight() -> None: +def preflight(task_root: Path | None = None) -> None: """Check what `policy()` will need, early enough to be worth reporting. `policy()` runs from `agent_argv`, which is built AFTER the task's checkout — so without this a provider with a broken `HOME` fetches a repository, links a transcript, and only then fails with a message about the task runner. Called from `task_agent.preflight()`. + + `task_root` is passed IN rather than read here, for `_task_root_of`'s reason: `task_agent` + imports this module, so asking it for `workspace_root()` would close a cycle. `None` skips that + one check rather than guessing, which keeps every existing caller honest. + + ⚠️ **The configuration check runs BEFORE the socket proof**, which binds real sockets: an + operator whose `GRID_TASK_ROOT` is in the wrong place should be told that, not made to wait on + an unrelated probe that is about to succeed anyway. """ home_directory() + if task_root is not None: + _refuse_a_path_inside_a_denied_tree( + f"{WORKSPACE_ROOT_HINT}", _resolved(task_root), + [_resolved(home_directory()), _resolved(paths.grid_home())], SystemExit) _prove_the_sandbox_can_bind_its_sockets() @@ -367,6 +379,54 @@ def _read_rule(path: str) -> str: return f"Read(/{path}/**)" +def _denied_ancestor(path: str, denied: list[str]) -> str | None: + """The denied tree `path` sits inside, or `None`. Both sides must already be `_resolved`. + + Equality counts: a workspace that IS a denied directory is inside it for every purpose here. + """ + target = Path(path) + for tree in denied: + parent = Path(tree) + if target == parent or parent in target.parents: + return tree + return None + + +def _refuse_a_path_inside_a_denied_tree(what: str, path: str, denied: list[str], raise_as): + """ND-01 — the two layers disagree about a path under a denied tree, and only one says so. + + `policy` builds two controls over the same paths. The sandbox's `filesystem` block has + `denyRead` **and** `allowRead`, and there the allow really does win: a workspace under a denied + `$HOME` stays readable, and `tests/e2e_agent_sandbox.py` measured that against the real binary. + The `permissions` block has **only** `deny`, because Claude Code's permission layer has no allow + that beats one — so the same workspace is covered by `Read(//$HOME/**)` with nothing to + re-allow it. + + MEASURED 2026-08-20 on Claude Code 2.1.234, two arms, same prompt and same project shape: with + the root inside `$HOME` the `Write` tool returns `is_error: true` and the agent says so in its + own words (*"blocked by a Read deny rule in your permission settings"*), then works around it + with a shell redirect — 5 turns, 24 s. With the root outside, `Write` succeeds first time — + 3 turns, 15 s. **The task reports `completed` either way**, so the whole cost is invisible: a + less capable agent fails a task that should have run, and nothing anywhere says why. + + ⚠️ **Refused rather than repaired, because the repair is not available.** The obvious fix — + drop the ancestor from `denyRead` to make room for the workspace — is only sound for the sandbox + layer. In the `permissions` layer it would hand the in-process `Read` tool the operator's whole + home directory, which is the hole that layer exists to close (measured: without it, a run with + the entire sandbox above still read a file outside the workspace). So the configuration is + refused, loudly, at the two places that can see it. + """ + inside = _denied_ancestor(path, denied) + if inside is None: + return + raise raise_as( + f"{what} {path} is inside {inside}, which the agent sandbox denies. The sandbox itself " + f"would re-allow the workspace, but the permission layer that governs the Write tool has " + f"no allow that beats a deny — so the agent would silently lose Write, work around it, and " + f"still report success. Point {WORKSPACE_ROOT_HINT} at a directory outside {inside} " + f"(for example /Users/Shared/gnd on macOS, or /var/grid on Linux).") + + _WARNED_ABOUT: set[str] = set() @@ -418,6 +478,10 @@ def policy(workspace: Path, config_dir: Path) -> dict: _warn_if_the_path_is_long_enough_to_break_exec(workspace_path) home = home_directory() denied = [_resolved(home), _resolved(paths.grid_home()), _resolved(config_dir)] + # ⚠️ ND-01. The invariant is checked HERE, where the two lists are built and where the + # disagreement between them lives — the argument `task_agent._safe_segment` makes for validating + # a path at the point it is constructed rather than at each caller. + _refuse_a_path_inside_a_denied_tree("the task workspace", workspace_path, denied, ValueError) # The workspace is re-allowed explicitly, and it matters on a dev box: `GRID_TASK_ROOT` may sit # under the home directory that was just denied, and `allowRead` is what takes precedence. allowed = [workspace_path] diff --git a/tests/test_application_surface.py b/tests/test_application_surface.py index 724a7df..fd85e10 100644 --- a/tests/test_application_surface.py +++ b/tests/test_application_surface.py @@ -290,17 +290,47 @@ def _printed_strings(path: pathlib.Path): ⚠️ A string at MODULE level reaches nobody through these sinks and is therefore not walked; the constants that matter (`_MERGE_TURN_LABEL`, the refusal templates) are read inside a function - that prints or raises, so they are covered where they are used. + that prints or raises, so they are covered where they are used — and the tables that are NOT + are named in `_SENTENCE_TABLES`. + + ⚠️ **A LOCAL variable holding a sentence is followed** (ND-06). Half a sentence assigned to a + plain local and interpolated into what the function returns is still prose a person reads, and + without this the walker yielded only the punctuation around it: `cli/remote_task.` \ + `_no_trunk_message` shipped `main` to every new user past a green suite that way. The + resolution is deliberately shallow — one pass over the function's own `name = ` + assignments, no flow analysis — because it only has to see the shape people actually write, and + a local that never reaches a sink is still invisible, which is what keeps a wire value like + `kind = "merge"` out of the report. """ tree = ast.parse(path.read_text()) - def parts(node): + def parts(node, names): if isinstance(node, ast.Constant) and isinstance(node.value, str): yield node.lineno, node.value elif isinstance(node, ast.JoinedStr): for piece in node.values: if isinstance(piece, ast.Constant) and isinstance(piece.value, str): yield node.lineno, piece.value + elif isinstance(piece, ast.FormattedValue) and isinstance(piece.value, ast.Name): + # `f"{head} …"` — the ND-06 shape. + yield from names.get(piece.value.id, ()) + elif isinstance(node, ast.Name): + # `return head`, or `print(head)`. + yield from names.get(node.id, ()) + + def local_sentences(function): + """`name -> [(lineno, value)]` for this function's own string assignments.""" + names: dict[str, list] = {} + for node in ast.walk(function): + if (isinstance(node, ast.Assign) and len(node.targets) == 1 + and isinstance(node.targets[0], ast.Name)): + # Resolved against an EMPTY map, so one local built out of another is not chased. + # Depth is not what this scan is short of, and a fixpoint here would be a flow + # analysis nobody asked for. + collected = list(parts(node.value, {})) + if collected: + names.setdefault(node.targets[0].id, []).extend(collected) + return names for function in ast.walk(tree): if not isinstance(function, (ast.FunctionDef, ast.AsyncFunctionDef)): @@ -311,18 +341,19 @@ def parts(node): # both planes, and a per-module rule exempted `_project_create` — the busiest command on # the application's surface — for years without anybody choosing to. continue + names = local_sentences(function) for node in ast.walk(function): if isinstance(node, ast.Call): name = node.func.id if isinstance(node.func, ast.Name) else getattr( node.func, "attr", "") if name in _USER_FACING_CALLS: for argument in node.args: - yield from parts(argument) + yield from parts(argument, names) elif isinstance(node, ast.Return) and node.value is not None: # A helper that BUILDS a sentence for its caller to print — # `task_diff._nothing_to_show` is the one this exists for, and it is the most # user-facing prose in that module. - yield from parts(node.value) + yield from parts(node.value, names) def test_what_the_application_facing_handlers_print_speaks_no_git(): @@ -348,6 +379,74 @@ def test_what_the_application_facing_handlers_print_speaks_no_git(): + "\n ".join(sorted(found))) +def test_the_printed_string_walker_follows_a_sentence_held_in_a_local_variable(tmp_path): + """The walker must see a literal that reaches a sink through a LOCAL name, not only directly. + + ⚠️ **The third instance of one structural hole**, and the first that a module-level exemption + list could not have caught. `_DEFAULT_WARNINGS` and `_MERGE_TURN_LABEL` were both module-level + constants, and `_SENTENCE_TABLES` below was the answer to them — an explicit list somebody + maintains. That answer cannot reach this one: `cli/remote_task._no_trunk_message` assigned its + first sentence to a plain local `head` and returned `f"{head} …"`, so the literal was an + `ast.Name` inside a `FormattedValue` and the walker yielded only the punctuation around it. + `grid task create` printed `main` at the first wall a new user meets, and this file reported + 18 passed. + + So the walker resolves local string assignments inside the function it is already walking, + which needs no list and no maintenance. The three rows below are the whole contract: + + * the DIRECT case, which already worked — the positive control. Without it a walker that + returned nothing at all would pass the row that matters by finding no offence; + * the LOCAL case, which is the hole; + * a WIRE value in a local that never reaches a sink, which must stay invisible. That is the + bound `_printed_strings`' docstring draws and the reason widening it to every module-level + string was rejected — a scan that reports `_MERGE_KIND = "merge"` can only be kept green by + exempting the two words that matter most. + """ + module = tmp_path / "sample.py" + module.write_text( + "def direct():\n" + " print('the trunk is a direct constant')\n" + "\n" + "def interpolated_from_a_local():\n" + " head = 'this branch came through an f-string'\n" + " return f'{head} and then some'\n" + "\n" + "def printed_from_a_local():\n" + " note = 'this commit came through a bare name'\n" + " print(note)\n" + "\n" + "def returned_from_a_local():\n" + " line = 'this ref came back as a bare name'\n" + " return line\n" + "\n" + "def wire_value_never_printed():\n" + " kind = 'merge'\n" + " return len(kind)\n") + + seen = [value for _, value in _printed_strings(module)] + + assert "the trunk is a direct constant" in seen, ( + "the walker no longer sees a constant handed straight to `print`, so this test's other " + "rows prove nothing — fix the walker before reading them") + assert "this branch came through an f-string" in seen, ( + "a sentence assigned to a local variable and interpolated into a returned f-string is " + "invisible to the walker again (ND-06). `grid task create` shipped `main` past this scan " + "that way") + # ⚠️ The two bare-`Name` shapes get their own rows because a mutation sweep found that the + # f-string row above does not reach them: disabling the bare-`Name` branch left this test + # green. They are also the shapes most likely to be written next — a sentence built over + # several lines and then printed is the ordinary way to write a long one. + assert "this commit came through a bare name" in seen, ( + "`print(local)` is invisible to the walker — the same hole as ND-06 through a different " + "AST shape") + assert "this ref came back as a bare name" in seen, ( + "`return local` is invisible to the walker — the same hole as ND-06 through a different " + "AST shape") + assert "merge" not in seen, ( + "the walker now reports a wire value that reaches nobody — that is the failure mode which " + "makes this scan unusable, not a stricter version of it") + + # Module-level sentence TABLES on the application's surface, as `(module, attribute)`. `_printed_ # strings` walks the SINKS — what is handed to `print`/`SystemExit`/`TaskRefusal` — and collects only # the `Constant` pieces of an f-string, so a sentence reached through a `Name` subscript diff --git a/tests/test_local_cli.py b/tests/test_local_cli.py index b8aec6c..4ec9bb4 100644 --- a/tests/test_local_cli.py +++ b/tests/test_local_cli.py @@ -23906,6 +23906,82 @@ def test_task_create_without_a_project_explains_an_old_relay_too(monkeypatch, tm assert "relay" in str(caught.value).lower() +def test_task_create_with_a_project_does_not_leak_the_bare_framework_404(monkeypatch, tmp_path): + """ND-13. The one command in the C4 drill that handed the user FastAPI's two words. + + The sibling test above passes for a reason that does not cover this: without `--project`, the + command resolves a project first, and that call carries `_OLD_RELAY`. Name the project and the + resolution is skipped, so `POST /relay/v1/tasks` is the first request — and it had no hint, so + the user's entire diagnosis was `Not Found`. Thirteen of the fourteen commands in that drill + translated the same 404 into a sentence. + + ⚠️ **The reason it had no hint is sound, and the fix must not undo it.** `/relay/v1/tasks` + exists on relays predating the whole project plane, so a 404 HERE cannot mean "your relay is + old" — an old relay answers 201 and quietly puts the task in the caller's `default`, which is + what the echoed-`project_id` guard above catches. A 404 from a route every relay has means the + address being spoken to is not a relay at all: a proxy in front of it, or a wrong base URL. + So the sentence must say THAT, and pointing at "update your relay" would send somebody to + upgrade a server that is working. + """ + _seed_running_remote_grid(monkeypatch, tmp_path) + state.set_mode("remote") + + seen = [] + + def handler(request): + seen.append(request.url.path) + return httpx.Response(404, json={"detail": "Not Found"}) + + _mock_relay(monkeypatch, handler) + + with pytest.raises(SystemExit) as caught: + cli.main(["task", "create", "--project", "P1", "--prompt", "x"]) + + message = str(caught.value) + # The positive control: the request under test is the one that failed. Without it a command + # that refused before ever calling the relay satisfies every assertion below. + assert seen and seen[-1] == "/relay/v1/tasks", ( + f"the 404 under test did not come from the task route: {seen}") + assert message.strip() != "Not Found", ( + "the bare framework 404 reached the user unexplained (ND-13)") + assert "relay" in message.lower(), f"the sentence does not name the relay: {message}" + # ⚠️ Not the old-relay sentence. This route is not missing on an old relay, and a message + # that said so would send a user to upgrade a server that is answering correctly. + assert "predates" not in message.lower() and "update it" not in message.lower(), ( + f"a 404 here was diagnosed as an out-of-date relay, which it cannot be: {message}") + + +def test_a_real_404_from_the_task_route_is_not_masked_by_the_new_hint(monkeypatch, tmp_path): + """The other half of ND-13, and the half a new guard is most likely to get wrong. + + `_NOT_THE_RELAY` says something quite specific and quite alarming — *what is answering there + is probably not the relay* — so it must fire only for the bare framework 404 it was written + for. A relay that DOES have the route and is refusing something real (an unknown project, a + project the caller cannot reach) answers 404 with its own words, and swallowing those to + announce a misconfigured proxy would send the user to debug their network over a refusal that + was already explained. + + Keyed on the DETAIL being exactly FastAPI's `"Not Found"` rather than on the status, which is + the same test `_task_oneshot` applies for the other thirteen hints. + """ + _seed_running_remote_grid(monkeypatch, tmp_path) + state.set_mode("remote") + + _mock_relay(monkeypatch, lambda r: httpx.Response(404, json={"detail": { + "code": "no_such_project", + "message": "There is no project P1 you can reach on this grid."}})) + + with pytest.raises(SystemExit) as caught: + cli.main(["task", "create", "--project", "P1", "--prompt", "x"]) + + message = str(caught.value) + assert "no project P1" in message, ( + f"the relay's own refusal was replaced by the missing-route hint: {message}") + assert "not the relay" not in message, ( + f"a real refusal was diagnosed as a misconfigured proxy (ND-13's guard overreaching): " + f"{message}") + + def test_a_real_404_from_the_projects_route_is_not_masked(monkeypatch, tmp_path): """The other half, and the reason the hint is keyed on the BARE detail: a relay that does know the route and answers 404 about the project itself must have its own words shown, not be @@ -33062,6 +33138,62 @@ def handler(request): assert "t-1" in out and "cancelled" in out.lower() +def test_task_cancel_does_not_promise_the_agents_work_survived(monkeypatch, tmp_path, capsys): + """ND-02. `cancel` said the work was kept, and at the moment it says so nobody knows that. + + Measured on a live grid 2026-08-20: cancel a running turn, then `grid task fetch` it, and the + tree that comes back is the task's **input** — the agent's edits are not in it. `fetch` was + already honest about this ("recorded no result … it may hold only the task's input"); this + line was the half that still over-promised, and the two were read one after the other by the + same person in the same minute. + + The promise cannot be made conditional at this end either: `cancel` returns as soon as the + relay records it and the agent does not stop until the next lease beat, so what will finally + be recorded is unknown here. That leaves saying so — which is what the assertion pins. + + The `fetch` pointer stays. It is the only way to find out which of the two you got, and + deleting it to fix the honesty would take away the answer along with the wrong promise. + """ + _seed_running_remote_grid(monkeypatch, tmp_path) + state.set_mode("remote") + + def handler(request): + return httpx.Response(200, json={ + "id": "t-1", "project_id": "P1", "state": "failed", "error": "cancelled", + "prompt": "fix the parser", "member_key": "def456"}) + + _mock_relay(monkeypatch, handler) + rc = cli.main(["task", "cancel", "t-1"]) + out = capsys.readouterr().out + + assert rc == 0 + # The positive control: the line under test is actually being read. Without it every + # assertion below passes just as well against a command that printed nothing at all. + assert "grid task fetch t-1" in out, ( + f"cancel no longer points at fetch, so this test is checking an absent sentence:\n{out}") + assert "already done is kept" not in out, ( + f"cancel is promising the agent's work survived again (ND-02) — it does not know that " + f"when it prints:\n{out}") + lowered = out.lower() + assert not ("is kept" in lowered or "left where the agent got to" in lowered), ( + f"cancel found another way to promise the work survived:\n{out}") + + # ⚠️ **The `--help` is the other copy of this promise, and the first report of ND-02 was + # against the help rather than the printed line.** They were written apart and fixed apart + # once already — issue 46 reworded the parser's one-line description and left the epilogue + # standing — so both are pinned here, in one test, where a future edit to either is measured + # against the same rule. + parser = cli.build_parser() + cancel = parser._subparsers._group_actions[0].choices["task"] \ + ._subparsers._group_actions[0].choices["cancel"] + help_text = cancel.format_help() + assert "grid task fetch" in help_text, ( + f"the cancel help no longer mentions fetch, so the check below reads nothing:\n{help_text}") + assert "already done is kept" not in help_text and "is kept" not in help_text.lower(), ( + f"the cancel --help promises the agent's work survived (ND-02); it does not know that " + f"when it is written, let alone when it is read:\n{help_text}") + + def test_task_cancel_refuses_a_reply_it_cannot_read(monkeypatch, tmp_path): """The rule every sibling in this plane follows since 19a's review: an answer this command cannot read is NOT a successful cancellation. Reporting one would tell somebody their colleague's diff --git a/tests/test_task_agent.py b/tests/test_task_agent.py index fab3463..987a568 100644 --- a/tests/test_task_agent.py +++ b/tests/test_task_agent.py @@ -120,6 +120,16 @@ def test_a_member_key_is_accepted_as_a_path_segment(monkeypatch, tmp_path): # not the constant, because nothing from `remote/` is imported at this module's scope; the two # are pinned to each other by the test below. "store.git", + # The SAME directory on a case-insensitive filesystem (ND-11), which is what APFS and NTFS are + # by default — and what the provider fleet's macOS boxes run. Measured on this machine: create + # `store.git/marker`, then `cat STORE.GIT/marker`, and the store's own content comes back. The + # guard compared with `==`, so this spelling was accepted in all three positions and a + # workspace built from it would sit on the member's whole history — the exact disaster the + # row above exists to stop, reachable through a different capitalisation of it. + # ⚠️ `Path.resolve()` does not normalise case either, so comparing resolved paths misses it + # too; casefold is the tool. + "STORE.GIT", + "Store.Git", ] @@ -135,6 +145,37 @@ def test_the_reserved_segment_is_the_object_stores_own_directory_name(): assert task_worktree.STORE_DIR_NAME in _HOSTILE_SEGMENTS +def test_a_differently_cased_store_name_is_the_same_directory_where_the_provider_runs(tmp_path): + """Why the rows above are not paranoia: the collision is a property of the FILESYSTEM (ND-11). + + The guard refuses one NAME, and a name only means a directory once a filesystem has resolved + it. APFS and NTFS are case-INsensitive by default, so `STORE.GIT` and `store.git` are one + directory there — and that is what the provider fleet's macOS boxes run. + + Measured rather than assumed, and it reports which world it is in instead of skipping: on a + case-sensitive filesystem the two names really are two directories and there is nothing to + prove locally, but the guard still has to hold, because the machine that runs a provider is + not the machine that runs this suite. Either way the assertion below is the same. + + ⚠️ `Path.resolve()` does NOT normalise case, so a resolved-path comparison misses this too. + """ + from remote import task_worktree + + store = tmp_path / task_worktree.STORE_DIR_NAME + store.mkdir() + (store / "marker").write_text("the member's whole history") + shouted = tmp_path / task_worktree.STORE_DIR_NAME.upper() + + case_insensitive = (shouted / "marker").exists() + if case_insensitive: + assert (shouted / "marker").read_text() == "the member's whole history", ( + "the filesystem resolved both spellings to one directory but handed back different " + "content, which is neither of the two worlds this test knows how to reason about") + assert store.resolve() != shouted.resolve(), ( + "`resolve()` started normalising case, so the note above is stale — re-derive whether " + "casefold is still the right tool before trusting this test") + + @pytest.mark.parametrize("hostile", _HOSTILE_SEGMENTS) @pytest.mark.parametrize("position", ["project id", "member key", "conversation id"]) def test_a_hostile_path_segment_is_refused_before_anything_is_created( diff --git a/tests/test_task_evict.py b/tests/test_task_evict.py index 93d6d20..e20f106 100644 --- a/tests/test_task_evict.py +++ b/tests/test_task_evict.py @@ -114,6 +114,48 @@ def test_a_provider_over_its_workspace_cap_evicts_the_least_recently_used_and_ke "project — the cost this layout exists to remove") +def test_a_stray_file_beside_the_projects_does_not_warn_that_the_bound_has_stopped( + tmp_path, short_task_root, monkeypatch, capsys): + """ND-18. A `.DS_Store` under `projects/` made every sweep on every macOS provider say the + provider's workspace bound "is not being fully enforced". + + The warning was TRUE of the entry it named and false of everything a reader takes it to mean. + `_conversations` handed each child of `projects/` to `_subdirectories`, which listed it, got + `NotADirectoryError` for a file, and reported the one thing it is right to report when a real + listing fails. But a file there is not a failure — it is Finder, on every Mac, without anybody + choosing it — and the sweep went on to enforce the cap correctly over every genuine project. + So the impact was nil and the sentence said otherwise, once per sweep, forever. + + The fix is to stop asking: a non-directory is not a project and never was a candidate, so it + is filtered where the children are listed. The warning then keeps its meaning for the case it + was written for — which the sibling test below still holds it to. + + Both halves are asserted, and the eviction half is the positive control: a test that only + checked stderr would pass just as well against a sweep that had silently stopped working. + """ + from remote import task_evict + + monkeypatch.setenv(task_agent_root_env(), str(short_task_root)) + monkeypatch.setenv(task_evict.MAX_WORKSPACES_ENV, "1") + for conversation in _CONVERSATIONS[:2]: + _real_workspace(tmp_path, short_task_root, conversation) + # Exactly what Finder leaves behind the moment somebody opens the folder. + (short_task_root / "projects" / ".DS_Store").write_bytes(b"\x00\x00\x00\x01Bud1") + capsys.readouterr() + + task_evict.sweep(short_task_root, keep=None, + reserve=_never_reserved, release=_released) + + err = capsys.readouterr().err + assert _conversation_dirs(short_task_root) == [_CONVERSATIONS[1]], ( + "the cap stopped being enforced, so the stderr assertion below proves nothing") + assert ".DS_Store" not in err, ( + f"a stray file is still reported as a directory the sweep could not read:\n{err}") + assert "not being fully enforced" not in err, ( + f"the sweep told the operator its workspace bound had stopped working, while enforcing " + f"it correctly in the same call (ND-18):\n{err}") + + def test_eviction_skips_a_workspace_a_worker_is_holding_rather_than_waiting_for_it( tmp_path, short_task_root, monkeypatch): """Criterion 3, and the mechanism matters as much as the outcome. diff --git a/tests/test_task_sandbox.py b/tests/test_task_sandbox.py index 67fcff4..527d5c0 100644 --- a/tests/test_task_sandbox.py +++ b/tests/test_task_sandbox.py @@ -208,23 +208,32 @@ def test_the_temp_directory_is_writable_and_not_merely_readable(monkeypatch, tmp assert expected in filesystem["allowWrite"] -def test_the_workspace_stays_readable_even_when_it_sits_inside_the_denied_home(monkeypatch, - tmp_path): - """A dev box points `GRID_TASK_ROOT` at a directory under `$HOME`, which is denied wholesale. - - `allowRead` is documented to take precedence over `denyRead`, so the workspace is re-allowed by - name and the agent can still read the repository it was asked to work on. Without this the - confinement would be perfect and every task would fail. +def test_a_workspace_under_the_denied_home_is_no_longer_a_supported_layout(monkeypatch, tmp_path): + """OVERTURNED 2026-08-20 (ND-01). Kept, rewritten, rather than deleted. + + This test used to assert that a workspace under a denied `$HOME` *"stays readable"*, on the + documented ground that `allowRead` takes precedence over `denyRead`. That ground is still true + **of the sandbox layer** — `test_build_caches_survive_the_denied_home` below is the live proof, + since every build cache it checks sits inside the same denied `$HOME` — and + `tests/e2e_agent_sandbox.py` measures it against the real binary. + + What the old test did not cover is the SECOND layer. `permissions.deny` governs the in-process + tools and has no allow that beats it, so under that layout the `Write` tool is blocked while + Bash is not: measured on Claude Code 2.1.234, `Write` returns `is_error: true`, the agent works + around it with a shell redirect, and the task still reports `completed`. The old docstring's + *"without this the confinement would be perfect and every task would fail"* was therefore half + right — nothing failed, it just quietly cost more turns and depended on the agent being clever. + + So the layout is refused now, and this test says so at the address the old assertion lived at, + so nobody re-derives the old rule from a gap in the file. The refusal's own reasoning is in + `test_a_workspace_inside_a_denied_tree_is_refused_rather_than_half_confined`. """ from remote import task_sandbox workspace = Path.home() / ".grid-test-workspace" / "projects" / "p" / "workspace" - sandbox = task_sandbox.policy(workspace, tmp_path / "cfg")["sandbox"] - - assert str(Path.home().resolve()) in sandbox["filesystem"]["denyRead"] - assert str(workspace.resolve()) in sandbox["filesystem"]["allowRead"] - assert str(workspace.resolve()) in sandbox["filesystem"]["allowWrite"] + with pytest.raises(ValueError): + task_sandbox.policy(workspace, tmp_path / "cfg") def test_build_caches_survive_the_denied_home(tmp_path, config_dir): @@ -707,3 +716,79 @@ def test_temp_base_reads_what_the_child_will_actually_get(monkeypatch): monkeypatch.delenv("TMPDIR", raising=False) assert task_sandbox.temp_base() == Path("/tmp") + + +def test_a_workspace_inside_a_denied_tree_is_refused_rather_than_half_confined(tmp_path, + config_dir, + monkeypatch): + """ND-01, measured live 2026-08-20 on Claude Code 2.1.234: the two layers disagree, silently. + + `policy` builds TWO controls over the same paths. The sandbox's `filesystem` block has both + `denyRead` and `allowRead`, and there `allowRead` really does win — a workspace under a denied + `$HOME` stays readable, which is what the sibling test above measured. The `permissions` block + has **only** `deny`, because Claude Code's permission layer has no allow that beats a deny — so + the same workspace is covered by `Read(//$HOME/**)` with nothing to re-allow it. + + The result is not a confinement failure, it is a CAPABILITY failure that reports success: the + `Write` tool comes back `is_error: true` (*"blocked by a Read deny rule in your permission + settings"*), the agent works around it with a shell redirect, the task still says `completed`, + and the only trace is extra turns. Measured: 5 turns / 24 s with the root inside `$HOME`, + 3 turns / 15 s with it outside. + + Refused here rather than repaired, because the repair is not available: dropping `$HOME` from + the deny list to make room for the workspace would hand the in-process `Read` tool the whole + home directory, which is the hole the second layer exists to close. + """ + from remote import task_sandbox + + inside = Path.home() / "gnd" / "projects" / "p" / "m" / "c" / "workspace" + + with pytest.raises(ValueError) as refusal: + task_sandbox.policy(inside, config_dir) + + message = str(refusal.value) + assert str(Path.home().resolve()) in message, message + assert task_agent_env_name() in message, message + + +def test_a_workspace_outside_every_denied_tree_still_builds_a_policy(tmp_path, config_dir): + """The control for the refusal above: the ordinary layout must be untouched by it.""" + from remote import task_sandbox + + outside = tmp_path / "gnd" / "projects" / "p" / "m" / "c" / "workspace" + + policy = task_sandbox.policy(outside, config_dir) + + assert str(outside.resolve()) in policy["sandbox"]["filesystem"]["allowRead"] + + +def test_preflight_refuses_a_task_root_inside_a_denied_tree_before_any_task_is_fetched(monkeypatch, + tmp_path): + """The same refusal, moved to where an operator can act on it. + + `policy` runs from `agent_argv`, which is built AFTER the checkout — so the `policy` guard alone + reports a configuration error on a task that has already fetched a repository. `preflight` is + called from the guarded pre-spawn block, once, before any of that. + """ + from remote import task_sandbox + + with pytest.raises(SystemExit) as refusal: + task_sandbox.preflight(task_root=Path.home() / "gnd") + + assert str(Path.home().resolve()) in str(refusal.value) + + +def test_preflight_still_passes_for_a_task_root_outside_the_home(monkeypatch, tmp_path): + """The control: preflight must not start refusing every provider.""" + from remote import task_sandbox + + monkeypatch.setattr(task_sandbox, "_prove_the_sandbox_can_bind_its_sockets", lambda: None) + + task_sandbox.preflight(task_root=tmp_path / "gnd") + + +def task_agent_env_name() -> str: + """The variable an operator would change, named without importing `task_agent` into the policy.""" + from remote import task_sandbox + + return task_sandbox.WORKSPACE_ROOT_HINT