Provider onboarding, the rest of it: issues 59–62, and two tests that could not fail - #57
Merged
Conversation
The four tests for `grid join`'s task check each patched `preflight_before_serving`, and that function's own unit tests patched `preflight` and `resolve_binary`. So the chain cmd_remote_join → preflight_before_serving → preflight had never executed as one thing: every test named the seam, none crossed it. `test_the_join_runs_the_REAL_check_not_a_stand_in` patches nothing but the grid and the spawn. It reaches the refusal through GRID_TASK_PERMISSION_MODE deliberately — the first thing `preflight()` checks, needing no subprocess and no Claude Code on the box, so it asserts the same thing on a developer's Mac and on a Linux runner with no agent installed at all. Driving that chain by hand for the first time caught something no assertion would have: the workspace-root refusal ended without a full stop and ran straight into the "Inference is unaffected." line printed after it. Both sentences in `_require_a_reachable_workspace_root` now end in one. Mutation-proved: dropping `preflight()` from `preflight_before_serving` turns the new test red. 3149 passed, 9 skipped.
`tests/e2e_agent_settings.py` hand-builds the claim it feeds `run_task`, and that payload had not moved since the module was written: no `member_key`, no `conversation_id`. Both became REFUSALS afterwards — `member_key` (ADR 0033 issue 11) is fail-closed by design, so every one of the seven checks failed terminally with "the relay's claim named no member_key", for a reason that has nothing to do with what the module tests. Nobody noticed because the module is `e2e_*`, so `pytest tests/` never collects it. A security-boundary test that is neither collected by the ordinary run nor able to reach its subject is proving nothing twice over. With the claim fixed, on Claude Code 2.1.241 (the measurements in the docstring were taken on 2.1.223): 6 passed, 1 failed in 34.8s. The failure is a CONTROL — `test_the_control_proves_the_mcp_server_would_ otherwise_start`. Stripping `--strict-mcp-config` no longer starts the workspace's `.mcp.json` server, so the guarded test beside it now demonstrates nothing. Measured down to the cause, and it is NOT the vendor: same argv, same cwd, full shell environment -> server starts same argv, same cwd, `child_env` allowlist -> server does not start Not `--settings` either (tested with the real 2179-char payload), and not the sandbox (control still fails with GRID_TASK_SANDBOX=0). Issue 23's environment allowlist independently prevents it. Reassuring for the boundary — two mechanisms, one of them working — and fatal for the pair, which can only isolate the flag if the two runs differ by the flag alone. Left failing rather than deleted, per the module's own instruction.
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. --tasks, --max-tasks N and --tasks-root PATH join the existing remote-only group: a task is claimed from the relay, and local mode has no relay. The flags SET the child's environment rather than moving the reading into the run record, because the opt-in is read at serve time deliberately; this adds a second way to set it, not a second place to read it. Flag over variable, said in --help. --max-tasks REFUSES a bad value instead of defaulting, which is the opposite of GRID_MAX_TASKS's rule and deliberately so: a variable that refused would take task serving down for the life of a running process, while a flag the operator typed a second ago costs one retry. The issue-58 withholding is merged last, so a provider that cannot run a task is never told to claim one by a flag, however explicitly typed. Two traps, each with its own control: - All three default to None, --tasks included. `_reject_remote_only_flags` asks `is not None`, so a store_true defaulting to False would refuse every LOCAL join. Mutation-proved: only the new control catches it; the existing remote-only test stays green. - The preflight reads os.environ because that is what the child reads, so a --tasks-root this process never had reaches it only through the scoped context manager. Its test runs BOTH directions — with the manager mutated away, the env-bad/flag-good row passes for the wrong reason. 3166 passed, 9 skipped.
Two paths turned `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". The record now carries what the child concluded: `tasks` and `max_tasks`, produced by asking `task_opt_in` under the environment the child is handed, so it cannot drift from what the child itself decides. It records what the child GETS, not what was asked for — issue 58 withholds the opt-in from a provider that cannot run a task, and recording the request would report task serving as on for one deliberately told not to claim. Absent is UNKNOWN, never off: `isinstance(recorded, bool)`, because every record written before this carries no such key and reading it as False would tell every provider already serving tasks that it is not. `bool` is checked before `int` for the count, since True is an int. The worker count is reported too rather than deferred — GRID_MAX_TASKS=4 against a live identity is exactly as inert. Nothing respawns implicitly: the in-flight requests a restart would drop are the operator's call, so the sentence names `grid join --respawn` instead. Both properties mutation-proved. docs/cli.md covers this and issue 61's flags. 3175 passed, 9 skipped.
…le retires Two things that only make sense together. THE RULE. Five places said every provider in a grid must run tasks at an identical absolute path. That was true of the design it described, and issue 06 replaced that design in the same ADR: the transcript lives in the git worktree and travels in the result commit, so each provider plants its own per-cwd symlink and finds the same conversation. Issue 35's measurement 5 exists to put exactly that question — materialized at a DIFFERENT absolute path, and resumed. All five corrected; ADR 0032 carries an amendment rather than a deletion, because the record of why it stopped being true is what stops it coming back. THE ROOT. The default is now 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. Both flatten well under the transcript-name limit (144 and 143 of 200). `--tasks` creates it, 0700, and judges a pre-existing one. The property is "this provider can write it and no other account can read it" — phrased that way, not as an ownership test, because /var/grid created once with sudo and handed to a non-root provider is legitimate; what disqualifies it is being world-readable, and the refusal names the chmod that fixes it. mkdir is not exist_ok, so an account that wins the race against the sticky world-writable parent is judged, not adopted. These rules are for the default NOBODY CHOSE. A root named by --tasks-root or GRID_TASK_ROOT is the operator's business, exactly as ensure_workspace has always had it — and ensure_workspace itself is unchanged, with both of its allowances now pinned by tests. Found while doing it: three of issue 61's tests created /Users/Shared/grid on the developer's machine and would have failed on a Linux runner. All three now pin the constant at tmp_path; the control is a before/after of the real path across a full run. 3187 passed, 9 skipped.
Both packages are a hard requirement for the Linux sandbox, a stock provider VM has neither, and nothing in this tree checked: `bwrap`, `bubblewrap` and `socat` appeared in the source only inside comments. The single enforcement was Claude Code's own failIfUnavailable, which fires inside the child — after the claim, after the repository was fetched, on a member's task. The probe joins `preflight_before_serving`, so the answer arrives at `grid join`. Deliberately NOT added to the claim path: the vendor already fails loudly there with a clear message, and the gap was join time. Two packages, not one — measured on Ubuntu 24.04 against 2.1.223, with bwrap present and socat absent the run still refuses. They are named the way the vendor names them, so an operator meeting both messages reads about one thing. Linux only and sandbox-on only: macOS needs neither, and an operator who turned the sandbox off gets the provider that existed before it. Verified on the dev VM (Ubuntu 24.04.3): the exact predicate finds /usr/bin/bwrap and /usr/bin/socat, and reports both missing off PATH. No package uninstalled on a shared box, and no agent run on it. 3193 passed, 9 skipped.
kelvin1295
changed the title
CI caught it. The bubblewrap/socat probe is Linux-only and the CI runner has
neither package, so it raised inside every test that drives
`preflight_before_serving` for some OTHER reason — swamping the assertion
each of them actually makes. Green on a Mac, red on Linux: the same class I
had just fixed for /Users/Shared/grid, reintroduced one commit later by the
check I added to prevent a different silent failure.
`_sandbox_packages_present` answers the probe as a box that has both, and
every test that is not ABOUT the packages now says so out loud. Issue 59's
own tests set them explicitly and are untouched.
Verified by reproducing the CI condition locally rather than by trusting CI
a second time: a pytest plugin that reports sys.platform as linux and makes
shutil.which return None for both binaries. Under it, 2307 passed across
both suites. And the reproduction was proved to bite — removing the helper
from one test gives:
macOS 1 passed
simulated CI 1 failed
which is the entire failure class in two lines.
3193 passed, 9 skipped.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What this is
Six commits finishing the provider-onboarding work #56 started. Four are the remaining issues
(59–62); two are defects in the tests themselves, both found by running things rather than reading
them.
a762a7cb13f036ff768b4grid join --tasksis the surface of task serving935993ca6d9045--tasksmakes its own root, and the identical-path rule retires297100abubblewraporsocatThe two test defects
The join's task check was mocked on both sides. Its four CLI tests patched
preflight_before_serving; that function's own tests patchedpreflightandresolve_binary. Everytest named the seam, none crossed it. The new one patches nothing but the grid and the spawn, and
reaches the refusal through
GRID_TASK_PERMISSION_MODEdeliberately — the first thingpreflight()checks, needing no subprocess and no Claude Code on the box, so it asserts the same thing here and
on a Linux runner with no agent installed.
tests/e2e_agent_settings.pycould not reach its own subject. It hand-builds the claim it feedsrun_task, and that payload had not moved since the module was written: nomember_key, noconversation_id. Both became refusals afterwards, so all seven checks — a security boundary —failed terminally for an unrelated reason. Nobody noticed because the module is
e2e_*, whichpytest tests/never collects.With the claim fixed, on Claude Code 2.1.241 (the docstring's measurements were taken on
2.1.223): 6 passed, 1 failed. The failure is a control, left failing on purpose per the module's
own instruction. Measured down to the cause, and it is not the vendor:
Not
--settings(tested with the real 2179-byte payload), not the sandbox, notPATH. Issue 23'senvironment allowlist independently prevents it — reassuring for the boundary, fatal for the pair,
which can only isolate the flag if the two runs differ by the flag alone.
Issue 62, and the rule it retires
Five places said every provider in a grid must run tasks at an identical absolute path. That was
true of the design it described, and issue 06 replaced that design in the same ADR: the
transcript lives in the git worktree and travels in the result commit, so each provider plants its
own per-cwd symlink and finds the same conversation. Issue 35's measurement 5 exists to put exactly
that question — materialized at a different absolute path, and resumed.
All five corrected. ADR 0032 carries an amendment, not a deletion — the record of why it stopped
being true is what stops it coming back.
So the default root is now per-platform:
/var/gridon Linux,/Users/Shared/gridon macOS, where/varis root-owned and a provider withoutsudofails every task on it. Both flatten to 143/144against a 200-character transcript-name limit.
--taskscreates it0700and judges a pre-existing one. The property is this provider can writeit and no other account can read it — phrased that way rather than as an ownership test, because
/var/gridcreated once withsudoand handed to a non-root provider is legitimate; whatdisqualifies it is being world-readable, and the refusal names the
chmodthat fixes it. Theserules are for the default nobody chose — a root named by
--tasks-rootorGRID_TASK_ROOTisthe operator's business, and
ensure_workspaceis unchanged with both of its allowances now pinned.A defect this found in my own tests
Three of issue 61's tests passed
--taskswith no root named, so they called the realensure_default_workspace_rootand created/Users/Shared/gridon the developer's machine. Ona Linux runner the same tests would have failed, because
/varis not writable — green locally, redin CI. All three now pin the constant at
tmp_path; the control is a before/after of the real pathacross a full run of both suites.
Test plan
.venv/bin/pytest tests/— 3193 passed, 9 skipped.venv/bin/ruff check .— cleansocatrow the obvious way left a flat tuple, so the run died on a
ValueErrorduring unpacking —killed, but by a syntax fault rather than the fault it was meant to test
/usr/bin/bwrapand/usr/bin/socat, and reports both missing offPATH. No packageuninstalled on a shared box, and no agent run on it
0700, adopted on a second call,and a
chmod 755of it is refused namingchmod 700/Users/Shared/gridfix — it runs on Linux, which is where thosethree tests would have failed