diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index c8d12d6..341e804 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -67,10 +67,31 @@ jobs: # and therefore passes or fails on the time of day, not on the code. It # is deselected here rather than deleted so that it still runs locally, # where a human can tell the difference. + # + # `tests/property` runs in here with everything else -- it is inside + # `tests/` and needs no separate invocation. What it does *not* get from + # this leg is a different seed, which is the next step's job. run: | python -m pytest -q \ --deselect tests/test_wakeup.py::test_the_deadline_is_reported_as_a_deadline + - name: Generated tests, with seeds a developer never runs + shell: bash + # The local profile is `derandomize=True` on purpose: a property suite + # that fails one time in five and passes when you re-run it teaches + # people to re-run it. That makes every local run explore the *same* + # examples, which is exactly the wrong trade for CI -- so this leg turns + # randomisation back on and raises the count, and a rare counterexample + # surfaces on a pull request rather than never. + # + # Separate from the step above rather than folded into it, because a + # failure here means something different: not "this push broke a rule" + # but "a rule was already breakable and this seed found it". Both are + # worth failing on; only one of them is a regression. + env: + HYPOTHESIS_PROFILE: ci + run: python -m pytest tests/property -q + # -------------------------------------------------------------------------- # What a user actually installs # -------------------------------------------------------------------------- diff --git a/.gitignore b/.gitignore index 0cad764..a7a40c7 100644 --- a/.gitignore +++ b/.gitignore @@ -4,6 +4,20 @@ __pycache__/ venv/ *.egg-info/ .pytest_cache/ +.ruff_cache/ + +# Hypothesis's example database: the counterexamples it has found, so a +# property that failed once is re-checked with that input first on the next run. +# Local and machine-specific -- the properties themselves are the artefact, and +# `derandomize` is what makes a run reproducible without shipping this. +# `.hypothesis/patches/` is the "here is the failing case as a diff" offer, +# which is a suggestion rather than a result. +.hypothesis/ + +# mutmut copies the whole source tree here and runs pytest in it several hundred +# times. Regenerated by `mutmut run`, and large. +mutants/ +.mutmut-cache # Build outputs. Nothing here builds a wheel in the normal course of things -- # the install is editable and `grad --update` moves the checkout rather than # reinstalling from an artifact -- but the packaging metadata is only really diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index 762c917..bd808c9 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -75,6 +75,15 @@ What the CI checks, and why it is shaped the way it is: resolves whether or not the wheel contains it. If you change `[tool.setuptools]`, that job is the one to watch. +There is a second suite inside the first. `tests/property/` generates its inputs +with Hypothesis instead of listing them, and it is where a rule goes when the +examples keep running out — `hooks._segments` had four bugs in four commits and a +fifth that no example had reached. It runs with everything else and takes about a +second; `HYPOTHESIS_PROFILE=deep` turns it up when you have just changed one of +the modules it covers. Mutation testing is configured too, and is not in CI. +Both are in [`docs/testing.md`](docs/testing.md), including the two ways a +generated test can silently pass on the previous example's leftovers. + Some conventions worth knowing before a larger change: - **Capability is a CLI, not a framework.** New agent-facing capability is a diff --git a/README.md b/README.md index cf28c4e..157769a 100644 --- a/README.md +++ b/README.md @@ -290,15 +290,24 @@ shapes of the paths that have run, and they fail with actionable errors rather than tracebacks — but a real credential and a real run are what find the mismatches. -The test suite is 56 files and runs offline; the network is stubbed by a fixture, -because a suite that reaches the network does not fail, it hangs. The gate tests -run against a real ledger in a temporary workspace, since a mock of a gate proves -nothing about the gate. +The test suite runs offline; the network is stubbed by a fixture, because a suite +that reaches the network does not fail, it hangs. The gate tests run against a +real ledger in a temporary workspace, since a mock of a gate proves nothing about +the gate. ```bash python -m pytest -q ``` +Alongside the example-based tests, `tests/property/` generates inputs and checks +rules rather than outputs — a mean lies between the extremes it was taken over, a +rolling spend never falls when a run is submitted, and if the shell would run +`ssh` then the deny list says so. That last one found three bypasses on its first +run, including `( ssh gpu-box nvidia-smi )`: three tokens, no quoting, and the +shortest hole the hook ever had. Mutation testing (`mutmut`) is configured for +the same modules and run by hand rather than in CI. Both are described in +[`docs/testing.md`](docs/testing.md). + Three things to know before trusting it with a budget: - **Interfaces are not stable.** Ledger fields, exit codes and CLI flags still diff --git a/core/stats.py b/core/stats.py index c4d8a2c..4316e93 100644 --- a/core/stats.py +++ b/core/stats.py @@ -96,7 +96,25 @@ def summarise(values: Sequence[Any]) -> dict[str, Any]: if n == 0: return {"n": 0, "mean": None, "sd": None, "sem": None, "ci95": None, "min": None, "max": None, "samples": []} - mean = math.fsum(samples) / n + low, high = min(samples), max(samples) + # Clamped into the range it was taken over, which the true mean of any set + # of reals is always inside. `fsum` gives the exactly-rounded sum and the + # division then rounds once more, and that last rounding can land outside -- + # so `[3.05, 3.05, 3.05]` reported a mean of 3.0499999999999994, below its + # own minimum, and a standard deviation of 5e-16 for three runs that agreed + # exactly. + # + # Both halves of that matter and the second one more. This module exists + # because "`val_loss = 3.05` against a predicted `[2.9, 3.2]` was recorded as + # in-range with identical confidence whether the run-to-run spread was 0.001 + # or 0.3" -- so a spread of *zero*, which is what three identical seeds + # measured, is precisely the reading it must not get wrong. Clamping fixes + # the deviation too rather than only the mean: with the mean exact, every + # `x - mean` is exactly 0 and the variance is exactly 0. + # + # The correction is never more than one unit in the last place. Found by + # `tests/property/test_prop_stats.py`, which asserts the identity directly. + mean = min(high, max(low, math.fsum(samples) / n)) if n == 1: return {"n": 1, "mean": mean, "sd": None, "sem": None, "ci95": None, "min": samples[0], "max": samples[0], "samples": samples} @@ -112,8 +130,8 @@ def summarise(values: Sequence[Any]) -> dict[str, Any]: "sd": sd, "sem": sem, "ci95": [mean - half, mean + half], - "min": min(samples), - "max": max(samples), + "min": low, + "max": high, "samples": samples, } diff --git a/docs/testing.md b/docs/testing.md new file mode 100644 index 0000000..cf93559 --- /dev/null +++ b/docs/testing.md @@ -0,0 +1,130 @@ +# Testing + +Three suites, answering three different questions. They are not tiers of the +same thing and none of them replaces another. + +| Suite | Question | Cost | Runs in CI | +| --- | --- | --- | --- | +| `tests/*.py` | does this input produce that output? | ~3 min | yes, every push | +| `tests/property/` | is there *any* input that breaks the rule? | ~5 s | yes, every push | +| `mutmut` | would any test notice if this line were wrong? | CPU-hours | no, by hand | + +```bash +python -m pytest -q +``` + +That is still the command. The generated suite is inside `tests/`, so it runs +with everything else and needs no separate invocation. + +## Why the second suite exists + +The four commits before `tests/property/` was written were four bugs in one +function — `hooks._segments`, the quote-aware splitter that decides whether a +shell command reaches the deny list. Each was found by a person typing one more +string, and each got an example-based test recording the string that found it. + +That is a good record and a bad search. `tests/test_hooks.py` now pins nineteen +command lines; the shell accepts infinitely many, and the fifth bug was not in +the nineteen. It was `( ssh gpu-box nvidia-smi )` — three tokens, no quoting, no +substitution, and the shortest bypass the deny list ever had. + +So `tests/property/shellgrammar.py` builds command lines from a grammar instead, +and carries the answer alongside the text: every node knows which heads the +shell would execute in it. The property is then one line — if the shell runs +`ssh`, the hook says so — and Hypothesis searches for a counterexample rather +than waiting for one to be reported. It found three in the first run: + +- `( cmd )` and `{ cmd; }` were never read as starting a command; +- `if`, `then`, `do`, `else`, `!` and `time` were read *as* the command they + introduce, so `for h in a b; do ssh $h; done` had a head of `do`; +- `rm --recursive -f` matched none of the six alternations in the `rm -rf` rule, + which covered short-with-short and long-with-long and no mixed pair. + +The other five modules under `tests/property/` are chosen on the same basis: +pure functions of their arguments, deciding something irreversible, where the +answer is constrained by an identity rather than by an example. A mean lies +between the extremes it was taken over; a rolling spend never falls when a run +is submitted; a document hashes the same after a round trip through the archive. + +### Profiles + +```bash +HYPOTHESIS_PROFILE=deep python -m pytest tests/property -q +``` + +- `dev` (default) — 50 examples, `derandomize=True`. About a second. A property + suite that fails one time in five and passes when you re-run it teaches people + to re-run it. +- `ci` — 300 examples, random seeds, so CI explores what a developer never will. +- `deep` — 2000 examples. Worth running deliberately against a module that has + just changed. + +### Writing one + +Two rules, both learned the hard way in this directory: + +**Fixtures come first in the signature.** `@given` binds positional strategies +to the *trailing* parameters, so `def test(rows, tmp_path)` hands the strategy to +`tmp_path` and asks pytest for a fixture called `rows`. + +**A function-scoped fixture is set up once and shared by every example.** The +health check that says so is suppressed in `tests/property/conftest.py`, because +most properties here are pure — but anything that writes needs its own isolation +per example, or example 2 reads example 1's ledger. `test_prop_jsonl.py` uses a +module-level counter for a fresh file; `test_prop_ceilings.py` uses the +`fresh_workspace` fixture, which re-points `GRAD_ROOT` at a new directory. Both +of those exist because the first version of each test silently passed on +leftovers — a round trip that appended six records and read back 242. + +## Mutation testing + +Coverage says a line ran. It does not say anything would have failed if the line +were different, and those turn out to be very different questions: a line +executed by twenty tests that all assert on something else is covered and +unprotected. + +`mutmut` changes one line at a time and runs the tests. A mutant that survives is +a change to the source that no test objected to. + +```bash +mutmut run +mutmut results +mutmut show +``` + +Configured in `pyproject.toml` under `[tool.mutmut]`, deliberately narrow: +`source_paths` is eight modules, not the project. Mutation testing costs about +one test run per mutant, so the useful version of it is aimed rather than +sprayed. These eight are the pure ones, deciding the irreversible things — what +gets denied, what a run measured, what is written to the ledger — and they are +the ones `tests/property/` already covers, which is what makes a surviving +mutant a finding rather than a to-do. Widen it one module at a time, when that +module is what changed. + +It is **not** in CI. Hours per run is a thing to spend on a module you are +changing, not on every push. + +### On Windows + +mutmut refuses to run natively on Windows ([mutmut#397]) — it forks, and Windows +has no fork. Use WSL: + +```bash +wsl -d +python3 -m venv ~/gradmut/.venv +~/gradmut/.venv/bin/pip install -e ".[dev]" +cd /path/to/checkout && ~/gradmut/.venv/bin/python -m mutmut run +``` + +Working from a copy inside the WSL filesystem rather than over `/mnt/d` is worth +the `tar` — mutmut copies the whole tree into `mutants/` and then runs pytest in +it several hundred times, and 9p is not the filesystem for that. + +[mutmut#397]: https://github.com/boxed/mutmut/issues/397 + +## The three tests that fail for environmental reasons + +Unchanged, and documented in [`CONTRIBUTING.md`](../CONTRIBUTING.md): two lock +tests in `tests/test_desktop_app.py` fail if a real Grad is running, and +`tests/test_wakeup.py::test_the_deadline_is_reported_as_a_deadline` depends on +the time of day. CI deselects the third and holds nothing. diff --git a/hooks.py b/hooks.py index 325a0e1..c8f0f5e 100644 --- a/hooks.py +++ b/hooks.py @@ -114,15 +114,29 @@ def message(self) -> str: ("tools.report", "write"), ) -# Both orders of a combined flag (`-rf`, `-fr`) *and* the separated form -# (`rm -r -f x`), which the combined-only pattern let straight through. +# Recursive *and* forced, in either order and in any spelling. +# +# Six hand-written alternations stood here, one per order of one pair of +# spellings, and between them they covered the combined flag (`-rf`, `-fr`), the +# separated short form (`-r -f`) and the separated long form +# (`--recursive --force`). What no alternation covered was a *mixed* pair: +# `rm --recursive -f notes` matched none of them, and it is the spelling +# somebody writes when they are being explicit about the dangerous half. +# +# Two lookaheads instead, one per property, which is what the rule actually says +# -- "recursive appears, and force appears, within this command" -- and is +# order-free and spelling-free by construction rather than by enumeration. +# `[^|;&\r\n]*` in each keeps the search inside the one command, so +# `rm x | grep -rf y` is still not a recursive delete. +# +# The short branch has no trailing `\b` on purpose: `-rf` has no boundary +# between its `r` and its `f`, and requiring one is what made the combined form +# need its own alternation in the first place. The long branch keeps it, so +# `--recursive` matches and `--recursive-something` would not. _RM_RF = re.compile( - r"\brm\b[^|;&\r\n]*\s-\w*[rR]\w*f" - r"|\brm\b[^|;&\r\n]*\s-\w*f\w*[rR]" - r"|\brm\b[^|;&\r\n]*\s-\w*[rR]\b[^|;&\r\n]*\s-\w*f\b" - r"|\brm\b[^|;&\r\n]*\s-\w*f\b[^|;&\r\n]*\s-\w*[rR]\b" - r"|\brm\b[^|;&\r\n]*--recursive[^|;&\r\n]*--force" - r"|\brm\b[^|;&\r\n]*--force[^|;&\r\n]*--recursive" + r"\brm\b" + r"(?=[^|;&\r\n]*\s(?:-\w*[rR]|--recursive\b))" + r"(?=[^|;&\r\n]*\s(?:-\w*f|--force\b))" ) _CURL_PIPE_SH = re.compile(r"\b(curl|wget|iwr|Invoke-WebRequest)\b[^|]*\|[^|]*\b(sh|bash|zsh|python|pwsh|powershell)\b") _CREDENTIAL_READ = re.compile(r"keyring\s+get|get_password\s*\(|\.credentials\.json") @@ -214,14 +228,192 @@ def _cost_bearing_over_budget(command: str) -> Denial | None: ) +#: Longest first, so `||` is never read as two `|` and `&&` never as two `&`. +#: +#: `(` and `)` are here because grouping starts a command exactly as an operator +#: does. `( ssh box )` and `{ ssh box; }` are two of the shortest bypasses that +#: existed, and they were invisible for the same reason the substitution bugs +#: were: the segment kept its delimiter, so the head of `( conda )` was `(` and +#: the deny list matched nothing. Note `$(` sits ahead of `(`, and the explicit +#: substitution branch in `_segments` runs before this list is scanned at all -- +#: a substitution opens a frame, a bare parenthesis only opens a segment. +_OPERATORS = ("||", "&&", "$(", "|", ";", "&", "`", "(", ")", "\n", "\r") + +#: `{` is a separator only where the shell reads it as the group reserved word, +#: which is where a blank follows it. That distinction is not pedantry: `find . +#: -exec grep ssh {} \;` and `${HOME}` and `awk '{print $1}'` all contain a brace +#: that is not grouping, and splitting on those would deny commands nobody wrote. +#: `}` needs no rule of its own -- the `;` the shell requires before it has +#: already ended the segment, and a segment that merely *starts* with `}` has a +#: harmless head. +_GROUP_OPEN = re.compile(r"\{\s") + + +def _blind_segments(command: str) -> list[str]: + """The split that ignores quoting. Kept as the fallback -- see `_segments`.""" + return [s for s in re.split(r"\|\||&&|[|;&()\r\n]|\{\s|\$\(|`", command) if s.strip()] + + def _segments(command: str) -> list[str]: """Split on shell operators so `foo && ssh bar` is inspected as two commands. A newline is in the class because a newline *is* a command separator: without it `"true\\nssh gpu-box nvidia-smi"` was one segment whose head was `true`, and the cheapest possible bypass of the deny list was pressing Enter. + + **Quoting is respected, because an operator inside quotes is not one.** + `grep -n "a\\|b" file` is a single command, and splitting it blindly left a + tail of `b" file`; a pattern that happened to contain a denied word was + denied as though the user had typed it as a command. Read-only greps for the + deny list's own vocabulary are exactly what someone auditing this file + writes, so the false denial landed on the people best placed to hit it. + + **Command substitution is the exception, and stays live inside double + quotes**, where `"$(ssh box)"` really does start a new command -- which is + also the one place worth hiding one. Single quotes suppress it, as the shell + does. + + **A substitution opens a whole command context, not just a split point.** + Emitting one split at `$(` and carrying on as though the body were quoted + text is a fail-open: `"$(echo x | ssh box)"` keeps its `|` unsplit, every + head is `echo`, and the deny list waves through a pipeline the shell will + run. The suspended quote is stacked and restored at the *matching* closer, + so operators inside the body split and the text after it is quoted again. + + Matching is the operative word, and it is why the frame counts grouping + parentheses. Ending at the first `)` instead reopens the quote halfway + through `"$( (true) | ssh box )"`, and the rest of a body the shell really + does execute is read as a string. The mirror case is the reason this cannot + simply deny every `)`: in `"$(cat f) | ssh box"` the parenthesis genuinely + ends the substitution, the tail is literal text, and no `ssh` runs -- so a + denial there would be a false one. + + Unbalanced quotes -- and unclosed substitutions -- fall back to the blind + split: a string this cannot parse is one it must not vouch for. + Over-splitting only ever costs a false denial, and that is the direction a + deny list is allowed to be wrong in. """ - return [s for s in re.split(r"\|\||&&|[|;&\r\n]|\$\(|`", command) if s.strip()] + out: list[str] = [] + buf: list[str] = [] + quote: str | None = None + #: One frame per open substitution, innermost last: the quote state it + #: suspended, which delimiter closes it, and how many grouping parentheses + #: are open inside it. A substitution is a *command* context, so + #: `"$(a | ssh box)"` has to go back to splitting on `|` inside it and back + #: to quoted text after the `)`. + #: + #: The depth is what makes the closer the *matching* one. Popping at the + #: first `)` ends the frame early on `"$( (true) | ssh box )"`, which puts + #: the pipeline back inside quotes and lets an `ssh` the shell really runs + #: go uninspected. It has to stay a count and not a flag because the + #: grouping can nest. + suspended: list[list[Any]] = [] + i, n = 0, len(command) + while i < n: + char = command[i] + # A backslash takes the next character with it. Outside quotes that is + # the shell's rule exactly; inside double quotes the shell honours it + # only before `$`, a backtick, `"`, `\` and a newline, and treating the + # rest as escaped too can only ever suppress a split, never invent one. + # The characters it would wrongly escape are not operators in that + # position anyway, so the two agree everywhere it matters here. + if char == "\\" and quote != "'" and i + 1 < n: + buf.append(char) + buf.append(command[i + 1]) + i += 2 + continue + # Single quotes first, because they suppress substitution outright. + if quote == "'": + if char == "'": + quote = None + buf.append(char) + i += 1 + continue + # `$(` opens a command context anywhere it is not single-quoted; a + # backtick does the same, but only inside double quotes -- unquoted it + # is already an operator below. + if command.startswith("$(", i) or (char == "`" and quote == '"'): + suspended.append([quote, "`" if char == "`" else ")", 0]) + quote = None + out.append("".join(buf)) + buf = [] + i += 2 if char == "$" else 1 + continue + if quote == '"': + if char == '"': + quote = None + buf.append(char) + i += 1 + continue + if suspended: + frame = suspended[-1] + # A `(` here is grouping, not a substitution: the `$(` case above + # consumed both of its characters, so it can never reach this. + # + # It ends the segment as well as deepening the frame. Counting the + # depth without splitting kept the frame honest and left the group + # unread: `"$( (ssh box) )"` stayed one segment whose head was + # `(ssh`, so the fix for the *matching closer* had a bypass sitting + # inside the very string it was written for. + if frame[1] == ")" and char == "(": + frame[2] += 1 + out.append("".join(buf)) + buf = [] + i += 1 + continue + if char == frame[1]: + if frame[1] == ")" and frame[2]: + frame[2] -= 1 + out.append("".join(buf)) + buf = [char] + else: + quote = suspended.pop()[0] + # The closer ends the segment instead of joining it. A body + # that takes no arguments is a single token, and keeping the + # delimiter made that token `ssh)"`, which is not `ssh` and + # was not denied. It opens the *next* buffer rather than + # being dropped, so the quoted tail of `"$(date) ssh box"` + # inherits a harmless head instead of reading as a command + # the shell never runs. + out.append("".join(buf)) + buf = [char] + i += 1 + continue + if char in "'\"": + quote = char + buf.append(char) + i += 1 + continue + operator = next((op for op in _OPERATORS if command.startswith(op, i)), None) + if operator is None and _GROUP_OPEN.match(command, i): + operator = "{" + if operator is not None: + out.append("".join(buf)) + buf = [] + i += len(operator) + continue + buf.append(char) + i += 1 + # An unclosed substitution is as unparseable as an unbalanced quote. + if quote is not None or suspended: + return _blind_segments(command) + out.append("".join(buf)) + return [s for s in out if s.strip()] + + +#: Reserved words that *introduce* a command rather than being one. Skipping +#: them is finishing the parse, not widening the rule: `if ssh box; then ...` +#: and `for h in a b; do ssh $h; done` both run ssh, and both left a segment +#: whose first token was grammar. `sudo`, `nohup`, `env` and `exec` are +#: deliberately absent -- those are programs that run other programs, which is +#: the indirection class this module's docstring puts out of scope. +#: +#: `for`, `in` and `case` are absent for a different reason: what follows them +#: is a *name*, not a command, so skipping them would make `for ssh in a b` look +#: like a remote execution and deny a loop nobody should be denied. +_INTRODUCERS = frozenset({ + "if", "elif", "then", "else", "while", "until", "do", "!", "time", "{", "(", ";", +}) def _head(segment: str) -> str: @@ -232,6 +424,8 @@ def _head(segment: str) -> str: for token in tokens: if "=" in token and not token.startswith("-") and not token.startswith("/"): continue # leading VAR=value assignments + if token in _INTRODUCERS: + continue return token.rsplit("/", 1)[-1].rsplit("\\", 1)[-1].lower().removesuffix(".exe") return "" diff --git a/prompts/system.md b/prompts/system.md index bdd6ef9..0fe571a 100644 --- a/prompts/system.md +++ b/prompts/system.md @@ -154,8 +154,8 @@ you need a workflow. Don't guess flags. `submit` refuses without a passing preflight for the exact submission, without an open expectation, over either spend ceiling, or while a run is uncollected past its window. `ssh`, `scp`, `rsync`, `hf`, `huggingface-cli`, and `kaggle` are -denied directly — use `gpu.py`, `jobs.py`, and `kaggle.py`, which hold the -credentials. These are not obstacles to route around; +denied directly — use `gpu.py`, `jobs.py`, `kaggle.py`, and `modal.py`, which +hold the credentials. These are not obstacles to route around; they are the parts of the system that survive a deadline. `pip`, `pip3` and `conda` are denied too, and for a different reason: they diff --git a/pyproject.toml b/pyproject.toml index 6c24ed5..2c4842c 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -112,7 +112,19 @@ wiki = ["repowiki==0.3.1"] # install time. Optional because the campaign bookkeeping and the budget gate -- # the parts that matter -- are ours either way. evolve = ["shinka-evolve==0.0.7"] -dev = ["pytest>=8.0", "pytest-asyncio>=0.23"] +# `hypothesis` is not optional-in-practice: `tests/property/` imports it at +# module scope, so a `dev` install without it turns the generated suite into a +# collection error rather than a skip -- which is the right way round. Those +# tests are the ones that found the shell-parser bypasses, and a suite that +# quietly stops running them is worse than one that fails to start. +# +# `mutmut` is genuinely optional and is *not* run by CI. It answers a different +# question -- "does any test notice when this line changes?" -- at a cost +# measured in CPU-hours, which is a thing to spend deliberately on a module that +# has just changed rather than on every push. It also refuses to run natively on +# Windows (its issue #397), so on the machine this was written on it is a WSL +# job; `docs/testing.md` has the invocation. +dev = ["pytest>=8.0", "pytest-asyncio>=0.23", "hypothesis>=6.100", "mutmut>=3.7"] [project.scripts] grad = "agent:main" @@ -143,3 +155,64 @@ addopts = "-q" markers = [ "slow: integration tests that start a real Jupyter kernel", ] + +# --------------------------------------------------------------------------- +# Mutation testing +# --------------------------------------------------------------------------- +[tool.mutmut] +# The question this answers is not "is the code covered" but "would anything +# notice if it were wrong", and those turn out to be very different questions. +# A line executed by twenty tests that all assert on something else is covered +# and unprotected, and coverage reports it as green. +# +# `source_paths` is a short list on purpose, and it is not the whole project. +# Mutation testing costs roughly one test run per mutant, so the useful version +# of it is aimed rather than sprayed: these are the modules that are pure +# functions of their arguments, that decide something irreversible (what gets +# denied, what a run measured, what is written to the ledger), and that a +# generated suite in `tests/property/` already covers -- which is what makes a +# surviving mutant here a finding rather than a to-do. +# +# Widen it deliberately, one module at a time, when that module is what changed. +source_paths = [ + "hooks.py", + "core/stats.py", + "core/jsonl.py", + "core/traces.py", + "core/submission.py", + "core/kaggle_quota.py", + "core/effort.py", + "core/compaction.py", +] + +# The suite is about three minutes, and mutmut runs it once per surviving +# mutant. Naming the covering tests here is what turns days into hours: mutmut +# still narrows further by which tests actually reach the mutated function, but +# it cannot narrow below the set it was given. +pytest_add_cli_args_test_selection = [ + "tests/property", + "tests/test_hooks.py", + "tests/test_stats.py", + "tests/test_jsonl.py", + "tests/test_traces.py", + "tests/test_submission.py", + "tests/test_effort.py", + "tests/test_experiments.py", + "tests/test_gates.py", + "tests/test_kaggle.py", + "tests/test_context_and_compaction.py", +] + +# mutmut runs the suite from a copy of the tree under `mutants/`, so anything +# imported at collection time has to be there. `tests/` and `pyproject.toml` it +# copies by itself; these are the rest of what `tests/conftest.py` reaches for +# -- `ui` because the autouse process-state fixture imports it, `core` in full +# because the eight modules above are not the only ones their imports touch. +also_copy = ["core", "tools", "ui", "agent.py", "config"] + +# No cache provider, because several hundred pytest processes writing one +# `.pytest_cache` inside the mutant tree is contention for a file none of them +# reads. The generated tests run under Hypothesis's `dev` profile, which is +# `derandomize=True` -- deliberately, since a mutant killed by a lucky seed is a +# mutant reported as killed and never looked at again. +pytest_add_cli_args = ["-p", "no:cacheprovider", "-q"] diff --git a/tests/property/conftest.py b/tests/property/conftest.py new file mode 100644 index 0000000..200b3c1 --- /dev/null +++ b/tests/property/conftest.py @@ -0,0 +1,106 @@ +"""Hypothesis settings for the property suite. + +These tests answer a different question from the ones next door. `tests/` says +"this input produces that output", which is what you write once you know which +input to worry about. The four commits before this directory existed were all +the same shape -- a shell string nobody had thought to type, waved through by a +parser that looked right -- so the question here is "is there *any* input that +breaks the rule", asked by a generator that has no idea what a reviewer expects. + +Three profiles, because the cost of an answer and the value of one are not the +same in every setting: + + * `dev` (default) -- 50 examples. Adds about a second to a local run. + * `ci` -- 300 examples, and no `derandomize`, so CI explores seeds a developer + never will and a rare counterexample surfaces on somebody else's machine + rather than never. + * `deep` -- 2000 examples with a long deadline, for running deliberately + against a module that has just changed. + +Select with `HYPOTHESIS_PROFILE=deep python -m pytest tests/property`. + +`derandomize` is on for `dev` so that a local run is reproducible: a property +suite that fails one time in five and passes when you re-run it teaches people +to re-run it. +""" + +from __future__ import annotations + +import itertools +import os +import sys +from pathlib import Path + +import pytest +from hypothesis import HealthCheck, Verbosity, settings + +# `shellgrammar.py` is a helper, not a test, and it lives next to the tests that +# use it because it is about them and nothing else. Neither `tests/` nor this +# directory is a package -- adding `__init__.py` here would change how pytest +# imports the fifty modules next door, which is a large change to make for one +# import -- so the directory goes on the path the same way `tests/conftest.py` +# puts the repository root there. +sys.path.insert(0, str(Path(__file__).resolve().parent)) + +# The repository's `tests/conftest.py` installs four autouse, function-scoped +# fixtures -- the temp workspace, the temp app directory, the process-state +# reset and the network refusal. Hypothesis sets those up once and then runs +# every example inside them, which is what `function_scoped_fixture` warns +# about, and it is right to warn: an example that writes into the workspace can +# be read by the next one. +# +# Suppressed rather than worked around, because the properties here are either +# pure functions of their arguments or make their own per-example directory +# (see `tests/property/test_prop_jsonl.py`). What the fixtures are actually +# providing is the *negative* guarantee -- no network, no writes into the +# developer's real `%LOCALAPPDATA%` -- and that one does not decay across +# examples. +_COMMON = { + "suppress_health_check": [HealthCheck.function_scoped_fixture], + # Wall-clock deadlines and a Windows filesystem do not mix: the first + # example to touch a cold path pays for the whole directory tree and gets + # blamed for it. Coverage is bounded by `max_examples` here, not by time. + "deadline": None, +} + +settings.register_profile("dev", max_examples=50, derandomize=True, **_COMMON) +settings.register_profile("ci", max_examples=300, **_COMMON) +settings.register_profile( + "deep", max_examples=2000, verbosity=Verbosity.normal, **_COMMON +) + +settings.load_profile(os.environ.get("HYPOTHESIS_PROFILE", "dev")) + + +@pytest.fixture +def fresh_workspace(tmp_path): + """A whole workspace per *example*, for the properties that write a ledger. + + The suppression above is safe for a pure function and not safe for anything + that appends to a file. `tests/conftest.py` points `GRAD_ROOT` at one + `tmp_path` per test *function*, so a hundred examples would share one + ledger: run counts would accumulate across examples, `budget.create` would + refuse the second example's project as already existing, and "spend equals + the sum of what was submitted" would be false for every example after the + first. All three of those happened. + + Returns a callable rather than a path, because the reset has to happen at + the top of each example and a fixture body runs once. + + `os.environ` is written directly rather than through `monkeypatch`, which is + also function-scoped; the outer fixture's own monkeypatch still owns the + original value and restores it at teardown. + """ + from core import config, paths + + counter = itertools.count() + + def reset(): + root = tmp_path / f"ws-{next(counter)}" + os.environ["GRAD_ROOT"] = str(root) + os.environ["GRAD_CONFIG"] = str(root / "config" / "grad.toml") + config._cache.clear() + paths.ensure_workspace() + return root + + return reset diff --git a/tests/property/shellgrammar.py b/tests/property/shellgrammar.py new file mode 100644 index 0000000..8b84337 --- /dev/null +++ b/tests/property/shellgrammar.py @@ -0,0 +1,227 @@ +"""A generator for shell command lines that knows what it built. + +`hooks._segments` is a hand-written shell parser, and the four commits before +this file existed were four bugs in it -- each found by a person typing one more +string. The example-based tests in `tests/test_hooks.py` are the record of those +four strings. What they cannot do is find the fifth. + +So this builds command lines from a grammar instead, and carries the answer +alongside the text: every node knows which command heads the shell would +*execute* in it, which makes `Node.heads` the oracle. No shell is invoked -- the +point is to compare the parser against the language, not against another parser +-- so the grammar is restricted to constructs whose semantics are not in dispute: + + * the operators `hooks._OPERATORS` already lists, plus the newline; + * `( ... )` and `{ ...; }`, which run their contents; + * `$( ... )` and backticks, which run their contents and are live inside + double quotes; + * single quotes, which suppress all of it. + +Everything the module docstring names as a known bypass -- `bash -c`, +`ssh host "cmd"`, aliases, `eval`, environment indirection -- is *not* generated. +Those are architectural, the docstring says so, and a property suite that +generated them would be asserting a claim the code has never made. + +The oracle is allowed to under-claim and never to over-claim. `$(echo ls)` at +command position really does run `ls` in a shell, and `heads` says only `echo`; +that direction costs a missed counterexample, which is a weaker suite. The other +direction would cost a false failure, which is a suite people learn to ignore. + +The vocabulary keeps the two directions independent: `SAFE_HEADS` and +`SAFE_ARGS` contain nothing that trips the three regex rules in `evaluate_bash` +(`rm -rf`, a download piped into a shell, a credential read), so a denial from a +generated command is always attributable to the head that earned it. +""" + +from __future__ import annotations + +from dataclasses import dataclass +from typing import Any + +from hypothesis import strategies as st + +import hooks + +#: Command heads the deny list refuses. Taken from the module rather than +#: copied, so a new entry is covered by every property here the day it lands. +DENIED_HEADS = sorted(hooks._DENIED_COMMANDS) + +#: Heads that must never be denied. Deliberately includes `python`, because +#: `python -m tools.gpu submit` is the *suggested* route and denying the +#: suggestion would be the worst possible failure. +SAFE_HEADS = ["echo", "true", "ls", "cat", "python", "git", "grep", "wc"] + +#: Argument words. No `-rf`, no URL, no `keyring`: see the module docstring. +SAFE_ARGS = ["x", "-n", "file.txt", "--json", "notes/", "1", "a.py"] + + +# --------------------------------------------------------------------------- +# the model +# --------------------------------------------------------------------------- +@dataclass(frozen=True) +class Node: + """A fragment of shell, and what it runs. + + `has_squote` and `has_backtick` exist because two of the wrappers below are + not re-entrant in the shell itself. `'a'b'c'` is three fragments rather than + a nested quote, and a backtick cannot contain a backtick without escaping -- + so nesting either inside itself would produce a string whose real meaning is + not the one this node claims, and the oracle would be lying rather than the + parser wrong. Both are filtered at the point of composition. + """ + + text: str + #: Heads this fragment executes. Empty for a fragment that is only data. + heads: tuple[str, ...] + has_squote: bool = False + has_backtick: bool = False + + def runs_denied(self) -> bool: + return any(head in hooks._DENIED_COMMANDS for head in self.heads) + + +def _simple(head: str, spelling: str, args: list[str], assignment: str | None) -> Node: + """One command: an optional `VAR=value` prefix, a head, some arguments. + + `spelling` is how the head is written -- bare, path-qualified, or with the + Windows extension `_head` strips. All four resolve to the same command, and + a deny list that only knows the bare form is one `/usr/bin/` away from + silent. + """ + written = {"bare": head, "path": f"/usr/bin/{head}", "dot": f"./{head}", + "exe": f"{head}.exe"}[spelling] + prefix = f"{assignment} " if assignment else "" + return Node(" ".join([prefix + written, *args]).strip(), (head,)) + + +_HEAD_SPELLINGS = st.sampled_from(["bare", "path", "dot", "exe"]) +_ASSIGNMENTS = st.sampled_from([None, "FOO=1", "LC_ALL=C"]) + + +def simple_commands(heads: list[str]) -> st.SearchStrategy[Node]: + return st.builds( + _simple, + st.sampled_from(heads), + _HEAD_SPELLINGS, + st.lists(st.sampled_from(SAFE_ARGS), max_size=3), + _ASSIGNMENTS, + ) + + +#: Operators that separate two commands, both of which run. `&` backgrounds the +#: left one and a newline ends it; in neither case does the right one stop +#: running, which is the only thing the deny list has to agree with. +_BINARY = ["&&", "||", ";", "|", "&", "\n"] + + +def _merge(text: str, *parts: Node, extra: tuple[str, ...] = ()) -> Node: + return Node( + text, + tuple(h for p in parts for h in p.heads) + extra, + any(p.has_squote for p in parts), + any(p.has_backtick for p in parts), + ) + + +def _binary(op: str, left: Node, right: Node) -> Node: + sep = op if op == "\n" else f" {op} " + return _merge(f"{left.text}{sep}{right.text}", left, right) + + +def _subshell(inner: Node) -> Node: + """`( cmd )` -- a subshell. A different process, the same language.""" + return _merge(f"( {inner.text} )", inner) + + +def _brace_group(inner: Node) -> Node: + """`{ cmd; }` -- a group. The trailing `;` is what the shell requires.""" + return _merge(f"{{ {inner.text}; }}", inner) + + +def _conditional(inner: Node) -> Node: + """`if true; then cmd; fi`. The body runs, and `then` is not a command.""" + return _merge(f"if true; then {inner.text}; fi", inner) + + +def _loop(inner: Node) -> Node: + """`for i in 1 2; do cmd; done`. Same shape, and `do` is not a command.""" + return _merge(f"for i in 1 2; do {inner.text}; done", inner) + + +#: Reserved words that take a command and are valid in any command position, so +#: the grammar can nest them freely. `!` and `time` are *not* here: bash requires +#: both at the start of a pipeline, so `a | ! b` is a syntax error rather than a +#: command, and generating one would have the property demand a denial for a +#: string no shell would run. They are covered by +#: `test_a_reserved_word_does_not_hide_the_command_after_it` instead. +_COMPOUND = (_subshell, _brace_group, _conditional, _loop) + + +def _substitution(inner: Node, form: str) -> Node: + """Command substitution. Live unquoted and inside double quotes alike. + + The quoted forms are the ones worth generating most: they are where a + reader's intuition says "this is a string" and the shell says "this is a + command", and three of the four fixed bugs lived in exactly that gap. + """ + text = { + "bare": f"$({inner.text})", + "double": f'"$({inner.text})"', + "backtick": f'"`{inner.text}`"', + "argument": f'echo "$({inner.text})"', + }[form] + node = _merge(text, inner, extra=("echo",) if form == "argument" else ()) + if form == "backtick": + node = Node(node.text, node.heads, node.has_squote, True) + return node + + +def _single_quoted(inner: Node) -> Node: + """`echo '$(cmd)'` -- data, not a command. The shell runs nothing in here. + + Generated so the no-false-denial property has something with the shape of an + execution and none of the substance. Wrapped in `echo` because a bare quoted + string is not a command line anybody writes. + """ + return Node(f"echo '$({inner.text})'", ("echo",), True, inner.has_backtick) + + +def commands(heads: list[str], *, max_leaves: int = 4) -> st.SearchStrategy[Node]: + """Command lines built from `heads`, with the executed set carried along. + + Wrapped in `deferred` for its repr and nothing else: `recursive` renders as + its whole expansion, and a failure report that opens with 54 kB of strategy + before it gets to the counterexample is a report nobody reads to the end. + """ + return st.deferred( + lambda: st.recursive( + simple_commands(heads), + lambda children: st.one_of( + st.builds(_binary, st.sampled_from(_BINARY), children, children), + st.one_of(*[st.builds(wrap, children) for wrap in _COMPOUND]), + st.builds( + _substitution, + children, + st.sampled_from(["bare", "double", "argument"]), + ), + st.builds( + _substitution, + children.filter(lambda n: not n.has_backtick), + st.just("backtick"), + ), + st.builds(_single_quoted, children.filter(lambda n: not n.has_squote)), + ), + max_leaves=max_leaves, + ) + ) + + +def describe(node: Node) -> dict[str, Any]: + """What to print when a property fails, so the report is the whole story.""" + segments = hooks._segments(node.text) + return { + "command": node.text, + "executes": sorted(set(node.heads)), + "segments": segments, + "parsed heads": [hooks._head(s) for s in segments], + } diff --git a/tests/property/test_prop_ceilings.py b/tests/property/test_prop_ceilings.py new file mode 100644 index 0000000..7b547c4 --- /dev/null +++ b/tests/property/test_prop_ceilings.py @@ -0,0 +1,473 @@ +"""Properties of the spend and quota ceilings. + +These are the gates the project describes as standing between the agent and a +$40 mistake, and the failure they exist to prevent is not "a gate raised the +wrong exception". It is "a sequence of individually reasonable submissions added +up to a number nobody chose". That is a property of a *history*, not of a call, +which is why the tests here build a ledger a record at a time and then ask the +same question after every one. + +Three invariants, and each of them has a plausible way to be false that no +single example would show: + + * spend is monotone -- submitting a run never lowers the rolling total, so N + jobs submitted before any is collected cannot all pass a check that each of + them individually passes; + * a collection does not change what a run cost the ceiling by more than the + difference between its estimate and its actual, so "collect to free up + headroom" cannot be a way to spend twice; + * the gate and the report agree -- whatever `status` says is over is exactly + what `over_budget` names and exactly what `check` refuses. + +The ledger is real rather than mocked, which the repository's own conftest goes +out of its way to make possible: "a mock of a gate proves nothing about the +gate". +""" + +from __future__ import annotations + +import datetime as _dt +import itertools + +import pytest +from hypothesis import assume, given, note +from hypothesis import strategies as st + +import hooks +from core import budget, config, gates, kaggle_quota, quota_log +from core import ledger_store as ls +from core.errors import GateRefusal + +_ids = itertools.count() + +#: Dollar figures at the scale this project actually works in. A run costing +#: 1e300 tests float64, not the ceiling. +usd = st.floats(min_value=0.0, max_value=500.0, allow_nan=False, allow_infinity=False) +hours = st.floats(min_value=0.0, max_value=60.0, allow_nan=False, allow_infinity=False) + + +def _submit(estimate_usd: float, *, project: str = "unassigned", smoke: bool = False) -> str: + """One run, in flight, in the ledger. Returns its id. + + Real records through the real append path, as §24 asks for: "the budget + gates deserve the same treatment §6's gates got: tested against a real + ledger, not mocks, because they are what stands between a loop and a bill." + """ + run_id = ls.new_id("run") + ls.append_run_event( + { + "type": ls.T_RUN_SUBMITTED, + "id": run_id, + "status": "in_flight", + "submitted_at": ls.now_iso(), + "project": project, + "estimate_usd": estimate_usd, + "smoke": smoke, + "platform": "hf-jobs", + } + ) + return run_id + + +def _collect(run_id: str, actual_usd: float) -> None: + ls.append_run_event( + { + "type": ls.T_RUN_COLLECTED, + "id": run_id, + "status": "completed", + "collected_at": ls.now_iso(), + "cost_usd_actual": actual_usd, + "results": {}, + "deviations": [], + } + ) + + +def _submit_kaggle(hours: float) -> None: + """One metered Kaggle run, in flight. The hours ledger's own unit.""" + ls.append_run_event( + { + "type": ls.T_RUN_SUBMITTED, + "id": ls.new_id("run"), + "status": "in_flight", + "submitted_at": ls.now_iso(), + "platform": kaggle_quota.PLATFORM, + kaggle_quota.F_KIND: "gpu", + kaggle_quota.F_ACCELERATOR: "P100", + kaggle_quota.F_ESTIMATE: hours, + } + ) + + +# --------------------------------------------------------------------------- +# the rolling total +# --------------------------------------------------------------------------- +@given(st.lists(usd, min_size=1, max_size=6)) +def test_spend_never_falls_as_runs_are_submitted( + fresh_workspace, estimates: list[float] +) -> None: + """The invariant the whole in-flight accounting exists for. + + "A job that has not been collected yet is not free. Without this, N jobs + submitted before any is collected all pass the ceiling check." That is a + statement about a sequence, and it is only ever true or false of one. + """ + fresh_workspace() + seen = 0.0 + for estimate in estimates: + _submit(estimate) + total = ls.rolling_spend(30)["total_usd"] + note({"added": estimate, "total": total}) + assert total >= seen - 1e-6 + seen = total + assert seen == pytest.approx(sum(estimates), abs=1e-3) + + +@given(st.lists(st.tuples(usd, usd), min_size=1, max_size=5)) +def test_collecting_a_run_replaces_its_estimate_and_nothing_else( + fresh_workspace, pairs: list[tuple[float, float]] +) -> None: + """An actual supersedes its own estimate, not somebody else's. + + A collection that subtracted the estimate but added the actual to a + different pool -- or that left both -- would make "collect in-flight runs so + their estimates become actuals", the fix every spend refusal prints, either + a no-op or a way to double-count. + """ + fresh_workspace() + ids = [_submit(estimate) for estimate, _ in pairs] + assert ls.rolling_spend(30)["total_usd"] == pytest.approx( + sum(e for e, _ in pairs), abs=1e-3 + ) + for run_id, (_, actual) in zip(ids, pairs): + _collect(run_id, actual) + rolling = ls.rolling_spend(30) + note(rolling) + assert rolling["total_usd"] == pytest.approx(sum(a for _, a in pairs), abs=1e-3) + assert rolling["in_flight_usd"] == pytest.approx(0.0, abs=1e-3) + assert rolling["actual_usd"] == pytest.approx(rolling["total_usd"], abs=1e-3) + + +@given(st.lists(usd, max_size=5), st.integers(min_value=1, max_value=90)) +def test_the_window_is_the_only_thing_that_drops_a_run( + fresh_workspace, estimates: list[float], window: int +) -> None: + """Everything inside the window counts, and the split adds up. + + `total_usd`, `actual_usd` and `in_flight_usd` are rounded separately, so + they are allowed to disagree in the last place and nowhere else -- a gap + bigger than that is a run counted in the total and in neither half. + """ + fresh_workspace() + for estimate in estimates: + _submit(estimate) + rolling = ls.rolling_spend(window) + note(rolling) + assert rolling["total_usd"] == pytest.approx( + rolling["actual_usd"] + rolling["in_flight_usd"], abs=1e-3 + ) + assert len(rolling["runs"]) == len(estimates) + + +@given(st.lists(usd, max_size=4)) +def test_a_run_older_than_the_window_is_outside_it( + fresh_workspace, estimates: list[float] +) -> None: + """The window is measured from `now`, and `now` is injectable for this test. + + Without the injection this property could only be checked by waiting a + month, which is why `check_spend` and `rolling_spend` both take it. + """ + fresh_workspace() + for estimate in estimates: + _submit(estimate) + future = _dt.datetime.now(_dt.timezone.utc) + _dt.timedelta(days=400) + assert ls.rolling_spend(30, now=future)["total_usd"] == 0.0 + assert ls.rolling_spend(1000, now=future)["total_usd"] == pytest.approx( + sum(estimates), abs=1e-3 + ) + + +# --------------------------------------------------------------------------- +# the gate +# --------------------------------------------------------------------------- +@given(st.lists(usd, max_size=4), usd) +def test_the_monthly_ceiling_is_never_crossed_quietly( + fresh_workspace, history: list[float], estimate: float +) -> None: + """Either the projected total is inside the ceiling, or the gate raised. + + Stated as a disjunction on purpose: it does not matter which branch a given + example takes, only that no example takes neither. That is the shape of + every ceiling claim in this project and the shape a single example cannot + have. + """ + fresh_workspace() + cfg = config.load(reload=True) + for spent in history: + _submit(spent) + monthly = float(cfg.get("spend", "monthly_usd", 200.0)) + per_job = float(cfg.get("spend", "per_job_usd", 25.0)) + before = ls.rolling_spend(int(cfg.get("spend", "window_days", 30)))["total_usd"] + note({"before": before, "estimate": estimate, "monthly": monthly}) + try: + result = gates.check_spend(estimate, cfg) + except GateRefusal as refusal: + assert refusal.code in ("spend_per_job", "spend_monthly") + assert estimate > per_job or before + estimate > monthly + else: + assert estimate <= per_job + assert result["projected_usd"] <= monthly + + +@given(usd) +def test_a_refusal_names_a_command_that_could_change_the_answer( + fresh_workspace, estimate: float +) -> None: + """"Errors carry the next command", from CONTRIBUTING, held to mechanically. + + A ceiling that refuses without a route forward is the kind that gets argued + around, and the argument is usually correct. + """ + fresh_workspace() + cfg = config.load(reload=True) + assume(estimate > float(cfg.get("spend", "per_job_usd", 25.0))) + with pytest.raises(GateRefusal) as caught: + gates.check_spend(estimate, cfg) + assert caught.value.fix + assert caught.value.detail + + +# --------------------------------------------------------------------------- +# the project allocation +# --------------------------------------------------------------------------- +@given(st.lists(usd, max_size=4), st.floats(min_value=1.0, max_value=400.0)) +def test_over_budget_names_exactly_what_status_says_is_over( + fresh_workspace, estimates: list[float], ceiling: float +) -> None: + """Two readers of one fact, which must not be able to disagree. + + `budget status` prints one and the hook in `hooks.py` acts on the other, so + a discrepancy is a project the meter calls fine and the agent cannot spend + in -- or worse, the reverse. + """ + fresh_workspace() + budget.create("proj-a", title="a", budget={"gpu_usd": ceiling}) + for estimate in estimates: + _submit(estimate, project="proj-a") + state = budget.status("proj-a") + note(state["resources"]["gpu_usd"]) + expected = [ + name + for name, node in state["resources"].items() + if node["ceiling"] is not None and node["spent"] > float(node["ceiling"]) + ] + assert budget.over_budget("proj-a") == expected + + +@given(st.lists(usd, max_size=4), st.floats(min_value=1.0, max_value=400.0), usd) +def test_the_project_gate_refuses_exactly_when_the_projection_crosses( + fresh_workspace, estimates: list[float], ceiling: float, proposed: float +) -> None: + """`check` and `status` are the same arithmetic, or the gate is decoration. + + Exit 12 rather than 6 is the whole point of this gate existing separately, + so it has to fire on the project's own numbers rather than on the machine's. + """ + fresh_workspace() + budget.create("proj-b", title="b", budget={"gpu_usd": ceiling}) + for estimate in estimates: + _submit(estimate, project="proj-b") + spent = budget.status("proj-b")["resources"]["gpu_usd"]["spent"] + note({"spent": spent, "ceiling": ceiling, "proposed": proposed}) + try: + budget.check("proj-b", gpu_usd=proposed, what="a job") + except GateRefusal: + assert spent + proposed > ceiling + else: + assert spent + proposed <= ceiling + + +@given(usd) +def test_a_project_with_no_ceiling_is_tracked_and_not_bounded( + fresh_workspace, proposed: float +) -> None: + """An unbudgeted project is not an overrun. + + The distinction matters because `over_budget` is consulted for *every* + cost-bearing command, including in a workspace where nobody has set a + budget at all -- and a gate that refused there would make the feature + mandatory by accident. + """ + fresh_workspace() + budget.create("proj-c", title="c", budget={}) + assert budget.over_budget("proj-c") == [] + assert budget.check("proj-c", gpu_usd=proposed) is not None + assert budget.over_budget(None) == [] + assert budget.over_budget("no-such-project") == [] + + +# --------------------------------------------------------------------------- +# Kaggle hours, which the dollar ceilings cannot see +# --------------------------------------------------------------------------- +@given(hours, hours) +def test_a_run_is_refused_by_the_session_cap_or_fits_in_one_session( + fresh_workspace, estimate: float, cap: float +) -> None: + """Kaggle stops the kernel at the cap, so a longer run has already failed. + + The gate is about that, not about cost: the hours are spent either way and + only what was checkpointed survives. + """ + assume(cap > 0) + root = fresh_workspace() + # Through the config file rather than by poking the dataclass, because the + # dataclass is not where the value comes from: `Config.get` reads the + # overlay and the project overrides ahead of the file, and a test that set + # the attribute directly would pass while the real precedence was broken. + path = root / "config" / "grad.toml" + path.parent.mkdir(parents=True, exist_ok=True) + path.write_text( + f"[kaggle.quota]\nmax_session_hours = {cap}\n", encoding="utf-8" + ) + cfg = config.load(reload=True) + try: + result = kaggle_quota.check_session(cfg, "gpu", estimate, accelerator="P100") + except GateRefusal as refusal: + assert estimate > cap + assert refusal.fix + else: + assert estimate <= cap + assert result["estimate_hours"] == pytest.approx(estimate) + + +@given(st.lists(hours, max_size=4)) +def test_accelerator_hours_never_falls_as_runs_are_submitted( + fresh_workspace, estimates: list[float] +) -> None: + """The weekly allowance has the same monotonicity claim as the dollars. + + It is a separate implementation over a different unit -- `hours_for_quota` + beside `cost_for_ceiling` -- so it needs its own statement of the property + rather than inheriting the one above. + """ + fresh_workspace() + seen = 0.0 + for i, estimate in enumerate(estimates): + _submit_kaggle(estimate) + pools = kaggle_quota.accelerator_hours(window_days=7)["pools"] + total = pools.get("gpu", {}).get("total_hours", 0.0) + note({"i": i, "added": estimate, "total": total}) + assert total >= seen - 1e-6 + seen = total + + +@given(hours) +def test_hours_are_never_negative_however_the_record_reads( + fresh_workspace, estimate: float +) -> None: + """A malformed record folds to zero rather than to a credit. + + A negative estimate in a ledger is damage; treating it as *refunding* the + weekly allowance would make the damage spendable. + """ + fresh_workspace() + for value in (-estimate, "not a number", None, float("nan")): + run = ls.Run( + "run-x", + { + "platform": kaggle_quota.PLATFORM, + kaggle_quota.F_KIND: "gpu", + kaggle_quota.F_ESTIMATE: value, + }, + ) + folded = kaggle_quota.hours_for_quota(run) + note({"value": value, "folded": folded}) + assert folded >= 0.0 + + +# --------------------------------------------------------------------------- +# the turn-boundary warning +# --------------------------------------------------------------------------- +# `hooks.budget_warning` is what the Stop hook prints between turns, and it is +# where mutation testing found the largest hole in this file: seventy-eight +# mutants, none of them covered by any test at all. It is not an enforcement +# point -- the Stop hook's `block` forces continuation rather than halting, so +# enforcement lives in `agent.py` and `pre_tool_use` -- which is precisely why +# nothing noticed. A warning nobody tests is a warning that can quietly stop +# appearing, and the thing it warns about is money. +@given(st.floats(min_value=0.0, max_value=2.0), st.floats(min_value=1.0, max_value=100.0)) +def test_the_warning_appears_exactly_when_a_threshold_is_crossed( + fresh_workspace, ratio: float, ceiling: float +) -> None: + """One rule, checked at every fraction rather than at three chosen ones. + + `WARN_AT` is `(0.75, 0.9, 1.0)`, and the claim is that the warning appears + if and only if the spend has reached the lowest of them -- so the boundary + is checked from both sides at every step, which is the part a + three-example test cannot do. + """ + fresh_workspace() + budget.create("warn", title="w", budget={"gpu_usd": ceiling}) + budget.set_current("warn") + _submit(ratio * ceiling, project="warn") + + # Read off the meter rather than recomputed from `ratio`. `spend` rounds to + # four places on the way out, so `ratio = 1.0` can arrive as a fraction of + # 0.99995 -- and a test that reconstructs the arithmetic instead of reading + # the number under test is asserting its own copy of the code. That the + # meter and the gate agree is `test_over_budget_names_exactly_what_status_ + # says_is_over`'s job; this one is about the warning agreeing with the meter. + fraction = budget.status("warn")["resources"]["gpu_usd"]["fraction"] + warning = hooks.budget_warning() + note({"ratio": ratio, "fraction": fraction, "warning": warning}) + + if fraction >= min(hooks.WARN_AT): + assert warning is not None + assert warning["threshold"] == max(t for t in hooks.WARN_AT if fraction >= t) + assert warning["project"] == "warn" + # The message is what a person reads at a turn boundary, and the only + # part of it that is a claim rather than a formatting choice is whether + # it says the ceiling has been passed. + assert ("now denied" in warning["message"]) == (warning["fraction"] >= 1.0) + else: + assert warning is None + + +@given(st.floats(min_value=0.8, max_value=3.0), st.floats(min_value=0.0, max_value=0.7)) +def test_the_warning_names_the_resource_nearest_its_ceiling( + fresh_workspace, hot: float, cold: float +) -> None: + """One line, for the resource that matters. + + "Reports the *highest* threshold crossed rather than one line per resource: + a turn boundary is a bad place for a wall of text." Which of two resources + is nearest its ceiling is a comparison, and a comparison written the wrong + way round still produces a plausible-looking warning about the wrong thing. + """ + fresh_workspace() + budget.create("two", title="t", budget={"gpu_usd": 100.0, "credits_usd": 100.0}) + budget.set_current("two") + _submit(hot * 100.0, project="two") + quota_log.record({"project": "two", "credits_usd": cold * 100.0}) + + warning = hooks.budget_warning() + note({"hot": hot, "cold": cold, "warning": warning}) + assert warning is not None + assert warning["resource"] == "gpu_usd" + + +@given(usd) +def test_no_project_means_no_warning(fresh_workspace, spent: float) -> None: + """Nothing selected, nothing budgeted, nothing to say. + + The Stop hook runs after every turn in every workspace, including one where + nobody has made a project -- so this is the common case rather than an edge + one, and a warning here would be noise on every turn forever. + """ + fresh_workspace() + _submit(spent) + assert hooks.budget_warning() is None + budget.create("unbudgeted", title="u", budget={}) + budget.set_current("unbudgeted") + assert hooks.budget_warning() is None diff --git a/tests/property/test_prop_hooks.py b/tests/property/test_prop_hooks.py new file mode 100644 index 0000000..77971f7 --- /dev/null +++ b/tests/property/test_prop_hooks.py @@ -0,0 +1,316 @@ +"""Properties of the Bash deny list. + +Two of them, and they pull in opposite directions on purpose. + +`test_a_denied_head_anywhere_is_denied` is the safety direction: if the shell +would run `ssh`, the hook has to say so, however the string was folded. This is +the one that finds bypasses. + +`test_safe_commands_are_never_denied` is the usability direction: a hook that +denies `grep -n "ssh" hooks.py` teaches people to turn it off, and the person +most likely to type that command is the one auditing this file. A deny list is +allowed to over-split -- that only ever costs a false *segment* -- but it is not +allowed to over-deny. + +Both take their vocabulary from `hooks` itself, so adding an entry to +`_DENIED_COMMANDS` extends the suite rather than leaving a hole in it. +""" + +from __future__ import annotations + +import shlex + +import pytest +import shellgrammar as sg +from hypothesis import assume, given, note +from hypothesis import strategies as st + +import hooks + + +# --------------------------------------------------------------------------- +# the safety direction +# --------------------------------------------------------------------------- +@given(sg.commands(sg.DENIED_HEADS + sg.SAFE_HEADS)) +def test_a_denied_head_anywhere_is_denied(node: sg.Node) -> None: + """Whatever the shell runs, the hook sees. + + The generator folds a command into operators, subshells, brace groups, + substitutions and quotes, and remembers which heads survive as commands. + Every one of those foldings is something a person types; none of them is + the `bash -c` class the module docstring rules out of scope. + """ + assume(node.runs_denied()) + note(sg.describe(node)) + assert hooks.evaluate_bash(node.text) is not None + + +@given(sg.commands(sg.SAFE_HEADS)) +def test_safe_commands_are_never_denied(node: sg.Node) -> None: + """Nothing built from harmless heads is refused. + + The vocabulary in `shellgrammar` is chosen to keep this honest: no `rm -rf`, + no download piped into a shell, no credential read, so the only thing that + could deny here is the head rule misreading a segment. + """ + note(sg.describe(node)) + assert hooks.evaluate_bash(node.text) is None + + +@given(st.sampled_from(sg.DENIED_HEADS)) +def test_a_reserved_word_does_not_hide_the_command_after_it(denied: str) -> None: + """`if`, `do`, `then`, `!` and `time` introduce a command; they are not one. + + Left unhandled, each of these was a one-word bypass: `for h in a b; do ssh + $h; done` split cleanly on the `;` and then reported the head of + ` do ssh $h` as `do`. + + `!` and `time` are here rather than in the grammar because bash requires + both at the head of a pipeline, so they cannot be nested arbitrarily without + generating strings no shell would accept. + """ + for command in ( + f"! {denied} box", + f"time {denied} box", + f"if {denied} box; then true; fi", + f"if true; then {denied} box; fi", + f"if false; then true; else {denied} box; fi", + f"while {denied} box; do true; done", + f"until true; do {denied} box; done", + f"for h in a b; do {denied} $h; done", + ): + note(command) + assert hooks.evaluate_bash(command) is not None, command + + +@given(st.sampled_from(sg.DENIED_HEADS), st.sampled_from(sg.SAFE_HEADS)) +def test_a_denied_word_as_an_argument_is_not_a_command(denied: str, safe: str) -> None: + """`grep -n "ssh" hooks.py` is a read, not a remote execution. + + The false denial this is about was a real one: the blind split left a tail + of `b" file` for `grep -n "a\\|b" file`, and a pattern containing a denied + word was refused as though it had been typed as a command. + """ + for command in ( + f'{safe} -n "{denied}" hooks.py', + f"{safe} '{denied} box' notes.md", + f'{safe} "a|{denied}" file.txt', + f"{safe} --pattern={denied}", + ): + note(command) + assert hooks.evaluate_bash(command) is None + + +# --------------------------------------------------------------------------- +# the splitter itself +# --------------------------------------------------------------------------- +@given(sg.commands(sg.DENIED_HEADS + sg.SAFE_HEADS)) +def test_segments_invent_nothing(node: sg.Node) -> None: + """Every character of every segment came from the command. + + A splitter that emits text the caller never typed can deny a command that + was never written, and the head rule downstream would have no way to tell. + Checked as a multiset over characters rather than as substrings, because + `$(` is consumed rather than kept and the segment boundaries genuinely do + not line up with the input. + """ + segments = hooks._segments(node.text) + note(sg.describe(node)) + source = list(node.text) + for segment in segments: + for char in segment: + assert char in source, f"segment {segment!r} invented {char!r}" + source.remove(char) + + +#: Text made mostly of the characters `_segments` gives meaning to. Plain +#: `st.text()` is the wrong alphabet for a parser: it draws from the whole of +#: Unicode, so a backslash or a `$(` turns up rarely enough that the branches +#: that matter go unvisited. Mutation testing is what showed this -- eight +#: mutants inside the backslash-escape branch survived, including two that make +#: `_segments` raise IndexError on a command ending in a backslash, which no +#: generated example had ever produced. +shellish = st.text( + alphabet=st.sampled_from(list("\\'\"$(){}[]|&;`<>\n\r\t abcdefgHIJ.-/=*")), + max_size=60, +) + + +@given(st.one_of(shellish, st.text(max_size=60))) +def test_the_splitter_terminates_on_anything(text: str) -> None: + """Arbitrary text in, a list of strings out, no exception. + + `evaluate_bash` runs on whatever the model emitted, which is not + necessarily a shell command at all -- and a hook that raises is a hook that + fails open, because the SDK has nothing to do with the exception but let the + call through. A command ending in a lone backslash is the shortest way to + get there, and it is one keystroke from something a person types. + """ + segments = hooks._segments(text) + assert all(isinstance(s, str) for s in segments) + assert all(s.strip() for s in segments) + verdict = hooks.evaluate_bash(text) + assert verdict is None or isinstance(verdict, hooks.Denial) + + +@given(st.sampled_from(sg.DENIED_HEADS), st.sampled_from(["", " ", "\t"])) +def test_a_command_ending_in_a_backslash_is_still_a_command( + denied: str, trailing: str +) -> None: + """The last character is the one with no character after it. + + `_segments` consumes a backslash *and the character it escapes*, which needs + a bounds check, and the bounds check had no test: mutating `i + 1 < n` to + `i - 1 < n` or `i + 1 <= n` survived the whole suite. Both raise IndexError + here, and `evaluate_bash` raising is `pre_tool_use` raising, which the SDK + resolves by letting the call through -- so the failure mode of a missing + bounds check in a deny list is that it stops denying. + """ + for command in (f"{denied} box{trailing}\\", f"ls{trailing}\\", "\\"): + note(command) + hooks._segments(command) # must not raise + hooks.evaluate_bash(command) + assert hooks.evaluate_bash(f"{denied} box \\") is not None + + +@given(st.sampled_from(sg.DENIED_HEADS)) +def test_a_quoted_windows_path_resolves_to_its_program(denied: str) -> None: + """`"C:\\tools\\ssh.exe" box` is `ssh`, and Windows is the first-class target. + + `_head` strips a backslash-separated directory and a `.exe` suffix for + exactly this, and nothing tested either: `rsplit("\\\\", 1)` mutated to + `split("\\\\", 1)` survived the suite, which means no test had ever passed it + a path with two backslashes in it. + + Quoted, because that is the spelling that runs. Bare `C:\\tools\\ssh.exe` + is not a Windows path to the shell the hook protects -- bash eats the + backslashes and tries to execute `C:toolsssh.exe`, which is nothing -- so + `_head` declining to see a program there agrees with what would happen. + """ + for command in ( + f'"C:\\tools\\{denied}.exe" box', + f"'C:\\Program Files\\bin\\{denied}.exe' box", + f'"D:\\a\\b\\{denied}" box', + ): + note(command) + assert hooks.evaluate_bash(command) is not None, command + + +@given(st.sampled_from(sg.SAFE_HEADS), st.sampled_from(["|", ";", "&"])) +def test_an_escaped_operator_is_not_an_operator(safe: str, operator: str) -> None: + """`echo a\\;b` is one command, because the backslash took the `;` with it. + + The false-denial direction of the escape branch. Over-splitting here would + make the tail a segment of its own, and a tail that happens to start with a + denied word would be refused as though it had been typed as a command. + + One character each, because a backslash escapes exactly one: `a\\&&b` is + `a&` followed by a real `&`, and splitting it is right. The first version of + this test listed `&&` and was wrong about the shell rather than about the + code. + """ + command = f"{safe} a\\{operator}b" + note(command) + assert hooks._segments(command) == [command] + assert hooks.evaluate_bash(command) is None + + +@given(st.sampled_from(sg.DENIED_HEADS), st.sampled_from(["|", ";", "&&"])) +def test_an_escape_does_not_hide_the_next_command(denied: str, operator: str) -> None: + """...and the direction that would cost more if it were wrong. + + A backslash escapes one character. `echo \\x {op} ssh box` still has a real + operator in it, and the escape branch must not run past it. + """ + command = f"echo \\x {operator} {denied} box" + note(command) + assert hooks.evaluate_bash(command) is not None + + +@given(st.one_of(shellish, st.text(max_size=60))) +def test_the_head_of_a_segment_is_one_of_its_tokens(text: str) -> None: + """`_head` names a token that is there, lowercased and stripped of its path. + + The bug this guards against is the one fixed two commits ago in reverse: a + closing delimiter left on a token made `ssh)"` out of `ssh`. Anything that + produces a head no tokeniser would agree with is a head the deny list is + matching by accident. + """ + for segment in hooks._segments(text): + head = hooks._head(segment) + if not head: + continue + try: + tokens = shlex.split(segment, posix=True) + except ValueError: + tokens = segment.split() + stripped = { + t.rsplit("/", 1)[-1].rsplit("\\", 1)[-1].lower().removesuffix(".exe") + for t in tokens + } + assert head in stripped, f"{head!r} is not a token of {segment!r}" + + +# --------------------------------------------------------------------------- +# cost-bearing detection +# --------------------------------------------------------------------------- +@given( + st.sampled_from(hooks._COST_BEARING), + st.sampled_from(["", "cd notes && ", "true; ", "FOO=1 "]), + st.sampled_from(["", " --json", " --spec s.toml --expect e1"]), +) +def test_a_cost_bearing_command_is_recognised_however_it_is_written( + pair: tuple[str, str], prefix: str, suffix: str +) -> None: + """The budget hook's half of §15 finds the module and verb it is looking for. + + This is the token loop's *only* enforcement point -- no submitter sees a + turn -- so a spelling it fails to recognise is a ceiling that silently is + not one. + """ + module, verb = pair + command = f"{prefix}python -m {module} {verb}{suffix}" + note(command) + assert hooks.cost_bearing_command(command) == (module, verb) + + +@given(st.sampled_from(hooks._COST_BEARING)) +def test_a_cost_bearing_verb_inside_single_quotes_is_not_a_submission( + pair: tuple[str, str], +) -> None: + """Writing *about* a submit is not submitting. + + `echo 'python -m tools.jobs submit'` spends nothing, and a budget hook that + denies it is denying the agent's ability to explain what it was going to do. + """ + module, verb = pair + assert hooks.cost_bearing_command(f"echo 'python -m {module} {verb}'") is None + + +# --------------------------------------------------------------------------- +# the regex rules +# --------------------------------------------------------------------------- +@given( + st.lists(st.sampled_from(["-r", "-f", "-R", "--recursive", "--force", "-v"]), + min_size=2, max_size=4, unique=True), + st.sampled_from(["notes", "data/", "/tmp/x", "."]), +) +def test_recursive_force_delete_is_denied_in_any_flag_order( + flags: list[str], target: str +) -> None: + """`rm -r -f`, `rm -f -r`, `rm --force --recursive`: one rule, every order. + + The combined-only pattern this replaced let the separated form straight + through, which is the spelling a model writes when it is being careful. + """ + recursive = {"-r", "-R", "--recursive"} & set(flags) + force = {"-f", "--force"} & set(flags) + assume(recursive and force) + command = " ".join(["rm", *flags, target]) + note(command) + assert hooks.evaluate_bash(command) is not None + + +@pytest.mark.parametrize("empty", ["", " ", "\n", "\t\n "]) +def test_nothing_is_not_denied(empty: str) -> None: + assert hooks.evaluate_bash(empty) is None diff --git a/tests/property/test_prop_jsonl.py b/tests/property/test_prop_jsonl.py new file mode 100644 index 0000000..5569897 --- /dev/null +++ b/tests/property/test_prop_jsonl.py @@ -0,0 +1,238 @@ +"""Properties of the one write path to the ledgers. + +`core/jsonl.py` is the only place in the project that appends to a ledger, and +the ledger is the thing every other claim rests on -- "every number in a report +traces to a run record" is false the moment a record does not survive a +round-trip. Two properties the module's own docstring states, and which are +stated here as tests rather than as prose: + + * writers take an exclusive lock around each line write, so lines never + interleave; + * readers tolerate a torn final line. + +The generated records deliberately include the characters that break line-based +formats -- embedded newlines, carriage returns, tabs, non-ASCII -- because +`append` writes one record per line and `ensure_ascii=False` means the escaping +is doing real work rather than being a formality. + +Each example gets its own directory. The autouse `workspace` fixture in +`tests/conftest.py` is function-scoped, so Hypothesis would otherwise hand every +example the same `tmp_path` and let one example read the previous one's ledger. +""" + +from __future__ import annotations + +import itertools +import json +import threading +from pathlib import Path + +from hypothesis import HealthCheck, given, note, settings +from hypothesis import strategies as st + +from core import jsonl + +#: JSON-representable values, nested. `allow_nan=False` because NaN is not JSON +#: and a ledger that writes `NaN` writes a file no other reader can parse. +scalars = st.one_of( + st.none(), + st.booleans(), + st.integers(min_value=-(2**53), max_value=2**53), + st.floats(allow_nan=False, allow_infinity=False, width=64), + st.text(max_size=40), +) +records = st.dictionaries( + st.text(min_size=1, max_size=12), + st.recursive( + scalars, + lambda children: st.one_of( + st.lists(children, max_size=4), + st.dictionaries(st.text(min_size=1, max_size=8), children, max_size=4), + ), + max_leaves=6, + ), + max_size=6, +) + + +#: Module-level and never reset, which is the whole point. A counter created +#: inside the test body is re-created for every example, so every example gets +#: `ledger-1.jsonl` and reads the previous one's records -- which is exactly the +#: cross-example leak the per-example directory is supposed to prevent, wearing +#: the costume of a fix. It showed up as a round-trip test reading 242 records +#: back from six appends. +_ledgers = itertools.count() + + +def _fresh(tmp_path: Path) -> Path: + """A ledger nothing else has written to. + + Hypothesis reuses the function-scoped `tmp_path` across every example of one + test, so the isolation each example needs has to come from here. + """ + return tmp_path / f"ledger-{next(_ledgers)}.jsonl" + + +# --------------------------------------------------------------------------- +# the round trip +# --------------------------------------------------------------------------- +@given(st.lists(records, max_size=8)) +def test_what_was_appended_is_what_is_read(tmp_path: Path, rows: list[dict]) -> None: + """Every record, in order, unchanged. + + Order is part of the contract and not an accident of the filesystem: the + ledgers are append-only and read oldest-first, and `rolling_spend` and the + quota fold both depend on a record's position meaning its time. + """ + path = _fresh(tmp_path) + for row in rows: + jsonl.append(path, row) + note(path.read_text(encoding="utf-8") if path.exists() else "") + assert jsonl.read(path) == rows + + +@given(records) +def test_a_record_never_becomes_two_lines(tmp_path: Path, row: dict) -> None: + """One record, one line, whatever is in it. + + A newline inside a value would otherwise split one record into two, and the + second half would be dropped as damaged by every reader -- silently, because + the first half parses. + """ + path = _fresh(tmp_path) + jsonl.append(path, row) + text = path.read_text(encoding="utf-8") + assert text.endswith("\n") + assert text.count("\n") == 1 + + +@given(st.lists(records, min_size=1, max_size=6), st.text(max_size=20)) +def test_a_torn_final_line_costs_only_itself( + tmp_path: Path, rows: list[dict], tail: str +) -> None: + """A reader that opens the file mid-write still gets everything before it. + + This is the property the whole lock design exists to make cheap: a partial + write at the end is normal, and it must cost one record rather than the + file. + """ + path = _fresh(tmp_path) + for row in rows: + jsonl.append(path, row) + with open(path, "a", encoding="utf-8", newline="\n") as fh: + fh.write('{"partial": ' + tail) + note(path.read_text(encoding="utf-8")) + read = jsonl.read(path) + assert read[: len(rows)] == rows + + +@given(st.lists(st.booleans(), min_size=1, max_size=8)) +def test_damaged_lines_names_exactly_the_lines_that_are_damaged( + tmp_path: Path, damaged: list[bool] +) -> None: + """`ledger verify` reports the damage, so its line numbers have to be right. + + 1-indexed and counted against the file rather than against the records, or + the number it prints points a human at the wrong line. + """ + path = _fresh(tmp_path) + lines = [ + "{not json" if bad else json.dumps({"i": i}) for i, bad in enumerate(damaged) + ] + path.write_text("\n".join(lines) + "\n", encoding="utf-8") + expected = [n for n, bad in enumerate(damaged, start=1) if bad] + assert jsonl.damaged_lines(path) == expected + assert len(jsonl.read(path)) == damaged.count(False) + + +@given(st.lists(records, max_size=5)) +def test_blank_lines_are_not_damage(tmp_path: Path, rows: list[dict]) -> None: + """An empty line is skipped by both readers, and consistently. + + `read` and `damaged_lines` are separate loops over the same file, and the + one thing worse than a reader that disagrees with itself is a `verify` that + reports damage the reader silently tolerated. + """ + path = _fresh(tmp_path) + body = "\n\n".join(json.dumps(r) for r in rows) + path.write_text(body + "\n\n\n" if rows else "\n\n", encoding="utf-8") + assert jsonl.damaged_lines(path) == [] + assert jsonl.read(path) == rows + + +@given(st.lists(st.one_of(records, st.integers(), st.text(max_size=8)), max_size=6)) +def test_only_objects_are_records(tmp_path: Path, values: list) -> None: + """A bare number on a line is valid JSON and is not a record. + + Every consumer of this module indexes into what it reads, so a naked scalar + surviving the reader would be an AttributeError several frames away from the + file that caused it. + """ + path = _fresh(tmp_path) + path.write_text( + "".join(json.dumps(v) + "\n" for v in values), encoding="utf-8" + ) + assert jsonl.read(path) == [v for v in values if isinstance(v, dict)] + assert jsonl.damaged_lines(path) == [] + + +# --------------------------------------------------------------------------- +# whole-file JSON +# --------------------------------------------------------------------------- +@given(st.recursive(scalars, lambda c: st.lists(c, max_size=4), max_leaves=6)) +def test_a_json_file_round_trips(tmp_path: Path, obj: object) -> None: + path = _fresh(tmp_path) + jsonl.write_json(path, obj) + assert jsonl.read_json(path) == obj + assert not list(path.parent.glob("*.tmp*")), "a temp file survived the write" + + +@given(st.lists(st.integers(min_value=0, max_value=50), min_size=1, max_size=6)) +def test_an_update_sees_what_the_last_one_wrote( + tmp_path: Path, additions: list[int] +) -> None: + """Read-mutate-write is atomic per *update*, not merely per file. + + The records this guards are the preflight ones, which are the input to the + gate that decides whether code may cost money: a submitter folding a smoke + result while `preflight run` writes its own checks must not drop either set. + """ + path = _fresh(tmp_path) + for value in additions: + jsonl.update_json(path, lambda cur, v=value: (cur or []) + [v]) + assert jsonl.read_json(path) == additions + + +# --------------------------------------------------------------------------- +# concurrency +# --------------------------------------------------------------------------- +@settings(max_examples=15, suppress_health_check=[HealthCheck.function_scoped_fixture]) +@given(st.integers(min_value=2, max_value=6), st.integers(min_value=2, max_value=8)) +def test_concurrent_appends_never_interleave( + tmp_path: Path, writers: int, each: int +) -> None: + """The claim the lock exists for, from more than one thread. + + An OS file lock keeps *processes* apart and does nothing about threads in + one process -- the UI and an in-process CLI are exactly that case -- so the + per-path mutex is what closes it. A torn line here is a corrupted ledger, + which is unrecoverable rather than merely wrong. + """ + path = _fresh(tmp_path) + + def write(worker: int) -> None: + for i in range(each): + jsonl.append(path, {"worker": worker, "i": i, "pad": "x" * 200}) + + threads = [threading.Thread(target=write, args=(w,)) for w in range(writers)] + for t in threads: + t.start() + for t in threads: + t.join() + + assert jsonl.damaged_lines(path) == [] + read = jsonl.read(path) + assert len(read) == writers * each + for worker in range(writers): + mine = [r["i"] for r in read if r["worker"] == worker] + assert mine == list(range(each)), "one writer's records lost their order" diff --git a/tests/property/test_prop_stats.py b/tests/property/test_prop_stats.py new file mode 100644 index 0000000..62f6053 --- /dev/null +++ b/tests/property/test_prop_stats.py @@ -0,0 +1,265 @@ +"""Properties of the replication statistics. + +`core/stats.py` decides whether a run confirmed its prediction, refuted it, or +settled nothing. That verdict is the input to `Run.unjudged_deviations` and to +the evolutionary search's selection step, so an arithmetic slip here does not +show up as a crash -- it shows up as a search that prefers a lucky seed, months +later, with no way to tell which conclusions were affected. + +Statistics is unusually well suited to this kind of test, because the answers +are constrained by identities rather than by examples: a mean lies between the +extremes it was taken over, an interval straddles its centre, and shifting every +sample by a constant shifts the mean by that constant and leaves the spread +alone. None of those need a fixture, and each of them fails loudly if the +arithmetic drifts. +""" + +from __future__ import annotations + +import math + +from hypothesis import assume, given, note +from hypothesis import strategies as st + +from core import stats + +#: Bounded away from the float extremes on purpose. A metric of 1e308 is not a +#: measurement this system will ever see, and generating one only tests whether +#: `fsum` overflows -- which is a question about CPython, not about this module. +measurements = st.floats( + min_value=-1e6, max_value=1e6, allow_nan=False, allow_infinity=False +) +samples = st.lists(measurements, min_size=1, max_size=12) + + +# --------------------------------------------------------------------------- +# summarise +# --------------------------------------------------------------------------- +@given(samples) +def test_the_mean_lies_between_the_extremes(values: list[float]) -> None: + """The identity that catches a summation bug for what it is. + + A `mean` outside `[min, max]` is not a rounding artefact; it is the wrong + sum or the wrong denominator, and it would otherwise be reported as a + measurement with a straight face. + """ + summary = stats.summarise(values) + note(summary) + assert summary["min"] <= summary["mean"] <= summary["max"] + + +@given(samples) +def test_the_interval_is_centred_on_the_mean(values: list[float]) -> None: + """`ci95` is symmetric about `mean`, or absent. + + Absent at n = 1 by design -- one sample has no spread, and reporting 0.0 + would claim a precision nobody measured. + """ + summary = stats.summarise(values) + note(summary) + if summary["ci95"] is None: + assert summary["n"] == 1 + return + low, high = summary["ci95"] + assert low <= summary["mean"] <= high + assert math.isclose( + summary["mean"] - low, high - summary["mean"], rel_tol=1e-9, abs_tol=1e-12 + ) + + +@given(samples, measurements) +def test_shifting_every_sample_shifts_the_mean_and_nothing_else( + values: list[float], shift: float +) -> None: + """Translation equivariance: the location moves, the spread does not. + + This is the property that separates a real standard deviation from one + computed against a fixed origin, and it is invisible to any single example + because every example has *some* origin. + """ + before = stats.summarise(values) + after = stats.summarise([v + shift for v in values]) + note({"before": before, "after": after}) + assert math.isclose(after["mean"], before["mean"] + shift, rel_tol=1e-9, abs_tol=1e-6) + if before["sd"] is not None: + assert math.isclose(after["sd"], before["sd"], rel_tol=1e-6, abs_tol=1e-6) + + +@given(samples) +def test_the_order_of_the_samples_does_not_change_the_summary( + values: list[float], +) -> None: + """Reversing the seeds must not move the answer. + + Runs arrive in whatever order `parse_metrics` read them, which is the order + the artifact happened to be written in. A statistic that depends on it is + reporting a property of the file. + """ + forward = stats.summarise(values) + backward = stats.summarise(list(reversed(values))) + for key in ("n", "min", "max"): + assert forward[key] == backward[key] + assert math.isclose(forward["mean"], backward["mean"], rel_tol=1e-9, abs_tol=1e-9) + if forward["sd"] is not None: + assert math.isclose(forward["sd"], backward["sd"], rel_tol=1e-9, abs_tol=1e-9) + + +@given(st.lists(st.one_of(st.booleans(), st.none(), st.text(max_size=3)), max_size=8)) +def test_nothing_numeric_means_nothing_measured(values: list[object]) -> None: + """Booleans, strings and None are not measurements. + + `True` is an `int` in Python, so a flag averaged into a metric produces a + number that looks exactly like a result. `numeric` excludes them, and this + pins the consequence rather than the implementation. + """ + summary = stats.summarise(values) + assert summary["n"] == 0 + assert summary["mean"] is None + assert stats.observed_interval(summary) is None + assert stats.compare(summary, 0.0, 1.0)["in_range"] is None + + +@given(samples, st.integers(min_value=0, max_value=4)) +def test_a_nan_sample_is_dropped_rather_than_propagated( + values: list[float], where: int +) -> None: + """One NaN must not erase the other seeds. + + A run that reported NaN measured nothing, but the seeds beside it did, and + a summary that returns NaN throughout would make the whole replication + unreadable -- including its `min` and `max`, which are not in doubt. + """ + poisoned = list(values) + poisoned.insert(min(where, len(poisoned)), float("nan")) + summary = stats.summarise(poisoned) + note(summary) + assert summary["n"] == len(values) + assert not math.isnan(summary["mean"]) + + +# --------------------------------------------------------------------------- +# compare +# --------------------------------------------------------------------------- +@given(samples, measurements, measurements) +def test_a_verdict_is_one_of_three_and_agrees_with_its_reason( + values: list[float], a: float, b: float +) -> None: + """`in_range` and `relation` are two spellings of one answer. + + They are read by different callers -- `report check` looks at the relation, + `Run.unjudged_deviations` at the tri-state -- so a disagreement between them + would put a run in the pending list and out of it at the same time. + """ + low, high = min(a, b), max(a, b) + verdict = stats.compare(stats.summarise(values), low, high) + note(verdict) + pairs = { + stats.CONTAINED: True, + stats.DISJOINT: False, + stats.OVERLAPPING: None, + } + assert verdict["in_range"] is pairs[verdict["relation"]] + assert verdict["reason"] + + +@given(samples, measurements, measurements) +def test_containment_and_disjointness_are_what_they_claim( + values: list[float], a: float, b: float +) -> None: + """The verdict checked against the interval arithmetic directly. + + `compare` is the one function here whose answer a reader cannot verify by + eye, because it compares two intervals rather than a number to a range. So + it is checked against the definition instead of against a table of cases. + """ + low, high = min(a, b), max(a, b) + summary = stats.summarise(values) + observed = stats.observed_interval(summary) + assume(observed is not None) + obs_low, obs_high = observed + verdict = stats.compare(summary, low, high) + note({"observed": observed, "predicted": [low, high], "verdict": verdict}) + + if verdict["relation"] == stats.CONTAINED: + assert low <= obs_low and obs_high <= high + elif verdict["relation"] == stats.DISJOINT: + assert obs_high < low or obs_low > high + else: + assert not (low <= obs_low and obs_high <= high) + assert not (obs_high < low or obs_low > high) + + +@given(samples, measurements) +def test_a_one_sided_prediction_leaves_the_open_end_open( + values: list[float], bound: float +) -> None: + """"At least 0.8" cannot be refuted from above. + + A `None` bound is infinite rather than zero, and the difference is a whole + class of false alarm: with `low=None` read as 0.0, every negative loss would + be reported as disjoint from its own prediction. + """ + summary = stats.summarise(values) + upper_only = stats.compare(summary, None, bound) + lower_only = stats.compare(summary, bound, None) + note({"<= bound": upper_only, ">= bound": lower_only}) + observed = stats.observed_interval(summary) + assert observed is not None + if observed[1] <= bound: + assert upper_only["relation"] == stats.CONTAINED + if observed[0] >= bound: + assert lower_only["relation"] == stats.CONTAINED + assert stats.compare(summary, None, None)["relation"] == stats.CONTAINED + + +@given(samples) +def test_an_observation_is_contained_by_its_own_interval(values: list[float]) -> None: + """The reflexive case, which every other verdict is measured against.""" + summary = stats.summarise(values) + low, high = stats.observed_interval(summary) + assert stats.compare(summary, low, high)["in_range"] is True + + +# --------------------------------------------------------------------------- +# rounding, and the table +# --------------------------------------------------------------------------- +@given(samples) +def test_rounding_a_summary_changes_no_structure(values: list[float]) -> None: + """`round_summary` is cosmetic, and the ledger depends on it staying so. + + The rounded figure is what a report cites *and* what the ledger holds, so a + key dropped or a None turned into a number here is a discrepancy between a + published number and its own provenance. + """ + summary = stats.summarise(values) + rounded = stats.round_summary(summary) + assert rounded.keys() == summary.keys() + for key, value in summary.items(): + if value is None: + assert rounded[key] is None + elif isinstance(value, float): + # One unit in the last place kept, not half of one. Rounding moves a + # value by at most half a unit, but the *rounded* value then has its + # own representation error, and the two can add: 0.0234375 rounds to + # a float that is 5.000000000005e-07 away from it rather than exactly + # 5e-07. A bound of 5e-7 was a claim about decimal arithmetic in a + # test of binary floats. + assert abs(rounded[key] - value) < 1e-6 + + +@given(st.integers(min_value=1, max_value=200)) +def test_the_critical_value_falls_towards_the_normal_one(df: int) -> None: + """t is always above 1.96 and never rises with more evidence. + + A transcription error in the table -- thirty hand-typed numbers -- would + show up here as a non-monotone step, and nowhere else until an interval came + out the wrong width. + """ + assert stats.t95(df) >= 1.959 + if df > 1: + assert stats.t95(df) <= stats.t95(df - 1) + + +@given(st.integers(max_value=0)) +def test_no_degrees_of_freedom_is_not_a_number(df: int) -> None: + assert math.isnan(stats.t95(df)) diff --git a/tests/property/test_prop_submission.py b/tests/property/test_prop_submission.py new file mode 100644 index 0000000..dbfae79 --- /dev/null +++ b/tests/property/test_prop_submission.py @@ -0,0 +1,224 @@ +"""Properties of the submission hash and the overrides that feed it. + +The hash is the identity of an experiment. `core/experiments.py` stores the +resolved document in an archive and re-derives the hash from it later, possibly +on another machine and long after the spec file was edited; `core/gates.py` +looks a preflight record up by it before any money is spent. So two things have +to be true, and neither is checkable from a single example: + + * the same document always hashes the same, whatever order its keys were + built in and whatever route it took through JSON; + * different documents hash differently, or the preflight gate can be satisfied + by a dry run of something else. + +`parse_override` and `_set_dotted` are here because they are how a document +acquires the values that get hashed -- `--set lr=3e-4` has to survive as the +float `0.0003` rather than the string `"3e-4"`, since the two are different +experiments to the hash and the same experiment to a reader. +""" + +from __future__ import annotations + +import json + +from hypothesis import assume, given, note +from hypothesis import strategies as st + +from core.errors import ConfigError +from core.submission import _set_dotted, hash_resolved, parse_override + +#: TOML-shaped values: what a resolved spec document actually contains after +#: `tomllib` has read it and the overrides have been folded in. +values = st.recursive( + st.one_of( + st.none(), + st.booleans(), + st.integers(min_value=-(2**53), max_value=2**53), + st.floats(allow_nan=False, allow_infinity=False), + st.text(max_size=30), + ), + lambda children: st.one_of( + st.lists(children, max_size=4), + st.dictionaries(st.text(min_size=1, max_size=8), children, max_size=4), + ), + max_leaves=8, +) +documents = st.dictionaries(st.text(min_size=1, max_size=10), values, max_size=6) + +#: Dotted paths with no empty component. `a..b` is not a path anybody writes and +#: `parse_override` makes no promise about it. +key_paths = st.lists( + st.text(alphabet="abcdefghijklmnopqrstuvwxyz_", min_size=1, max_size=6), + min_size=1, + max_size=4, +).map(".".join) + + +# --------------------------------------------------------------------------- +# the hash +# --------------------------------------------------------------------------- +@given(documents) +def test_the_hash_is_a_function_of_the_document_and_nothing_else(doc: dict) -> None: + """Same content, same hash, however the dict was assembled. + + Python preserves insertion order and `sort_keys=True` is what makes that + irrelevant. Without it, a document built by folding overrides in a different + order would be a different experiment, and `preflight` would refuse a dry + run it had already done. + """ + shuffled = dict(reversed(list(doc.items()))) + assert hash_resolved(doc) == hash_resolved(shuffled) + assert hash_resolved(doc) == hash_resolved(doc) + + +@given(documents) +def test_the_hash_survives_the_archive(doc: dict) -> None: + """A document written to disk and read back is the same experiment. + + This is the round trip `core/experiments.py` actually performs, and the one + that would make the verifier report a mismatch for every archived run if the + canonical form and the stored form disagreed. + """ + archived = json.loads(json.dumps(doc, default=str)) + note({"original": doc, "archived": archived}) + assert hash_resolved(archived) == hash_resolved(doc) + + +@given(documents, documents) +def test_different_documents_get_different_hashes(a: dict, b: dict) -> None: + """The direction the preflight gate depends on. + + A collision means a dry run of one pipeline satisfies the gate for another, + which is the one failure this hash exists to prevent. Truncated to + `HASH_LEN`, so this is a statement about the truncation being long enough as + much as about SHA-256. + """ + assume(a != b) + assert hash_resolved(a) != hash_resolved(b) + + +@given(documents, st.integers(min_value=4, max_value=64)) +def test_a_shorter_hash_is_a_prefix_of_the_longer_one(doc: dict, length: int) -> None: + """Truncation is truncation, not a different digest. + + A record written with one length and looked up with another must still + match on the shared prefix, or the archive and the ledger disagree about + which run is which. + """ + full = hash_resolved(doc, length=None) + assert full.startswith(hash_resolved(doc, length=length)) + assert len(hash_resolved(doc, length=length)) == length + + +# --------------------------------------------------------------------------- +# overrides +# --------------------------------------------------------------------------- +@given(key_paths, values) +def test_a_json_value_survives_the_round_trip_through_an_override( + key: str, value: object +) -> None: + """`--set lr=3e-4` is the float, not the string. + + Types survive into the hash by going through JSON, so this is the property + that makes `--set epochs=10` and `--set epochs="10"` two different + experiments -- which they are. + """ + text = f"{key}={json.dumps(value)}" + parsed_key, parsed_value = parse_override(text) + note({"text": text, "key": parsed_key, "value": parsed_value}) + assert parsed_key == key + assert parsed_value == value + + +@given(key_paths, st.text(max_size=20)) +def test_a_value_that_is_not_json_stays_a_string(key: str, raw: str) -> None: + """An unquoted word is a string rather than an error. + + `--set model=resnet50` is the common case and is not valid JSON; refusing it + would make every override need quoting that the shell would then eat. + """ + try: + json.loads(raw) + except json.JSONDecodeError: + pass + else: + assume(False) + assert parse_override(f"{key}={raw}") == (key, raw) + + +@given(key_paths, st.text(max_size=10)) +def test_only_the_first_equals_separates_key_from_value(key: str, tail: str) -> None: + """`--set cmd=a=b` sets `cmd` to `a=b`. + + Splitting on every `=` would silently truncate any value containing one, + and a command line or a query string is exactly the kind of value that + does. + """ + parsed_key, parsed_value = parse_override(f"{key}=a={tail}") + assert parsed_key == key + assert parsed_value == f"a={tail}" + + +@given(st.text(max_size=20).filter(lambda s: "=" not in s)) +def test_an_override_with_no_value_is_refused_with_the_fix(text: str) -> None: + """A refusal names the thing the caller skipped, literally enough to paste.""" + try: + parse_override(text) + except ConfigError as exc: + assert "--set" in str(getattr(exc, "fix", "") or "") + else: + raise AssertionError(f"{text!r} was accepted as an override") + + +@given(key_paths, values) +def test_a_dotted_path_is_readable_back_from_where_it_was_written( + key: str, value: object +) -> None: + """`_set_dotted` and the obvious walk agree. + + The nesting is what the hash sees, so a path that writes to the wrong depth + produces a document that differs from the one the user asked for by a level + nobody looks at. + """ + target: dict = {} + _set_dotted(target, key, value) + node = target + for part in key.split(".")[:-1]: + node = node[part] + assert node[key.split(".")[-1]] == value + + +@given(key_paths, values, values) +def test_the_last_override_of_a_path_wins(key: str, first: object, second: object) -> None: + """Overrides are applied in order and the later one is the answer.""" + target: dict = {} + _set_dotted(target, key, first) + _set_dotted(target, key, second) + node = target + for part in key.split(".")[:-1]: + node = node[part] + assert node[key.split(".")[-1]] == second + + +@given(st.lists(key_paths, min_size=2, max_size=4, unique=True), values) +def test_writing_one_path_leaves_the_others_alone(paths: list[str], value: object) -> None: + """Setting `train.lr` must not drop `train.epochs`. + + A `_set_dotted` that replaced an existing dict rather than descending into + it would pass every single-key test and silently delete a sibling on the + second `--set`. + """ + # Sorted so a path is never written after something that would make it a + # non-dict: `--set a=1 --set a.b=2` is a genuine conflict and the last one + # legitimately wins. + paths = sorted(paths) + assume(not any(b.startswith(a + ".") for a in paths for b in paths if a != b)) + target: dict = {} + for path in paths: + _set_dotted(target, path, value) + note(target) + for path in paths: + node = target + for part in path.split(".")[:-1]: + node = node[part] + assert path.split(".")[-1] in node diff --git a/tests/test_hooks.py b/tests/test_hooks.py index 1a0e30f..a05e780 100644 --- a/tests/test_hooks.py +++ b/tests/test_hooks.py @@ -10,7 +10,7 @@ import pytest -from hooks import evaluate_bash, probe +from hooks import _segments, evaluate_bash, probe @pytest.mark.parametrize( @@ -106,3 +106,230 @@ def test_command_string_matching_is_not_the_security_model(): """Documented honestly: `ssh` reached through an interpreter is invisible here, which is why the credentials live in Credential Manager instead.""" assert evaluate_bash("python -c \"import subprocess; subprocess.run(['ssh','h','ls'])\"") is None + + +@pytest.mark.parametrize( + "command", + [ + r'grep -n "zzz\|pip install" file', + r'grep -rn "ssh\|scp" hooks.py', + "grep -n 'pip install|conda' notes.md", + 'echo "a | b"', + "echo 'ssh box'", + ], +) +def test_an_operator_inside_quotes_is_not_an_operator(command): + """`_segments` split on `|` without seeing quotes, so a grep for the deny + list's own vocabulary was denied as though it were the command it matched: + `grep -n "zzz\\|pip install" file` left a tail of `pip install" file` whose + head was `pip`. Anyone auditing this file writes exactly that grep.""" + assert evaluate_bash(command) is None + + +@pytest.mark.parametrize( + "command", + [ + 'echo "$(ssh gpu-box ls)"', + "echo `ssh gpu-box ls`", + 'echo "unbalanced | ssh gpu-box ls', + # An operator *inside* a double-quoted substitution. These are the ones + # the first quote-aware split let through: it emitted a split at `$(` + # and then read the body as quoted text, so the `|` never separated + # anything and every head was `echo`. + 'echo "$(echo x | ssh gpu-box ls)"', + 'echo "$(true && ssh gpu-box ls)"', + 'echo "$(true; ssh gpu-box ls)"', + 'echo "`echo x | ssh gpu-box ls`"', + 'echo "$(echo "$(ssh gpu-box ls)")"', + 'X="$(ssh gpu-box ls)"', + 'echo "$(unclosed | ssh gpu-box ls"', + # Grouping parentheses inside the body. Closing the frame at the first + # `)` rather than the matching one reopened the quote halfway through, + # putting the pipeline back inside a string the shell nonetheless runs. + 'echo "$( (true) | ssh gpu-box ls )"', + 'echo "$((true) | ssh gpu-box ls)"', + 'echo "$( ( (true) ) | ssh gpu-box ls )"', + 'echo "`(true) | ssh gpu-box ls`"', + 'echo "$( (unbalanced | ssh gpu-box ls )"', + # A body that takes no arguments: the command name is also the last + # token, so a retained closer made the head `ssh)"` rather than `ssh`. + 'echo "$(ssh)"', + "echo \"`ssh`\"", + 'echo "$(pip)"', + 'echo "$(echo "$(ssh)")"', + "echo $(ssh)", + ], +) +def test_quote_awareness_still_fails_closed(command): + """Everything the blind split denied must still be denied. + + Quoting was taught to the splitter to stop false denials, and the whole risk + of that change is in this direction: a deny list that becomes less + aggressive can only be checked by what it no longer catches. Command + substitution stays live inside double quotes, which is where one would be + hidden, and it opens a whole command context rather than a single split + point -- otherwise `"$(echo x | ssh box)"` keeps its pipeline intact and the + shell runs an `ssh` nothing ever inspected. An unbalanced quote or an + unclosed substitution falls back to the blind split, because over-splitting + costs a false denial and that is the direction this list may be wrong in.""" + assert evaluate_bash(command) is not None + + +def test_a_substitution_body_is_segmented_as_commands(): + """The mechanism behind the case above, pinned directly: the body splits on + its own operators and the text after the `)` is quoted again.""" + assert _segments('echo "$(echo x | ssh box)"') == [ + 'echo "', + "echo x ", + " ssh box", + ')"', + ] + # Single quotes suppress substitution, as the shell does, so this is one + # command and `ssh` is never a head. + assert _segments("echo '$(ssh box)'") == ["echo '$(ssh box)'"] + + +def test_a_substitution_ends_at_its_matching_parenthesis(): + """Why the frame counts grouping parentheses rather than stopping at the + first `)`, and why it cannot just treat every `)` as dangerous. + + In the first the parenthesis is grouping, the body continues, and the shell + runs the pipeline -- so the `|` has to keep splitting. In the second the + parenthesis genuinely closes the substitution and the tail is literal text + that runs nothing, so denying it would be a false denial. The two differ + only by where the closer is. + + The grouping parenthesis now ends a segment as well as deepening the frame, + which is why `(true)` arrives as `true` rather than as ` (true) `. Counting + the depth without splitting kept the *frame* right and left the *group* + unread -- `"$( (ssh box) )"` stayed one segment whose head was `(ssh`, a + bypass sitting inside the very string this test was written for.""" + assert _segments('echo "$( (true) | ssh box )"') == [ + 'echo "', + "true", + ") ", + " ssh box ", + ')"', + ] + assert evaluate_bash('echo "$( (ssh box) )"') is not None + assert evaluate_bash('echo "$(cat f) | ssh box"') is None + + +def test_the_closer_ends_a_segment_and_opens_the_next(): + """Where the closing delimiter goes, which both directions depend on. + + It cannot stay in the body's segment: a substitution that takes no + arguments is a single token, and `ssh)"` is not `ssh`, so the head never + matched and the deny list let it through. It cannot simply be dropped + either -- the quoted tail of `"$(date) ssh box"` would then head as `ssh`, + and that text is an argument to `echo` that no shell ever executes. Opening + the next buffer with it satisfies both: the body ends clean, and the tail + inherits a head that matches nothing.""" + assert _segments('echo "$(ssh)"') == ['echo "', "ssh", ')"'] + assert evaluate_bash('echo "$(ssh)"') is not None + assert _segments('echo "$(date) ssh box"') == ['echo "', "date", ') ssh box"'] + assert evaluate_bash('echo "$(date) ssh box"') is None + + +@pytest.mark.parametrize( + "command", + [ + "(ssh gpu-box nvidia-smi)", + "( ssh gpu-box nvidia-smi )", + "((ssh gpu-box ls))", + "true; (ssh gpu-box ls)", + "echo hi && (pip install torch)", + "echo hi | (kaggle competitions list)", + "{ ssh gpu-box ls; }", + "{ conda install pytorch; }", + "true && { hf jobs run img cmd; }", + # Grouping inside a substitution, which had the same hole one level down. + 'echo "$( (ssh gpu-box ls) )"', + ], +) +def test_grouping_starts_a_command(command): + """`( cmd )` and `{ cmd; }` run `cmd`, and neither used to reach the head rule. + + Found by the generated suite in `tests/property`, which shrank it to + `( conda )` -- three tokens, no quoting, no substitution, and the shortest + bypass this list ever had. The parser already counted grouping parentheses + *inside* a substitution frame, to find the matching closer; it just never + treated one as the start of a command, so the head of `( ssh box )` was `(` + and matched nothing. + + A brace only counts where a blank follows it, which is where the shell reads + it as the group reserved word rather than as brace expansion or a parameter + -- see `test_a_brace_that_is_not_a_group_is_not_a_separator`.""" + assert evaluate_bash(command) is not None + + +@pytest.mark.parametrize( + "command", + [ + "! ssh gpu-box ls", + "time ssh gpu-box ls", + "if ssh gpu-box ls; then true; fi", + "if true; then ssh gpu-box ls; fi", + "if false; then true; else pip install torch; fi", + "if false; then true; elif true; then kaggle kernels push; fi", + "while ssh gpu-box ls; do true; done", + "until true; do scp a gpu-box:/b; done", + "for h in a b; do ssh $h nvidia-smi; done", + ], +) +def test_a_reserved_word_is_not_the_command_it_introduces(command): + """`do`, `then`, `else`, `if`, `!` and `time` are grammar, not programs. + + `for h in a b; do ssh $h; done` split cleanly on its semicolons and then + reported the head of ` do ssh $h` as `do`. Skipping these in `_head` is + finishing the parse rather than widening the rule -- which is why `sudo`, + `nohup`, `env` and `exec` are deliberately *not* skipped. Those are programs + that run other programs, the indirection class this module's docstring puts + out of scope alongside `bash -c`.""" + assert evaluate_bash(command) is not None + + +@pytest.mark.parametrize( + "command", + [ + "rm --recursive -f notes", + "rm -r --force notes", + "rm -f --recursive notes", + "rm --force -r notes", + ], +) +def test_a_recursive_delete_is_denied_with_mixed_flag_spellings(command): + """Six alternations covered short-with-short and long-with-long, and nothing + covered one of each. + + `rm --recursive -f notes` matched none of them -- and it is the spelling + somebody writes when they are being explicit about the dangerous half. Two + lookaheads now say what the rule means, "recursive appears and force appears + in this command", which is order-free and spelling-free by construction + rather than by enumeration.""" + assert evaluate_bash(command) is not None + + +@pytest.mark.parametrize( + "command", + [ + r"find . -name '*.py' -exec grep -l ssh {} \;", + "echo ${HOME}/notes", + "awk '{print $1}' data/results.tsv", + "python -c \"print({'a': 1})\"", + "echo {1..5}", + # Not recursive, so not this rule's business however forceful it is. + "rm -f figures/001.png", + "rm --force figures/001.png", + "rm -r notebooks/scratch", + "rm -iv notes/old.md", + ], +) +def test_a_brace_that_is_not_a_group_is_not_a_separator(command): + """The cost of the two fixes above, held to zero. + + Splitting on every `{` would have denied `find ... -exec grep ssh {} \\;`, + and matching `rm` plus any `-r`-ish flag would have denied every single-file + delete. Both are commands this agent runs constantly, and a deny list that + cries wolf on them is one somebody turns off.""" + assert evaluate_bash(command) is None diff --git a/tests/test_stats.py b/tests/test_stats.py index 02b0e9c..9c312a7 100644 --- a/tests/test_stats.py +++ b/tests/test_stats.py @@ -49,6 +49,40 @@ def test_the_sample_standard_deviation_is_used_not_the_population_one(): assert summary["sd"] == pytest.approx(math.sqrt(5.0 / 3.0)) +@pytest.mark.parametrize( + "value", [3.05, 0.95, 0.1, 699050.7704552475, 1e-7, 12345.6789] +) +def test_seeds_that_agreed_exactly_report_no_spread(value): + """Three runs that reported the same number measured a spread of zero. + + They did not. `fsum` returns the exactly-rounded sum and the division by n + rounds once more, and that second rounding can land a unit in the last place + *outside* the samples -- so `[3.05, 3.05, 3.05]` summarised to a mean of + 3.0499999999999994, which is below its own minimum, and an sd of 5e-16. + + The mean being outside `[min, max]` is the visible half and the smaller one: + a report citing both looks like a corrupt ledger, but the number is right to + fifteen places. The half that matters is the spread. This module exists + because a result "was recorded as in-range with identical confidence whether + the run-to-run spread was 0.001 or 0.3" -- so zero spread, which is what + identical seeds measured, is the one reading it must not invent noise for. + + Found by `tests/property/test_prop_stats.py`, which asserts + `min <= mean <= max` over generated samples; it took 2000 examples to find a + triple where the rounding goes the wrong way. + """ + summary = stats.summarise([value] * 3) + assert summary["min"] <= summary["mean"] <= summary["max"] + assert summary["mean"] == value + assert summary["sd"] == 0.0 + assert summary["sem"] == 0.0 + assert summary["ci95"] == [value, value] + # And the verdict that rests on it: a point interval, contained by any + # prediction that contains the point. + assert stats.observed_interval(summary) == (value, value) + assert stats.compare(summary, value, value)["in_range"] is True + + def test_junk_samples_are_excluded_rather_than_averaged(): assert stats.numeric([1.0, "x", None, True, float("nan"), 2.0]) == [1.0, 2.0] # A quantity whose every sample is unusable has nothing to summarise. diff --git a/tests/test_ui_tokens.py b/tests/test_ui_tokens.py index 7d8cdda..1bea096 100644 --- a/tests/test_ui_tokens.py +++ b/tests/test_ui_tokens.py @@ -244,6 +244,22 @@ def test_the_switch_is_one_attribute_and_ships_in_the_same_sheet(): assert sheet.count("--grad-handle:") == 1 +def test_every_palette_declares_the_scheme_the_browser_draws_in(): + """The scrollbars were the one part of the inversion no token could reach. + + Everything else here dresses something the app renders; a scrollbar is drawn + by the browser, which takes its colour from `color-scheme` alone -- so + without this the dark palette kept light scrollbars on every pane that + overflowed. It is emitted from the *colour* half on purpose: the non-colour + half ships once, with the light palette, and a declaration there would pin + every theme to `light` with no way to override it.""" + for name in tokens.PALETTES: + assert name in tokens.COLOR_SCHEME, name + assert f"color-scheme: {tokens.COLOR_SCHEME[name]};" in tokens.colour_variables(name) + # One per palette in the shipped sheet: `:root` and the dark override block. + assert tokens.stylesheet().count("color-scheme:") == len(tokens.PALETTES) + + def test_an_unknown_theme_falls_back_rather_than_failing(): """What a settings file written by a newer version looks like from an older one. The answer is the design's default, not a stylesheet that will not diff --git a/tests/test_ui_turns.py b/tests/test_ui_turns.py index c427674..4d646d4 100644 --- a/tests/test_ui_turns.py +++ b/tests/test_ui_turns.py @@ -19,6 +19,7 @@ class does with a client that behaves in a particular way -- so the client here from __future__ import annotations import asyncio +import time import pytest @@ -311,8 +312,14 @@ def make_client(options=None): session = app.Session("race") starting = asyncio.create_task(session.start()) + # Real time rather than bare event-loop ticks: `start` now does its blocking + # half (the SDK import, the config read, the credential-store hydrate) on a + # worker thread, so waiting for the client means waiting for a thread to be + # scheduled and not merely for the loop to come round again. Two hundred + # `sleep(0)`s take microseconds and used to be plenty; they are not a wait + # at all once anything real happens off the loop. for _ in range(200): - await asyncio.sleep(0) + await asyncio.sleep(0.01) if made: break assert made, "the start task never built a client" @@ -331,6 +338,72 @@ def make_client(options=None): assert session.client is None +async def test_a_cold_start_leaves_the_event_loop_free(monkeypatch): + """The prompt has to reach the browser before the turn starts working. + + As reported: send the first message of a session and it takes five to seven + seconds to appear in the chat, which reads as a broken app -- the composer + has already cleared, so there is nothing on screen at all. + + Nothing was slow in the way that phrasing suggests. NiceGUI draws an element + by queueing it and letting a per-client outbox task emit it when the event + loop next runs something else, and between the composer drawing the prompt + and the SDK subprocess there was nothing for the loop to run: `_stopped` + returns without awaiting when no interrupt is pending, `apply_effort` and + `apply_model` both return early while the client is None, and `start` then + imported the SDK (two seconds), read the config and hydrated the credential + store -- all synchronously, all on the loop. + + So the property is not "the cold start is fast". It is "the cold start does + not stop the loop", which is what lets the queued prompt go out. Measured + here by a ticker that counts how often it gets to run; before the fix, the + answer was zero. + """ + import claude_agent_sdk + import agent as agent_mod + from core import config as config_mod + + class _Client: + async def __aenter__(self): + return self + + async def __aexit__(self, *_): + return False + + monkeypatch.setattr(claude_agent_sdk, "ClaudeSDKClient", lambda options=None: _Client()) + monkeypatch.setattr(agent_mod, "build_options", lambda cfg, **_: object()) + # Both halves of the cold start, kept synchronous -- which is what they are + # -- and made slow enough to measure. `time.sleep` rather than + # `asyncio.sleep` on purpose: a coroutine that awaits is not the thing this + # test is about. + monkeypatch.setattr(agent_mod, "preflight_environment", lambda: time.sleep(0.25)) + # The real config, read once before the patch, so what `start` records + # afterwards (`client_effort`, `client_model`) comes off a real object. + loaded = config_mod.load() + monkeypatch.setattr(config_mod, "load", lambda *a, **k: loaded) + + ticks = 0 + + async def ticker() -> None: + nonlocal ticks + while True: + await asyncio.sleep(0.01) + ticks += 1 + + counting = asyncio.create_task(ticker()) + session = app.Session("cold") + try: + await session.start() + finally: + counting.cancel() + + assert session.client is not None + assert ticks >= 5, ( + f"the loop ran {ticks} times during a 250 ms cold start; " + "anything queued for the browser is stuck until it finishes" + ) + + async def test_a_release_waits_out_a_socket_blip(monkeypatch): """`_release_when_gone`: a socket that comes back keeps its session.""" monkeypatch.setattr(app, "RELEASE_GRACE_S", 0.05) diff --git a/ui/app.py b/ui/app.py index 5ae0023..6fb2730 100644 --- a/ui/app.py +++ b/ui/app.py @@ -185,20 +185,54 @@ def __init__(self, key: str = "default") -> None: #: deep in the SDK. self._lifecycle = asyncio.Lock() + @staticmethod + def _cold_start(agent: Any) -> tuple[Any, Any]: + """The blocking half of building a client. Runs on a worker thread. + + Nothing in here is slow enough to notice on its own. Together they were + the reason a fresh prompt took five to seven seconds to appear on + screen, and the reason is *when* they run rather than how long they + take: this is the first thing a turn does, the event loop has not + yielded since the composer drew the prompt, and NiceGUI's outbox cannot + send a queued element while the loop is busy. So the prompt sat in the + queue for the whole cold start, with the composer already cleared and + nothing on screen to show it had been sent. + + Measured on the machine this was written on: importing the SDK is two + seconds, `config.load` is eighty milliseconds, and + `preflight_environment` is another three hundred -- most of that the + credential store's backend coming up, which is a blocking COM call on + Windows with no async form to prefer. + + The SDK import is *inside* this function rather than at the call site + for the same reason it was lazy before -- the UI has to load on a + machine without it -- and it stays a `from ... import`, executed per + call, so a test that patches `claude_agent_sdk.ClaudeSDKClient` still + gets its double. + + `preflight_environment` scrubs and hydrates `os.environ`, which is + process-global rather than loop-local, so a thread is a legitimate place + for it. The lock the caller holds is what keeps two of these from + overlapping. + """ + from claude_agent_sdk import ClaudeSDKClient # noqa: PLC0415 + + cfg = config_mod.load() + agent.preflight_environment() + return cfg, ClaudeSDKClient + async def start(self) -> None: if self.client is not None: return import agent # noqa: PLC0415 - imported here so the UI can load without the SDK - from claude_agent_sdk import ClaudeSDKClient # noqa: PLC0415 async with self._lifecycle: # Re-checked under the lock: a start that waited here waited on # another start, and the client it built is the one to use. if self.client is not None: return - cfg = config_mod.load() - agent.preflight_environment() - await self._connect(agent, ClaudeSDKClient, cfg) + cfg, client_cls = await asyncio.to_thread(self._cold_start, agent) + await self._connect(agent, client_cls, cfg) # Consumed, not kept. A rewind aims at one rebuild; leaving these set # would have the *next* one -- an effort change, an interrupt, a # model switch -- silently truncate the conversation again, at a diff --git a/ui/render.py b/ui/render.py index 01dbb65..4efbc18 100644 --- a/ui/render.py +++ b/ui/render.py @@ -161,6 +161,11 @@ def _document(name: str, body: str, theme: str | None = None) -> str: return f"""