diff --git a/.harness/config.yaml b/.harness/config.yaml index 881ac5c..e51ca28 100644 --- a/.harness/config.yaml +++ b/.harness/config.yaml @@ -16,11 +16,11 @@ standards_dir: .harness/standards architecture_docs: - .harness/docs/ARCHITECTURE.md test_command: .venv/bin/python -m pytest tests/ -q -# The interpreter the clean-clone check runs the suite under. Deliberately -# the oldest Python CI tests (3.10), not the one the harness runs in (3.14), -# so a version incompatibility is found before CI rather than by it. Absent, -# the check falls back to test_command's own interpreter. -clean_clone_python: .venv310/bin/python +# The executable the clean-clone check runs the suite under. Deliberately an +# older environment than the one the developer works in, and the one CI +# exercises, so an incompatibility is found before CI rather than by it. It +# replaces the first word of test_command; absent, that first word is used. +verification_runner: .venv310/bin/python # Bash commands stage agents may run without prompting. Everything else # is denied in headless mode. Read-only search and inspection is granted # broadly: a denial costs a turn and buys nothing, because the harness's diff --git a/.harness/docs/ARCHITECTURE.md b/.harness/docs/ARCHITECTURE.md index b8112ef..25c9ef6 100644 --- a/.harness/docs/ARCHITECTURE.md +++ b/.harness/docs/ARCHITECTURE.md @@ -60,10 +60,12 @@ Since story-032 the `create` value is **a path at or beneath** one of that stage `technical_plan` deliberately carries no `type` keyword. story-001 and story-002 write it as a free-form block scalar and story-003 onward write it as a structured object; the validator subset has no union keyword, and those artifacts are committed and unedited. Omitting `type` keeps every nested constraint biting — `schema_validator` applies `properties`/`required` only when the value is a dict — so a malformed *object* form is still rejected while a block scalar is accepted. The reason is recorded in that property's own `description`. -One file in the inventory declares a *contract* rather than an artifact shape. Since story-039 the set of configuration keys the harness reads is declared in **`schemas/harness-config.schema.json`** — one property per key, thirteen of them (`allowed_tools`, `architecture_docs`, `base_branch`, `branch_prefix`, `clean_clone_python`, `logs_dir`, `model`, `permission_mode`, `runs_dir`, `standards_dir`, `stories_dir`, `test_command`, `workflow`), each typed as `load_config` produces it and described by what it governs and what it falls back to. It is in `schemas/manifest.json` like every other file there, and passes the same parametrized draft-2020-12, unsupported-keyword and no-`additionalProperties` checks, but no stage is asked to satisfy it because no agent produces a config file. `harness_config.declared_config_keys(harness_root=None)` is its **only reader**, resolving it relative to its own module through `schema_validator.load_schema` so `schemas/` keeps one reader, and raising `ValueError` naming the path on a missing, unparseable or wrong-shaped schema rather than degrading to an empty or partial tuple — a degraded return would make the coverage below vacuous instead of red. +One file in the inventory declares a *contract* rather than an artifact shape. Since story-039 the set of configuration keys the harness reads is declared in **`schemas/harness-config.schema.json`** — one property per key, thirteen of them (`allowed_tools`, `architecture_docs`, `base_branch`, `branch_prefix`, `logs_dir`, `model`, `permission_mode`, `runs_dir`, `standards_dir`, `stories_dir`, `test_command`, `verification_runner`, `workflow`), each typed as `load_config` produces it and described by what it governs and what it falls back to. It is in `schemas/manifest.json` like every other file there, and passes the same parametrized draft-2020-12, unsupported-keyword and no-`additionalProperties` checks, but no stage is asked to satisfy it because no agent produces a config file. `harness_config.declared_config_keys(harness_root=None)` is its **only reader**, resolving it relative to its own module through `schema_validator.load_schema` so `schemas/` keeps one reader, and raising `ValueError` naming the path on a missing, unparseable or wrong-shaped schema rather than degrading to an empty or partial tuple — a degraded return would make the coverage below vacuous instead of red. The declaration is **not a run-time check**. Nothing calls `declared_config_keys` while a run executes, no target's `.harness/config.yaml` is validated against it, and no unknown key is refused — a target carrying an extra key runs exactly as it did before the file existed. `project` is the live example of a key a config file carries and nothing reads, and it is deliberately undeclared: the declared set *is* the set of keys the harness reads, so a key nothing reads is out of the set by construction rather than by oversight. +**A *retired* key is the one exception, and it refuses rather than being ignored.** Since story-041 `harness_config` also declares `RETIRED_CONFIG_KEYS`, a mapping from a name the harness once read to the name that replaced it, and `retired_config_problems(config)` returns one problem per retired key a loaded config still carries, naming both. `run_story` calls it immediately after `load_config` and refuses through the shared `refuse()` — **above** the workflow load and above the routing pre-flight, so a refused run creates no run directory, no state file, no log, no branch, and invokes no agent. The reasoning is the difference between a key nothing has ever read and a key that was read yesterday: an unknown key is inert, but a config still carrying `clean_clone_python` after the rename would fall through to a *different* fallback and quietly change what the clean-clone check exercises. Silently accepting it as the new key is worse still — it would make the rename undiscoverable. Declaring the mapping beside `declared_config_keys` keeps the config vocabulary, what is read and what used to be, in one module. The refusal message is composed from the mapping rather than written out, so retiring the next key is a one-line edit and adds no prose to the coordinator. + What the declaration is for is coverage, and the coverage is **set equality in both directions**, twice, in `tests/test_config_keys_are_obeyed.py`. Against `KEY_PROOFS` — a key declared with no proof fails, a proof naming an undeclared key fails — and against an AST scan of `orchestration/` and `scripts/` collecting every `config.get("...")` and `config["..."]`, extensionless scripts included, so a key the harness reads and the schema does not declare fails. Neither comparison is against a second maintained list. Every declared key carries a proof that **varies** it: the value is one the harness would never pick (each carries the token `xyzzy`, and an assertion checks that none coincides with the key's default or with this repository's own configured value), and ten keys are proven behaviourally while `model`, `permission_mode` and `allowed_tools` are proven on the invocation built for a fake runner, because those three are handed to the agent runner and observable nowhere else. `KEY_PROOFS` records which of the two each key gets, so a reader knows what *proven* means for it. A mutation control then replaces each key's read with its fallback literal in a throwaway copy of `orchestration/` and requires that key's proof to go red there, so a proof that sets a key and asserts nothing about its effect cannot survive. This subsumes but does not retire story-028's absence assertions: proving a literal is *absent* and proving the configured value *governs* are different claims, and both are kept. ### Prompts (`prompts/`) @@ -137,7 +139,7 @@ Outliving the session is what made **plan-time validation** possible, and story- ### Target-repository state (`.harness/`) -- `config.yaml` — repository-specific settings (branch prefix, model, permission mode, workflow name, `test_command`, and `clean_clone_python`), plus the optional `base_branch` key story-030 added, shipped as a commented example. The keys the harness reads are declared in `schemas/harness-config.schema.json` and each is proven to govern (see the `schemas/` section); the file itself is validated against nothing at run time. Leaving `base_branch` unset is the normal case: the base is then the repository's own default branch, read from `refs/remotes/origin/HEAD`. Set it only where the base is not the git default. +- `config.yaml` — repository-specific settings (branch prefix, model, permission mode, workflow name, `test_command`, and `verification_runner`), plus the optional `base_branch` key story-030 added, shipped as a commented example. The keys the harness reads are declared in `schemas/harness-config.schema.json` and each is proven to govern (see the `schemas/` section); the file itself is validated against nothing at run time, except that a retired key refuses the run (see the retired-key paragraph in the `schemas/` section). Leaving `base_branch` unset is the normal case: the base is then the repository's own default branch, read from `refs/remotes/origin/HEAD`. Set it only where the base is not the git default. - `standards/` — repository standards (architecture, coding, testing) that verifiers evaluate against. - `stories/` — approved story artifacts produced by `l5-plan`, and validated, committed and pushed by it (see the `plan_validation.py` and `plan_commit.py` bullets above). The directory is what `snapshot` watches; its location comes from the config's `stories_dir`, defaulting to `.harness/stories`. - `runs//` — per-run state, events, and artifacts (not committed). @@ -419,12 +421,14 @@ The harness runs against any repository. Its only tie to a target's language, to **The rule is a scan, not a paragraph, and the choice has a record.** Two rules of exactly this shape were written down here before: the `git diff HEAD` baseline rule was documented *and* injected into every stage prompt and shipped five more times, and the git-history-loader rule was documented and shipped three more times. Both stopped when a scan landed. Two of the five ties also sit in a prompt, which no fixture reaches — `prompts/tester.md` is prose an agent reads — so "every configurable value is proven configurable" could vary every key in existence and still not notice a pytest filename sitting in it. Only a reader or a scan sees that, and readers had missed it since the file was written. -**Where the two halves live.** The declaration and the scan are `orchestration/harness_source.py` (above); the judgement is `tests/test_no_target_stack_in_harness_source.py`. The module reports mentions and judges none of them — the test module holds **two allowlists**, keyed by repository-relative path and the exact text of the matched line rather than by line number, so an unrelated edit above a tie does not churn the list and read as burn-down. `TEMPORARY_TIES` (36 entries) holds the mentions that name or assume a target's stack or layout. `PERMANENT_MENTIONS` (9 entries) holds the mentions that are the opposite of a tie — a docstring saying every scalar `story_parser` produces is a Python `str` is a fact about this code, and the coordinator docstring saying a target's test command need not be a Python interpreter is the harness declining the assumption — and **every entry carries a one-line reason**, so the permanent half is a judgement on the record rather than a suppression list. The two together equal exactly what the scan reports against this repository (45 distinct `(path, line text)` pairs from 46 findings — `prompts/tester.md:47` matches both rules), asserted in both directions and asserted disjoint. The classification rule sits in the module docstring beside the declaration so the next one is not re-argued. +**Where the two halves live.** The declaration and the scan are `orchestration/harness_source.py` (above); the judgement is `tests/test_no_target_stack_in_harness_source.py`. The module reports mentions and judges none of them — the test module holds **two allowlists**, keyed by repository-relative path and the exact text of the matched line rather than by line number, so an unrelated edit above a tie does not churn the list and read as burn-down. `TEMPORARY_TIES` (3 entries since story-041, 36 before it) holds the mentions that name or assume a target's stack or layout. `PERMANENT_MENTIONS` (9 entries) holds the mentions that are the opposite of a tie — a docstring saying every scalar `story_parser` produces is a Python `str` is a fact about this code, and `RETIRED_CONFIG_KEYS` spelling a retired key is the harness *refusing* the tie rather than carrying one — and **every entry carries a one-line reason**, so the permanent half is a judgement on the record rather than a suppression list. The two together equal exactly what the scan reports against this repository (12 distinct `(path, line text)` pairs from 13 findings — `prompts/tester.md:47` matches both rules), asserted in both directions and asserted disjoint. The classification rule sits in the module docstring beside the declaration so the next one is not re-argued. -**Why they are two lists and not one.** `TEMPORARY_TIES` reaching empty is the completion signal for `.harness/requests/the-interpreter-is-not-assumed-to-be-python.md` and `.harness/requests/the-test-location-comes-from-configuration.md` — the two stories queued behind this one, which fix the ties this one grandfathers. The artifact-schema mentions of `python`, `python_version` and `clean_clone_python` in `schemas/clean-clone-result.schema.json`, `schemas/revert-check-result.schema.json` and `schemas/harness-config.schema.json` are on the temporary list for that reason: the interpreter story names those schemas and they burn down with it. Merge the two lists and the signal is gone — a list that stops shrinking cannot be told from work that finished. +**Why they are two lists and not one.** `TEMPORARY_TIES` reaching empty is the completion signal for `.harness/requests/the-interpreter-is-not-assumed-to-be-python.md` and `.harness/requests/the-test-location-comes-from-configuration.md` — the two stories queued behind story-040, which fix the ties it grandfathers. Merge the two lists and the signal is gone — a list that stops shrinking cannot be told from work that finished. **story-040 fixes none of the five**, deliberately. `orchestration/story_coordinator.py`, `workflows/story-workflow.json` and `prompts/tester.md` carry no edit on its branch, and no run-time behaviour changed. +**The burn-down has since run once, and it removed exactly its own entries.** story-041 took off every `orchestration/story_coordinator.py` entry — the version probe, the record's interpreter-shaped fields, and the retired configuration key with the prose explaining it — together with every entry in `schemas/clean-clone-result.schema.json`, `schemas/revert-check-result.schema.json` and `schemas/harness-config.schema.json`. **What remains temporary is the two `prompts/tester.md` lines and the `workflows/story-workflow.json` restriction, and nothing else**, which is the completion signal for `the-test-location-comes-from-configuration` alone. The coordinator came off that module's untouched tuple and both of its entries came off `AUDITED_TIES` — asking the scan for a tie by name only works while the tie is there. In their place `REPAIRED_SCHEMAS` and `test_the_repaired_schemas_carry_no_mention_at_all` assert the scan reports *nothing* in the three schemas, and `test_the_coordinator_carries_no_temporary_tie` asserts the same for the coordinator in the weaker form the surviving permanent mention allows. Both read the claim off the scan rather than off the diff, so an entry removed while its mention is still in source goes red. One mention does survive in the coordinator, and deliberately: `self_route_problems`'s docstring naming the language *this harness* is written in, which the classification rule calls the opposite of a tie. + **What the scan does not catch, stated where a reader meets it.** `STACK_TOKENS` is a guess about languages nobody has tried and is incomplete by construction — a Ruby `Gemfile` or an Elixir `mix.exs` is reported by nothing here, which the suite demonstrates by planting both and finding them invisible. It is worth having anyway because it catches the *shape* of the mistake, which recurs, rather than every instance of it. The layout half **cannot read `orchestration/` at all**: this repository's own suite is called `tests/`, so in Python source a `tests/` literal cannot be told from an honest reference to it, and the half therefore reads only the target-facing files where a path can only mean a target's. `.harness/` and `tests/` are outside the scan entirely. The module is exempt from its own scan **by name and by nothing else** — the token list would otherwise report itself — and a second file carrying the same tokens is reported. And the scan is not tamper-proof: deleting it alongside a forced repair is not caught here at any granularity, and no assertion in the module implies otherwise. Every one of those limits is asserted by reading the module's text rather than by trusting it to be there. **The matcher's boundary is "not a letter or a digit", not `\b`.** That is what makes `clean_clone_python` and `platform.python_version()` visible — `_` and `.` are not alphanumeric — while `pipeline` and `pipe` are not, because `e` is; `\b` inverts both. The alternation is longest-token-first so `python3` claims its own match rather than being read as `python` followed by a digit, which the boundary would then reject and hide entirely. The layout pattern keeps the left boundary and drops the right one, because each path already ends in a slash and requiring a non-alphanumeric after it would hide `tests/conftest.py` — the exact shape being looked for. All four constructed cases are run through `scan` rather than reasoned about. @@ -543,7 +547,9 @@ It is narrower than Chapter 18's **checkpoints**, and the difference is the reas - **The transport's cost is measured, not characterised: 0.34 s → 0.53 s per clone on this repository** (`+0.19 s`, 1.56×), five `_build_clone` runs per command end to end — clone plus working-tree apply plus untracked copy plus commit — differing in exactly that one argument. The implementer and the tester measured it independently and agreed to within 0.01 s (0.34 s → 0.54 s); the script is `.harness/runs/story-033/measure-clone-transport.py` and the record is `clone-transport-measurement.md` beside it. Two clones can be built per story run — the clean-clone check and the revert check — so the worst case this adds is about 0.4 s, against a suite that takes five to six minutes. Quote the number when the trade comes up again; the ranges do not overlap, so it is the transport rather than noise. - **The CI retry in `.github/workflows/tests.yml` is a backstop for the next unknown, not this problem's solution.** It was added while the cause was unestablished, retries only the failed tests and only once, and it never made the clone stop touching the source's object files — it made an already-broken clone get a second chance. story-033 removes the failure mode; the retry stays because the next unexplained flake will not be this one. Do not read a green retry as evidence that a clone-time failure was transient, and do not reach for a retry when the failing operation can be made incapable of failing instead. Nothing under `.github/` was changed by this story. - A clean-clone failure reroutes rather than escalating. The suite failing where the code ships is a defect in the implementation, which is what a retry addresses, so it reuses the verification-failed branch's whole sequence — archive above the increment, increment `retry_count`, save, one event, reroute to the stage its own declaration names — and the existing escalation path at the ceiling. No new `RunState` field and no second retry axis, matching how every other routing decision here is kept to one. It borrowed the verifier's `on_failure.retry_stage` until story-028 deleted that constant; the destination now comes off the widened `clean_clone` declaration, which is the same fact moved to the key that turns the check on rather than a new one. -- A configured `clean_clone_python` that does not resolve escalates naming it, rather than falling back to the harness's interpreter. A check that quietly tests the wrong version is worse than one that refuses. The key exists because nothing local otherwise exercises a version CI does — the developer's venv is 3.14 while CI tests 3.10, 3.11 and 3.12 — and the record carries the interpreter and the version it reported so a reader can tell which Python the check exercised rather than assuming. A *fallback* interpreter reporting no recognizable version is not an error: the configured test command need not be Python at all, so the record simply carries no `python_version` there. +- **The key names a role, not a language: `verification_runner` is the executable the check runs the configured test command under.** It was `clean_clone_python` until story-041, which is a name that told orchestration what the target is written in across nine coordinator sites. What it actually names is the *first word* of `test_command`, whichever executable that is: the check substitutes the configured runner for that word and passes every remaining argument through untouched. Unset, the command runs exactly as written, which is what a target built from a compiled toolchain wants. **The limit is stated where a reader meets it** — in the schema description and in `run_clean_clone`'s docstring — because it is real: this is a first-word swap and nothing more, and a target whose environment difference cannot be expressed that way expresses it in `test_command` instead. The substitution was kept rather than widened into a second command key, which would have given two places to say the same thing. +- A configured `verification_runner` that does not resolve escalates naming it, rather than falling back to the harness's own executable. A check that quietly exercises the wrong environment is worse than one that refuses. The key exists because nothing local otherwise exercises the environment CI does — this repository's developer venv is a newer release than the ones CI tests — so `_resolve_interpreter` is kept exactly as it was: it is what makes a configured runner that does not exist refuse rather than degrade. +- **The record carries the runner and no version, and the version's removal is the point.** The record's field is `runner`, and until story-041 it was `python` alongside a `python_version` the coordinator obtained by *executing a snippet of a language's source* from orchestration. Nothing routed on that version; it was written into `clean-clone-result.json` and `revert-check-result.json` and read only by a person looking at a finished run, and a target whose suite is not Python got an empty field and no explanation. What the version bought over the path is genuine — a path is a name and a version is a fact, and a venv rebuilt on a newer release is still called `.venv310` — and it is still not worth executing a language from orchestration, nor worth a configured probe key and the obedience proof that key would owe. **Prefer deleting a field nothing routes on to making it portable**: the interpreter path already answers "which one ran", which was the question. The probe, its regular expression, the function around it and the `import re` that existed only for it are all gone. - story-014's own run was not governed by the check it adds, for the reason story-007 hit with `may_not_create`: the coordinator loads the workflow definition at run start, before the `clean_clone` key existed in it. Enforcement begins with the next story. Expect this of any story that adds an enforcement rule, and say so in its constraints rather than treating the gap as a defect. story-017 is the third instance, with `revert_check`; three is enough to call it the standing shape rather than a recurrence. - **The line the harness draws under `tests/` is not a path prefix, it is reverting.** Four story artifacts asserted in prose that the implementer's record lists nothing under `tests/`. The harness enforced something narrower — `may_not_create`, creation only — and never escalated on the difference, so the prose added no enforcement while being sometimes impossible to satisfy: a legitimate implementer change can break an existing test and the suite has to stay green. Every finding the sentence produced was a deviation from prose rather than a defect (story-011 had to add a schema to two inventories asserting exact set equality, story-013 could not carry the rule at all because deleting those inventories was its deliverable, story-012 shipped a schema that turned five story-013 assertions red and recorded the deviation as not clearable by a retry). Three separate causes, each removed by its own story, each followed by another, and nobody could enumerate the next. The distinction being reached for was never "must not touch `tests/`" but "must not author its own validation", and a path prefix conflates two acts: authoring coverage, which must not happen in the implementer, and keeping existing validation runnable, which is maintenance. Reverting separates them exactly and with no judgement — **maintenance is by definition the edit without which the suite fails** — so an edit under a governed prefix is permitted iff reverting it makes the suite fail. Do not reintroduce the prefix rule in prose beside the check. - The revert check is story-014's clone operation with the governed paths *restored to the state the stage found them in* instead of applied, and there is deliberately no second clone builder. `run_clean_clone` stays the single build-a-clone-and-run-the-suite path and both checks go through it. The added `revert` parameter defaults to reverting nothing, so the clean-clone check's artifact, events and routing are unchanged — which is the property to re-establish after any future edit to that path, because two clone builders would drift the same way two write paths for the run's history would. diff --git a/orchestration/harness_config.py b/orchestration/harness_config.py index 9ba261f..b77813d 100644 --- a/orchestration/harness_config.py +++ b/orchestration/harness_config.py @@ -18,6 +18,31 @@ # refused. CONFIG_SCHEMA_NAME = "harness-config" +#: Keys the harness once read, mapped to the key that replaced each. A +#: retired key is refused rather than ignored: a config still carrying one +#: would fall through to the replacement's fallback and quietly change what +#: the harness does, which is the drift the declaration exists to stop. It +#: lives beside `declared_config_keys` so the config vocabulary — what is +#: read, and what used to be — has one home. +RETIRED_CONFIG_KEYS: dict[str, str] = { + "clean_clone_python": "verification_runner", +} + + +def retired_config_problems(config: dict) -> list[str]: + """One problem per retired key a loaded config still carries. + + Each names the retired key and the key that replaced it, so the refusal + is actionable without opening the schema. An empty list is the whole of + "this config carries none". + """ + return [ + f"'{key}' is no longer read by the harness; it was replaced by " + f"'{replacement}'" + for key, replacement in RETIRED_CONFIG_KEYS.items() + if key in config + ] + def find_target_root(start: Path) -> Path: for candidate in [start, *start.parents]: diff --git a/orchestration/story_coordinator.py b/orchestration/story_coordinator.py index 38b1f41..bc291e4 100644 --- a/orchestration/story_coordinator.py +++ b/orchestration/story_coordinator.py @@ -20,7 +20,6 @@ class of edit, it does not audit one. import functools import hashlib import json -import re import shlex import shutil import subprocess @@ -942,9 +941,6 @@ def append_retry_record( #: what failed; not the whole log, which the run directory is not a home for. CLEAN_CLONE_OUTPUT_TAIL = 8000 -_VERSION_PROBE = "import platform; print(platform.python_version())" -_VERSION = re.compile(r"\d+\.\d+\.\d+\S*") - @dataclass(frozen=True) class CleanCloneResult: @@ -957,17 +953,15 @@ class CleanCloneResult: ran: bool command: str - python: str - python_version: str | None = None + runner: str clone_path: str | None = None exit_code: int | None = None output_tail: str | None = None reason: str | None = None def as_record(self) -> dict: - record: dict = {"ran": self.ran, "command": self.command, "python": self.python} + record: dict = {"ran": self.ran, "command": self.command, "runner": self.runner} optional = { - "python_version": self.python_version, "clone_path": self.clone_path, "exit_code": self.exit_code, "output_tail": self.output_tail, @@ -992,27 +986,6 @@ def _resolve_interpreter(target_root: Path, interpreter: str) -> Path | None: return Path(found) if found else None -def _interpreter_version(interpreter: Path) -> str | None: - """What the interpreter reports its version to be, or None. - - None is not a failure: the configured test command need not be a Python - interpreter at all, and a record with no version is honest about that. A - *configured* clean_clone_python that does not exist is a different case, - handled by the caller, because a check quietly testing the wrong version - is worse than one that refuses. - """ - try: - result = subprocess.run( - [str(interpreter), "-c", _VERSION_PROBE], - capture_output=True, - text=True, - ) - except OSError: - return None - version = result.stdout.strip() - return version if result.returncode == 0 and _VERSION.fullmatch(version) else None - - def _build_clone( target_root: Path, clone: Path, @@ -1166,7 +1139,7 @@ def _link_interpreter_roots(target_root: Path, clone: Path, interpreters: list[s def run_clean_clone( target_root: Path, test_command: str, - clean_clone_python: str | None, + verification_runner: str | None, destination: Path, revert: list[str] | tuple[str, ...] = (), baseline: Path | None = None, @@ -1174,10 +1147,16 @@ def run_clean_clone( """Run the configured test command in a fresh clone with the story committed. The command is the target repository's own `test_command`; nothing about - it is written here. Only its interpreter is substituted, and only when the - configuration names a `clean_clone_python`, so the check can exercise the - oldest supported Python rather than whichever one the developer works in. - The caller owns `destination` and removes it whatever the result. + it is written here. Only its *first word* is substituted, and only when the + configuration names a `verification_runner`, so the check can exercise an + environment other than the one the developer works in and find an + incompatibility before CI rather than by it. The caller owns `destination` + and removes it whatever the result. + + That substitution is a first-word swap and nothing more: every remaining + argument is the configured command's own. A target whose environment + difference cannot be expressed by replacing the first word expresses it in + `test_command` instead. This is the single build-a-clone-and-run-the-suite path. `revert` and the `baseline` it is restored from are passed through to the clone builder and @@ -1186,27 +1165,27 @@ def run_clean_clone( restored to the state the stage found them in rather than applied. """ argv = shlex.split(test_command) - interpreter = clean_clone_python or argv[0] - command = shlex.join([interpreter, *argv[1:]]) + runner = verification_runner or argv[0] + command = shlex.join([runner, *argv[1:]]) - resolved = _resolve_interpreter(target_root, interpreter) - if clean_clone_python and resolved is None: + resolved = _resolve_interpreter(target_root, runner) + if verification_runner and resolved is None: return CleanCloneResult( ran=False, command=command, - python=interpreter, + runner=runner, reason=( - f"clean_clone_python names {clean_clone_python}, which is not an " - f"interpreter that exists under {target_root}" + f"verification_runner names {verification_runner}, which is not " + f"an executable that exists under {target_root}" ), ) clone = destination / "clone" _build_clone(target_root, clone, revert=revert, baseline=baseline) - _link_interpreter_roots(target_root, clone, [argv[0], interpreter]) + _link_interpreter_roots(target_root, clone, [argv[0], runner]) result = subprocess.run( - [interpreter, *argv[1:]], + [runner, *argv[1:]], cwd=clone, capture_output=True, text=True, @@ -1215,8 +1194,7 @@ def run_clean_clone( return CleanCloneResult( ran=True, command=command, - python=interpreter, - python_version=_interpreter_version(resolved) if resolved else None, + runner=runner, clone_path=str(clone), exit_code=result.returncode, output_tail=output[-CLEAN_CLONE_OUTPUT_TAIL:], @@ -1238,7 +1216,7 @@ def clean_clone_check( result = run_clean_clone( target_root, config["test_command"], - config.get("clean_clone_python"), + config.get("verification_runner"), scratch, ) finally: @@ -1556,14 +1534,14 @@ def revert_check( cannot be built at all. """ command = config["test_command"] - python = config.get("clean_clone_python") or shlex.split(command)[0] + runner = config.get("verification_runner") or shlex.split(command)[0] resolved = baseline if baseline is not None and baseline.is_dir() else None if resolved is None: result = CleanCloneResult( ran=False, command=command, - python=python, + runner=runner, reason=( "no baseline was captured for the stage, so there is no state " f"to revert the edits to: {baseline}" @@ -1575,7 +1553,7 @@ def revert_check( result = run_clean_clone( target_root, command, - config.get("clean_clone_python"), + config.get("verification_runner"), scratch, revert=list(paths), baseline=resolved, @@ -1584,7 +1562,7 @@ def revert_check( result = CleanCloneResult( ran=False, command=command, - python=python, + runner=runner, reason=f"the clone with the edits reverted could not be built: {error}", ) finally: @@ -2351,6 +2329,21 @@ def _refuse_bad_self_routes(workflow: dict, problems: list[str]) -> int: ) +def _refuse_retired_config_keys(target_root: Path, problems: list[str]) -> int: + """Refuse a run whose configuration still carries a retired key. + + Thin, like every other caller of `refuse`. The configuration is wrong, not + the story and not the tree, so the guidance names the file to edit and the + edit to make. + """ + return refuse( + f"{target_root / '.harness' / 'config.yaml'} carries configuration keys " + f"the harness no longer reads:", + problems, + "Rename each key to its replacement before running a story.", + ) + + def _refuse_dirty_tree(target_root: Path, paths: list[str]) -> int: """Refuse a run whose target tree already holds work no stage produced. @@ -2538,6 +2531,17 @@ def run_story( case and means the repository's own default branch; see `resolve_base`. """ config = harness_config.load_config(target_root) + + # Pre-flight: a retired configuration key is refused rather than ignored. + # Ignoring one lets the run fall back to the replacement's default and + # quietly exercise something other than what the config asked for. Above + # every other pre-flight, because it is decidable the moment the config + # loads: a refusal here leaves no run directory, no state.json, no log, no + # new branch, and invokes no agent. + retired = harness_config.retired_config_problems(config) + if retired: + return _refuse_retired_config_keys(target_root, retired) + workflow = harness_config.load_workflow(harness_root, config.get("workflow", "story-workflow")) rules = harness_config.load_rules(harness_root) stages = workflow["stages"] diff --git a/schemas/clean-clone-result.schema.json b/schemas/clean-clone-result.schema.json index 027be4d..fe33420 100644 --- a/schemas/clean-clone-result.schema.json +++ b/schemas/clean-clone-result.schema.json @@ -3,23 +3,19 @@ "title": "clean-clone-result", "description": "The coordinator's record of the clean-clone check: the configured test command run a second time in a fresh clone of the repository with the story committed into it, after the verifier passes and before the documenter runs. Coordinator-written rather than stage-written, so it appears in no stage's schemas map and no agent is asked to satisfy it; the artifact exists so a reader can tell the check ran rather than inferring it from a pass. Optional fields are expressed by absence rather than by null, as execution-history does: a check that refused to run has no exit code to report.", "type": "object", - "required": ["ran", "command", "python"], + "required": ["ran", "command", "runner"], "properties": { "ran": { "type": "boolean", - "description": "Whether the suite actually ran in the clone. False when the check refused to run, in which case reason says why and exit_code, output_tail and python_version are absent." + "description": "Whether the suite actually ran in the clone. False when the check refused to run, in which case reason says why and exit_code and output_tail are absent." }, "command": { "type": "string", - "description": "The command executed with the clone as its working directory, taken from the target repository's configured test_command with its interpreter replaced by the one named below." + "description": "The command executed with the clone as its working directory, taken from the target repository's configured test_command with its first word replaced by the executable named below." }, - "python": { + "runner": { "type": "string", - "description": "The interpreter the run used: .harness/config.yaml's clean_clone_python when that key is set, and test_command's own interpreter otherwise." - }, - "python_version": { - "type": "string", - "description": "The version that interpreter reported, so a reader can tell which Python the check exercised rather than assuming it matched CI. Absent when the interpreter reported no recognizable version, which is what a test command that is not a Python interpreter does." + "description": "The executable the run used: .harness/config.yaml's verification_runner when that key is set, and test_command's own first word otherwise." }, "clone_path": { "type": "string", diff --git a/schemas/harness-config.schema.json b/schemas/harness-config.schema.json index 7c65727..a1d0b85 100644 --- a/schemas/harness-config.schema.json +++ b/schemas/harness-config.schema.json @@ -23,10 +23,6 @@ "type": "string", "description": "Prepended to a story id to form the story branch name. Defaults to story/." }, - "clean_clone_python": { - "type": "string", - "description": "The interpreter the clean-clone and revert checks run the configured test command under. Unset, the check falls back to test_command's own leading word." - }, "logs_dir": { "type": "string", "description": "Directory, relative to the target root, holding raw agent output logs. Defaults to .harness/logs." @@ -55,6 +51,10 @@ "type": "string", "description": "The command the clean-clone and revert checks run inside a scratch clone. Read without a fallback: a target that omits it cannot run either check." }, + "verification_runner": { + "type": "string", + "description": "The executable the clean-clone and revert checks run the configured test command under: it replaces the first word of test_command, and every remaining argument is the configured command's own. Unset, the checks use test_command's own first word, so the command runs exactly as written. An environment difference that cannot be expressed as a first-word swap belongs in test_command instead." + }, "workflow": { "type": "string", "description": "The name of the workflow definition under workflows/ that a run executes. Defaults to story-workflow." diff --git a/schemas/revert-check-result.schema.json b/schemas/revert-check-result.schema.json index 8c335c2..9d69bbe 100644 --- a/schemas/revert-check-result.schema.json +++ b/schemas/revert-check-result.schema.json @@ -3,11 +3,11 @@ "title": "revert-check-result", "description": "The coordinator's record of the revert check: after a stage that declares both a changed-files record and a may_not_create list, its own modifications and deletions under those prefixes are permitted iff reverting them makes the suite fail. A forced repair breaks the suite when reverted; new coverage does not. Coordinator-written rather than stage-written, so it appears in no stage's schemas map and no agent is asked to satisfy it, and nothing in orchestration routes on it — it is evidence, like clean-clone-result. GRANULARITY: the check reverts every governed path at once and decides on that single run of the suite. A set containing one forced repair is therefore permitted in full, including any added coverage sitting in the other files of that set, and a single file mixing a forced repair with added coverage is not caught at all. The paths field states exactly what was reverted, so a reader can tell what the decision covered rather than assuming it discriminated per file. Optional fields are expressed by absence rather than by null, as clean-clone-result does: a check that could not run decided nothing.", "type": "object", - "required": ["ran", "paths", "command", "python"], + "required": ["ran", "paths", "command", "runner"], "properties": { "ran": { "type": "boolean", - "description": "Whether the suite actually ran in the clone with the edits reverted. False when the check could not run, in which case reason says why and permitted, exit_code, output_tail and python_version are absent." + "description": "Whether the suite actually ran in the clone with the edits reverted. False when the check could not run, in which case reason says why and permitted, exit_code and output_tail are absent." }, "paths": { "type": "array", @@ -16,20 +16,16 @@ }, "command": { "type": "string", - "description": "The command executed with the clone as its working directory, taken from the target repository's configured test_command with its interpreter replaced by the one named below." + "description": "The command executed with the clone as its working directory, taken from the target repository's configured test_command with its first word replaced by the executable named below." }, - "python": { + "runner": { "type": "string", - "description": "The interpreter the run used: .harness/config.yaml's clean_clone_python when that key is set, and test_command's own interpreter otherwise." + "description": "The executable the run used: .harness/config.yaml's verification_runner when that key is set, and test_command's own first word otherwise." }, "permitted": { "type": "boolean", "description": "Whether the edits are permitted. True when the suite failed with every path above reverted, which is what makes the set maintenance the change forced rather than validation the stage authored. False escalates the run immediately, without incrementing retry_count. Absent when ran is false, because a check that could not run permitted nothing and refused nothing." }, - "python_version": { - "type": "string", - "description": "The version that interpreter reported, so a reader can tell which Python the check exercised. Absent when the interpreter reported no recognizable version, which is what a test command that is not a Python interpreter does." - }, "clone_path": { "type": "string", "description": "Where the clone was built, under a temporary directory outside the target repository. The directory is removed once the run completes, so this identifies the run rather than naming a path to visit. Absent when no clone was built." diff --git a/tests/test_clean_clone_check.py b/tests/test_clean_clone_check.py index 2e89e5e..8a92b69 100644 --- a/tests/test_clean_clone_check.py +++ b/tests/test_clean_clone_check.py @@ -100,13 +100,11 @@ #: right in both environments. CORRECT_TEST_COMMAND = f"sh -c 'grep -q {MARKER} src/app.py'" -#: A stand-in interpreter. Answers the coordinator's version probe with a -#: version no real interpreter in this environment reports, and exits zero -#: for anything else, so "which Python did the check use" has one answer. -FAKE_VERSION = "3.0.1" -FAKE_INTERPRETER = f"""\ +#: A stand-in executable the configured command can be pointed at. It accepts +#: whatever arguments it is handed and exits zero, so a test that only needs +#: "the configured runner is what ran" has one answer and no second variable. +FAKE_INTERPRETER = """\ #!/bin/sh -if [ "$1" = "-c" ]; then echo {FAKE_VERSION}; exit 0; fi exit 0 """ @@ -161,6 +159,27 @@ def install_interpreter(target_root: Path, rel: str) -> str: return rel +def install_runner(target_root: Path, rel: str, *, status: int, + argv_log: Path) -> str: + """Drop a verification runner that is an interpreter of nothing at `rel`. + + story-041 removed the assumption that the executable running the suite is + a Python interpreter, and this is the executable that holds it removed: it + evaluates no source, it records the arguments it was handed into + `argv_log`, and it exits with the status baked into it. The status is a + parameter rather than a constant so "the record carries the suite's own + exit code" is a fact a test chose, not the zero every command returns when + nothing went wrong. + """ + path = target_root / rel + path.parent.mkdir(parents=True, exist_ok=True) + path.write_text( + f"#!/bin/sh\nprintf '%s\\n' \"$@\" > {argv_log}\nexit {status}\n", + encoding="utf-8") + path.chmod(0o755) + return rel + + def run_dir_of(target_root: Path, story_id: str = "story-001") -> Path: return target_root / ".harness" / "runs" / story_id @@ -266,9 +285,9 @@ def dirty_target(target_root: Path) -> Path: def clean_clone(target_root: Path, destination: Path, command: str, - python: str | None = None): + runner: str | None = None): return story_coordinator.run_clean_clone( - target_root, command, python, destination) + target_root, command, runner, destination) # -------------------------------------------------------------------------- @@ -430,11 +449,11 @@ def test_the_configured_command_is_what_runs(story_target, tmp_path): assert result.command == f"sh -c 'echo ran > {marker}'" -def test_the_configured_clean_clone_python_replaces_the_commands_interpreter( +def test_the_configured_verification_runner_replaces_the_commands_first_word( story_target, tmp_path, ): - """Pointed at an interpreter that is not the one the harness runs under, - the record names that one and reports its version, not the harness's.""" + """Pointed at an executable that is not the one the harness runs under, + the record names that one rather than the command's own first word.""" install_interpreter(story_target, "fakepy") dirty_target(story_target) @@ -444,15 +463,8 @@ def test_the_configured_clean_clone_python_replaces_the_commands_interpreter( assert result.ran is True assert result.exit_code == 0 - assert result.python == "./fakepy" + assert result.runner == "./fakepy" assert result.command == "./fakepy -m pytest -q" - assert result.python_version == FAKE_VERSION - assert result.python_version != platform_version() - - -def platform_version() -> str: - import platform - return platform.python_version() def test_the_command_keeps_its_arguments_when_the_interpreter_is_replaced( @@ -465,21 +477,21 @@ def test_the_command_keeps_its_arguments_when_the_interpreter_is_replaced( assert result.command == "./fakepy -m pytest tests/ -q" -def test_an_absent_clean_clone_python_falls_back_to_the_commands_own( +def test_an_absent_verification_runner_falls_back_to_the_commands_own( story_target, tmp_path, ): install_interpreter(story_target, "fakepy") dirty_target(story_target) result = clean_clone(story_target, tmp_path / "scratch", "./fakepy -m pytest", None) assert result.ran is True - assert result.python == "./fakepy" - assert result.python_version == FAKE_VERSION + assert result.runner == "./fakepy" + assert result.command == "./fakepy -m pytest" -def test_a_clean_clone_python_that_does_not_exist_refuses_rather_than_falls_back( +def test_a_verification_runner_that_does_not_exist_refuses_rather_than_falls_back( story_target, tmp_path, ): - """A check that quietly tests the wrong version is worse than one that + """A check that quietly runs the wrong executable is worse than one that refuses.""" dirty_target(story_target) result = clean_clone( @@ -488,21 +500,149 @@ def test_a_clean_clone_python_that_does_not_exist_refuses_rather_than_falls_back assert result.ran is False assert ".venv999/bin/python" in result.reason assert result.exit_code is None - assert result.python == ".venv999/bin/python" + assert result.runner == ".venv999/bin/python" assert not (tmp_path / "scratch" / "clone").exists() -def test_a_non_python_command_records_no_version_rather_than_refusing( +def test_a_command_whose_first_word_is_not_an_interpreter_runs_as_written( story_target, tmp_path, ): - """The configured test command need not be a Python interpreter, so a - fallback interpreter reporting no version is honest rather than an - error.""" + """The configured test command is the target's own, whatever it is, and + an unset runner leaves it running exactly as written.""" dirty_target(story_target) result = clean_clone(story_target, tmp_path / "scratch", "sh -c 'true'", None) assert result.ran is True - assert result.python_version is None - assert "python_version" not in result.as_record() + assert result.runner == "sh" + assert result.command == "sh -c true" + + +# -------------------------------------------------------------------------- +# story-041: the runner is not assumed to be an interpreter of any language +# +# The three tests above point the check at a stand-in that happens to be a +# shell script, but they assert about the *command* the check builds. What +# story-041 claims is stronger and about the *record*: a target whose +# configured runner is not a Python interpreter gets a correct one — the +# executable the configuration named recorded as the runner, the check +# recorded as having run, and the exit code tracking the suite's own. +# +# "The run did not raise" is not that claim, and neither is "the record is +# non-empty": both pass today against a check that recorded whatever it liked. +# So the runner below reports which arguments reached it, its exit status is a +# parameter rather than a constant, and the record's key set is asserted whole +# — a record that carried a version field, or that spelled the executable +# under any other key, fails here rather than passing unnoticed. +# -------------------------------------------------------------------------- + +#: The configured command in these tests names a first word that exists +#: nowhere, so nothing can run except by the substitution putting the +#: configured runner in its place. +FOREIGN_TEST_COMMAND = "xyzzy-suite-driver --profile ci tests/" +FOREIGN_ARGUMENTS = ["--profile", "ci", "tests/"] +RUNNER_PATH = "toolchain/run-suite" + + +@pytest.mark.parametrize("status", [0, 7]) +def test_a_runner_that_is_no_interpreter_is_what_runs_and_what_is_recorded( + story_target, tmp_path, status, +): + argv_log = tmp_path / "argv.txt" + install_runner(story_target, RUNNER_PATH, status=status, argv_log=argv_log) + dirty_target(story_target) + + result = clean_clone(story_target, tmp_path / "scratch", + FOREIGN_TEST_COMMAND, f"./{RUNNER_PATH}") + + assert result.ran is True + assert result.runner == f"./{RUNNER_PATH}" + assert result.command == f"./{RUNNER_PATH} --profile ci tests/" + # The suite's own status, not a status the check decided for it. + assert result.exit_code == status + # And it really was that executable, with the configured command's own + # remaining words: the record describes a run that happened rather than + # one the check narrated. + assert argv_log.read_text(encoding="utf-8").split() == FOREIGN_ARGUMENTS + + +def test_the_record_such_a_run_writes_is_exactly_the_fields_the_schema_declares( + story_target, tmp_path, +): + """The key set whole rather than one absence at a time. + + An assertion that the record carries no version field would pass against a + record read from the wrong place, or against a field renamed rather than + removed. Equality against the declared set cannot: it fails on a field + that survived, on one that was added, and on the executable spelled under + any key but `runner`. + """ + argv_log = tmp_path / "argv.txt" + install_runner(story_target, RUNNER_PATH, status=4, argv_log=argv_log) + dirty_target(story_target) + + record = clean_clone(story_target, tmp_path / "scratch", + FOREIGN_TEST_COMMAND, f"./{RUNNER_PATH}").as_record() + + assert set(record) == {"ran", "command", "runner", "clone_path", + "exit_code", "output_tail"} + assert set(record) <= set(SCHEMA["properties"]) + assert schema_validator.validate(record, SCHEMA) == [] + + +def test_the_check_writes_that_record_for_a_configuration_that_names_the_runner( + story_target, tmp_path, +): + """The same claim through the key the configuration carries. + + The test above hands `run_clean_clone` its runner directly. This one hands + the coordinator a configuration and reads the artifact off disk, which is + where a reader of a finished run meets it. + """ + argv_log = tmp_path / "argv.txt" + install_runner(story_target, RUNNER_PATH, status=3, argv_log=argv_log) + dirty_target(story_target) + run_dir = run_dir_of(story_target) + run_dir.mkdir(parents=True, exist_ok=True) + + story_coordinator.clean_clone_check( + run_dir, story_target, + {"test_command": FOREIGN_TEST_COMMAND, + "verification_runner": f"./{RUNNER_PATH}"}, + ARTIFACT) + + record = record_of(run_dir) + assert record["runner"] == f"./{RUNNER_PATH}" + assert record["ran"] is True + assert record["exit_code"] == 3 + assert record["command"] == f"./{RUNNER_PATH} --profile ci tests/" + assert argv_log.read_text(encoding="utf-8").split() == FOREIGN_ARGUMENTS + assert schema_validator.validate(record, SCHEMA) == [] + + +def test_a_run_recording_a_runner_the_configuration_did_not_name_fails_these( + story_target, tmp_path, +): + """The control the three assertions above need, which the story asks for + by name: a run that succeeds while recording a runner the configuration + did not name must fail them. + + The mutant is the one the module already carries — the configured runner + ignored in favour of the command's own first word — and the constructed + violation is run through the same helper the proofs use. It completes, it + raises nothing, and every assertion above about *which* executable ran is + false of it. + """ + module = variant("an interpreter the configuration did not name", tmp_path) + argv_log = tmp_path / "argv.txt" + install_runner(story_target, RUNNER_PATH, status=7, argv_log=argv_log) + dirty_target(story_target) + + result = module.run_clean_clone( + story_target, f"./{RUNNER_PATH} --profile ci tests/", "./fakepy", + tmp_path / "scratch") + + assert result.ran is True + assert result.runner != "./fakepy" + assert result.exit_code == 7 # -------------------------------------------------------------------------- @@ -559,7 +699,7 @@ def test_the_check_leaves_no_scratch_directory_behind_in_the_temp_root( def test_the_schema_uses_only_the_keywords_the_validator_supports(): assert schema_validator.unsupported_keywords(SCHEMA) == [] assert schema_validator.validate( - {"ran": True, "command": "x", "python": "y"}, SCHEMA) == [] + {"ran": True, "command": "x", "runner": "y"}, SCHEMA) == [] def test_no_union_keyword_appears_anywhere_in_the_schema(): @@ -580,21 +720,20 @@ def walk(node) -> None: def test_optional_fields_are_expressed_by_absence_from_required(): - assert set(SCHEMA["required"]) == {"ran", "command", "python"} + assert set(SCHEMA["required"]) == {"ran", "command", "runner"} optional = set(SCHEMA["properties"]) - set(SCHEMA["required"]) - assert optional == {"python_version", "clone_path", "exit_code", - "output_tail", "reason"} + assert optional == {"clone_path", "exit_code", "output_tail", "reason"} def test_the_schema_catches_a_record_missing_a_required_field(): """The schema constrains something: it is not vacuously satisfied.""" errors = schema_validator.validate({"ran": True, "command": "x"}, SCHEMA) - assert errors == ["$.python: expected a required property, found it missing"] + assert errors == ["$.runner: expected a required property, found it missing"] def test_the_schema_catches_a_wrongly_typed_exit_code(): errors = schema_validator.validate( - {"ran": True, "command": "x", "python": "y", "exit_code": "1"}, SCHEMA) + {"ran": True, "command": "x", "runner": "y", "exit_code": "1"}, SCHEMA) assert len(errors) == 1 assert "$.exit_code" in errors[0] @@ -785,7 +924,7 @@ def test_a_refused_check_escalates_naming_the_missing_interpreter( story_target, harness_root, ): configure(story_target, test_command=CORRECT_TEST_COMMAND, - clean_clone_python=".venv999/bin/python") + verification_runner=".venv999/bin/python") runner = Runner(story_target, [PASS]) assert story_coordinator.run_story( "story-001", harness_root, story_target, runner) == 2 @@ -1060,7 +1199,7 @@ def test_the_placeholder_exists_in_the_prompt_and_in_the_context( run_dir = run_dir_of(story_target) run_dir.mkdir(parents=True, exist_ok=True) - write_json(run_dir / ARTIFACT, {"ran": True, "command": "x", "python": "y"}) + write_json(run_dir / ARTIFACT, {"ran": True, "command": "x", "runner": "y"}) context = build_context_for(story_target, harness_root, run_dir) assert "clean_clone_result" in context assert "x" in context["clean_clone_result"] @@ -1123,10 +1262,10 @@ def build_context_for(target_root: Path, harness_root: Path, run_dir: Path) -> d ' _git(clone, "add", "-A")\n', "", ), - # The configured interpreter ignored in favor of the command's own. + # The configured runner ignored in favor of the command's own first word. "an interpreter the configuration did not name": ( - " interpreter = clean_clone_python or argv[0]", - " interpreter = argv[0]", + " runner = verification_runner or argv[0]", + " runner = argv[0]", ), # The scratch directory left behind. "a scratch directory that is never removed": ( @@ -1172,14 +1311,14 @@ def test_a_clone_without_the_story_committed_is_caught( assert "documenter" in runner.calls -def test_an_ignored_clean_clone_python_is_caught(story_target, tmp_path): +def test_an_ignored_verification_runner_is_caught(story_target, tmp_path): module = variant("an interpreter the configuration did not name", tmp_path) install_interpreter(story_target, "fakepy") dirty_target(story_target) result = module.run_clean_clone( story_target, "sh -c 'true'", "./fakepy", tmp_path / "scratch") - assert result.python != "./fakepy" - assert result.python_version != FAKE_VERSION + assert result.runner != "./fakepy" + assert result.command != "./fakepy -c true" def test_a_scratch_directory_left_behind_is_caught(story_target, tmp_path): diff --git a/tests/test_config_keys_are_obeyed.py b/tests/test_config_keys_are_obeyed.py index 374f3a2..7cd2517 100644 --- a/tests/test_config_keys_are_obeyed.py +++ b/tests/test_config_keys_are_obeyed.py @@ -91,7 +91,6 @@ "architecture_docs": ["docs/xyzzy-architecture.md"], "base_branch": "xyzzy-base", "branch_prefix": "xyzzy-branch/", - "clean_clone_python": "/xyzzy/bin/interpreter", "logs_dir": ".harness/xyzzy-logs", "model": "xyzzy-model", "permission_mode": "xyzzyPrompt", @@ -99,12 +98,13 @@ "standards_dir": ".harness/xyzzy-standards", "stories_dir": ".harness/xyzzy-stories", "test_command": "xyzzy-runner --all", + "verification_runner": "/xyzzy/bin/interpreter", "workflow": "xyzzy-workflow", } #: What the harness uses when the key is absent, as written in the code that #: reads it. `None` is the answer for the four keys with no fallback at all: -#: `allowed_tools`, `base_branch`, `clean_clone_python` and `model` are read +#: `allowed_tools`, `base_branch`, `verification_runner` and `model` are read #: with a bare `config.get`, and `test_command` is read with no default and #: no fallback, so a target that omits it cannot run the clean-clone check. FALLBACKS: dict[str, object] = { @@ -112,7 +112,6 @@ "architecture_docs": [], "base_branch": None, "branch_prefix": "story/", - "clean_clone_python": None, "logs_dir": ".harness/logs", "model": None, "permission_mode": "acceptEdits", @@ -120,6 +119,7 @@ "standards_dir": ".harness/standards", "stories_dir": ".harness/stories", "test_command": None, + "verification_runner": None, "workflow": "story-workflow", } @@ -161,9 +161,6 @@ def node_id(self) -> str: "branch_prefix": Proof( "test_branch_prefix_names_the_branch_the_run_creates_and_works_on", BEHAVIOURAL), - "clean_clone_python": Proof( - "test_clean_clone_python_is_the_interpreter_the_check_resolves", - BEHAVIOURAL), "logs_dir": Proof( "test_logs_dir_is_where_the_stage_log_is_written", BEHAVIOURAL), @@ -185,6 +182,9 @@ def node_id(self) -> str: "test_command": Proof( "test_test_command_is_the_command_the_clean_clone_path_builds", BEHAVIOURAL), + "verification_runner": Proof( + "test_verification_runner_is_the_executable_the_check_resolves", + BEHAVIOURAL), "workflow": Proof( "test_workflow_names_the_definition_the_run_actually_executes", BEHAVIOURAL), @@ -228,11 +228,6 @@ def node_id(self) -> str: 'config.get("branch_prefix", "story/")', '"story/"'), ), - "clean_clone_python": ( - ("orchestration/story_coordinator.py", - 'config.get("clean_clone_python")', - "None"), - ), "logs_dir": ( ("orchestration/story_coordinator.py", 'config.get("logs_dir", ".harness/logs")', @@ -274,6 +269,11 @@ def node_id(self) -> str: 'config.get("test_command")', HARDCODED_TEST_COMMAND), ), + "verification_runner": ( + ("orchestration/story_coordinator.py", + 'config.get("verification_runner")', + "None"), + ), "workflow": ( ("orchestration/story_coordinator.py", 'config.get("workflow", "story-workflow")', @@ -528,11 +528,11 @@ def complete_run(tmp_path: Path, **overrides: object) -> Run: def clean_clone_record(run: Run) -> dict: """What the clean-clone path builds for the fixture's configuration. - The command is *observed*, not executed: `clean_clone_python` names an - interpreter that does not exist, so `run_clean_clone` reports a check that - did not run, with the command it would have run and the interpreter it + The command is *observed*, not executed: `verification_runner` names an + executable that does not exist, so `run_clean_clone` reports a check that + did not run, with the command it would have run and the runner it resolved, before any clone is built. That keeps the proof deterministic - and free of any dependency on a second interpreter being installed. + and free of any dependency on a second toolchain being installed. """ artifact = "xyzzy-clean-clone-result.json" story_coordinator.clean_clone_check(run.run_dir, run.target, run.config, @@ -546,8 +546,8 @@ def clean_clone_record(run: Run) -> dict: EXPECTED_KEYS = ( "allowed_tools", "architecture_docs", "base_branch", "branch_prefix", - "clean_clone_python", "logs_dir", "model", "permission_mode", "runs_dir", - "standards_dir", "stories_dir", "test_command", "workflow", + "logs_dir", "model", "permission_mode", "runs_dir", "standards_dir", + "stories_dir", "test_command", "verification_runner", "workflow", ) @@ -945,14 +945,14 @@ def test_test_command_is_the_command_the_clean_clone_path_builds(tmp_path): assert "xyzzy-runner --all" in run.prompt_for("implementer") record = clean_clone_record(run) # The configured command's own arguments, under the configured - # interpreter: `--all` is the half that comes from `test_command`. + # runner: `--all` is the half that comes from `test_command`. assert record["command"] == "/xyzzy/bin/interpreter --all" -def test_clean_clone_python_is_the_interpreter_the_check_resolves(tmp_path): +def test_verification_runner_is_the_executable_the_check_resolves(tmp_path): run = complete_run(tmp_path) record = clean_clone_record(run) - assert record["python"] == "/xyzzy/bin/interpreter" + assert record["runner"] == "/xyzzy/bin/interpreter" assert record["ran"] is False assert "/xyzzy/bin/interpreter" in record["reason"] diff --git a/tests/test_no_target_stack_in_harness_source.py b/tests/test_no_target_stack_in_harness_source.py index b5f4e4d..2e1ceec 100644 --- a/tests/test_no_target_stack_in_harness_source.py +++ b/tests/test_no_target_stack_in_harness_source.py @@ -12,9 +12,12 @@ is keyed by repository-relative path and the exact text of the matched line rather than by a line number, so an unrelated edit above a tie does not churn the list and look like the burn-down. - * **the five audited ties.** Asserted present by name, by running the - scan rather than by reading the list — a scan that cannot see the ties - that motivated it has not been shown to work. + * **the audited ties that are still grandfathered.** Asserted present by + name, by running the scan rather than by reading the list — a scan that + cannot see the ties that motivated it has not been shown to work. The + audit's other two both sat in `orchestration/story_coordinator.py` and + were repaired by the-interpreter-is-not-assumed-to-be-python, which is + also why "no tie was fixed" now guards two files rather than three. * **the matcher.** The four boundary cases the story names are constructed and run through the real `scan`, not reasoned about. * **the stated limits.** Read out of `orchestration/harness_source.py`'s @@ -62,10 +65,11 @@ DECLARING_MODULE = "orchestration/harness_source.py" VALIDATION_REL = "tests/test_no_target_stack_in_harness_source.py" -#: The three files the story forbids itself to edit. Every tie in them is -#: grandfathered below and repaired by a later story, not by this one. +#: The files whose ties are still grandfathered below, and which no story +#: since has edited. `orchestration/story_coordinator.py` was among them +#: until the-interpreter-is-not-assumed-to-be-python repaired its ties, which +#: is what taking it off this tuple records. UNTOUCHED = ( - "orchestration/story_coordinator.py", "workflows/story-workflow.json", "prompts/tester.md", ) @@ -91,95 +95,24 @@ TEMPORARY_TIES: frozenset[tuple[str, str]] = frozenset({ - # --- The version probe: a Python snippet the harness executes. ------ - ('orchestration/story_coordinator.py', - '_VERSION_PROBE = "import platform; print(platform.python_version())"'), - - # --- The clean-clone record's own Python-shaped fields. ------------- - ('orchestration/story_coordinator.py', - ' python: str'), - ('orchestration/story_coordinator.py', - ' python_version: str | None = None'), - ('orchestration/story_coordinator.py', - ' record: dict = {"ran": self.ran, "command": self.command, "python": self.python}'), - ('orchestration/story_coordinator.py', - ' "python_version": self.python_version,'), - - # --- The clean_clone_python configuration key, and the prose that - # explains it. Line 1179 is here rather than in the permanent list - # because "the oldest supported Python" is an assumption about the - # target's stack, not a statement about the harness's own. - ('orchestration/story_coordinator.py', - ' *configured* clean_clone_python that does not exist is a different case,'), - ('orchestration/story_coordinator.py', - ' clean_clone_python: str | None,'), - ('orchestration/story_coordinator.py', - ' configuration names a `clean_clone_python`, so the check can exercise the'), - ('orchestration/story_coordinator.py', - ' oldest supported Python rather than whichever one the developer works in.'), - ('orchestration/story_coordinator.py', - ' interpreter = clean_clone_python or argv[0]'), - ('orchestration/story_coordinator.py', - ' if clean_clone_python and resolved is None:'), - ('orchestration/story_coordinator.py', - ' python=interpreter,'), - ('orchestration/story_coordinator.py', - ' f"clean_clone_python names {clean_clone_python}, which is not an "'), - ('orchestration/story_coordinator.py', - ' python=interpreter,'), - ('orchestration/story_coordinator.py', - ' python_version=_interpreter_version(resolved) if resolved else None,'), - ('orchestration/story_coordinator.py', - ' config.get("clean_clone_python"),'), - ('orchestration/story_coordinator.py', - ' python = config.get("clean_clone_python") or shlex.split(command)[0]'), - ('orchestration/story_coordinator.py', - ' python=python,'), - ('orchestration/story_coordinator.py', - ' config.get("clean_clone_python"),'), - ('orchestration/story_coordinator.py', - ' python=python,'), - # --- Two lines of prose in a prompt naming a pytest layout. --------- ('prompts/tester.md', 'New tests belong in tests/ and become permanent repository assets.'), ('prompts/tester.md', 'shared resolution in `tests/conftest.py`.'), - # --- The artifact schemas that burn down with the interpreter story, - # which names these three files by name. - ('schemas/clean-clone-result.schema.json', - ' "required": ["ran", "command", "python"],'), - ('schemas/clean-clone-result.schema.json', - ' "description": "Whether the suite actually ran in the clone. False when the check refused to run, in which case reason says why and exit_code, output_tail and python_version are absent."'), - ('schemas/clean-clone-result.schema.json', - ' "python": {'), - ('schemas/clean-clone-result.schema.json', - ' "description": "The interpreter the run used: .harness/config.yaml\'s clean_clone_python when that key is set, and test_command\'s own interpreter otherwise."'), - ('schemas/clean-clone-result.schema.json', - ' "python_version": {'), - ('schemas/clean-clone-result.schema.json', - ' "description": "The version that interpreter reported, so a reader can tell which Python the check exercised rather than assuming it matched CI. Absent when the interpreter reported no recognizable version, which is what a test command that is not a Python interpreter does."'), - ('schemas/harness-config.schema.json', - ' "clean_clone_python": {'), - ('schemas/revert-check-result.schema.json', - ' "required": ["ran", "paths", "command", "python"],'), - ('schemas/revert-check-result.schema.json', - ' "description": "Whether the suite actually ran in the clone with the edits reverted. False when the check could not run, in which case reason says why and permitted, exit_code, output_tail and python_version are absent."'), - ('schemas/revert-check-result.schema.json', - ' "python": {'), - ('schemas/revert-check-result.schema.json', - ' "description": "The interpreter the run used: .harness/config.yaml\'s clean_clone_python when that key is set, and test_command\'s own interpreter otherwise."'), - ('schemas/revert-check-result.schema.json', - ' "python_version": {'), - ('schemas/revert-check-result.schema.json', - ' "description": "The version that interpreter reported, so a reader can tell which Python the check exercised. Absent when the interpreter reported no recognizable version, which is what a test command that is not a Python interpreter does."'), - # --- The workflow restriction naming a directory in the target. ----- ('workflows/story-workflow.json', ' "may_not_create": ["tests/"],'), }) +#: What the-interpreter-is-not-assumed-to-be-python removed from the list +#: above: every `orchestration/story_coordinator.py` entry — the version +#: probe, the record's interpreter-shaped fields, and the retired +#: configuration key with the prose explaining it — together with every +#: entry in the three schemas that story names. What is left is the +#: completion signal for the-test-location-comes-from-configuration alone. + #: Each entry carries a one-line reason saying why that mention is not a #: tie, so the permanent half is a judgement on the record rather than a @@ -206,23 +139,23 @@ ('orchestration/story_coordinator.py', ' count that cannot be spent, and `True` is not a budget however much Python'): "names the language this validation itself is written in, explaining why a bool is refused", - ('orchestration/story_coordinator.py', - ' None is not a failure: the configured test command need not be a Python'): - "says outright that a target's test command need not be Python, which is the opposite of a tie", ('orchestration/story_parser.py', '- No type coercion. Every scalar parses to a Python ``str``; ``42`` and'): "a fact about what this parser returns to its own callers, not about any target", + ('orchestration/harness_config.py', + ' "clean_clone_python": "verification_runner",'): + "names a retired key in order to refuse it, which is the harness rejecting the tie rather than carrying one", } -#: The five ties the 2026-08-15 audit found, each identified by the file it -#: sits in and a fragment of the line, so the scan is asked for them by name -#: rather than being read off the list above. +#: The ties the 2026-08-15 audit found that are still grandfathered, each +#: identified by the file it sits in and a fragment of the line, so the scan +#: is asked for them by name rather than being read off the list above. The +#: audit's other two — the version probe and the retired configuration key, +#: both in `orchestration/story_coordinator.py` — are gone from the source, +#: so asking the scan for them by name would now be asking it for something +#: that is not there. AUDITED_TIES = ( - ("the version probe", - "orchestration/story_coordinator.py", "_VERSION_PROBE", 945), - ("the clean_clone_python configuration key", - "orchestration/story_coordinator.py", "clean_clone_python", None), ("the may_not_create restriction naming a directory", "workflows/story-workflow.json", '"may_not_create": ["tests/"]', 9), ("prompts/tester.md line 19", @@ -231,20 +164,12 @@ "prompts/tester.md", "tests/conftest.py", 47), ) -#: The artifact schemas the-interpreter-is-not-assumed-to-be-python names, -#: whose mentions burn down with it and are therefore temporary. -INTERPRETER_STORY_SCHEMAS = ( - "schemas/clean-clone-result.schema.json", - "schemas/revert-check-result.schema.json", - "schemas/harness-config.schema.json", -) - #: The mentions that are honest sentences rather than ties. A rule that #: cannot tell these from a tie gets turned off within two stories. LEGITIMATE_MENTIONS = ( ("orchestration/story_parser.py", "Every scalar parses to a Python"), ("orchestration/story_coordinator.py", - "the configured test command need not be a Python"), + "is not a budget however much Python"), ) @@ -389,24 +314,39 @@ def test_each_audited_tie_is_a_temporary_tie(here, label, path, fragment, assert entry not in PERMANENT_MENTIONS, (label, entry) -@pytest.mark.parametrize("schema", INTERPRETER_STORY_SCHEMAS) -def test_the_artifact_schema_mentions_are_temporary_ties(here, schema): - """the-interpreter-is-not-assumed-to-be-python names these three files, - so their mentions burn down with it and belong on the temporary half.""" - matches = [f for f in here if f.path == schema] - assert matches, schema - for finding in matches: - assert any(token in finding.line.lower() - for token in ("python", "python_version", - "clean_clone_python")), finding - assert (finding.path, finding.line) in TEMPORARY_TIES, finding +#: The three schemas the-interpreter-is-not-assumed-to-be-python names. +#: Every mention in them burned down with it, so the scan now reports +#: nothing at all in any of the three. +REPAIRED_SCHEMAS = ( + "schemas/clean-clone-result.schema.json", + "schemas/revert-check-result.schema.json", + "schemas/harness-config.schema.json", +) + + +@pytest.mark.parametrize("schema", REPAIRED_SCHEMAS) +def test_the_repaired_schemas_carry_no_mention_at_all(here, schema): + """Read off the scan rather than off the diff: an entry taken off the + temporary list for a mention still in source would be reported here.""" + assert [f for f in here if f.path == schema] == [] + + +def test_the_coordinator_carries_no_temporary_tie(here): + """Every grandfathered tie in the coordinator is repaired. What the scan + still reports there is the one permanent mention describing this + harness's own implementation language, which is the opposite of a tie.""" + reported = {(f.path, f.line) for f in here + if f.path == "orchestration/story_coordinator.py"} + assert reported & set(TEMPORARY_TIES) == set() + assert reported <= set(PERMANENT_MENTIONS) @pytest.mark.parametrize("path,fragment", LEGITIMATE_MENTIONS) def test_a_legitimate_mention_is_permanent_and_never_a_tie(here, path, fragment): """The docstring saying this parser's scalars are Python strings, and the - one saying a target's test command need not be a Python interpreter.""" + one explaining why a bool is refused as a budget: both describe the + language this harness is written in, not any target's.""" matches = findings_for(here, path, fragment) assert matches, (path, fragment) for finding in matches: diff --git a/tests/test_retired_config_keys.py b/tests/test_retired_config_keys.py new file mode 100644 index 0000000..711e78d --- /dev/null +++ b/tests/test_retired_config_keys.py @@ -0,0 +1,513 @@ +"""Independent validation for story-041's retired-key refusal. + +A configuration key the harness no longer reads is refused, not ignored. The +distinction is the whole point: `verification_runner` falls back to the +configured test command's own first word when it is unset, so a config still +carrying `clean_clone_python` after the rename would load cleanly, resolve a +runner nobody asked for, and quietly change what the clean-clone check +exercises. A silent fallback is exactly the drift a rename is supposed to +surface. + +Written from the story's acceptance criteria rather than from the +implementation, at three altitudes: + + * **the function.** `harness_config.retired_config_problems` is a pure + function over a loaded config, so it is driven directly, and the mapping + it reads is held to naming a replacement the harness actually declares. + * **the refusal.** A throwaway target carrying the retired key is run + through the real `story_coordinator.run_story` with a fake agent runner, + and what the refusal *left behind* is read off the tree rather than + inferred from the exit status. + * **the ordering.** The refusal is claimed to sit above every other + pre-flight. That is shown by breaking a later one and observing the + retired key win — including a workflow name that cannot be loaded at all, + which raises without the retired key and refuses cleanly with it. + +Every absence asserted here carries a demonstration that it can fail: + + * "the refused run created no run directory, no state file, no branch and + invoked no agent" sits beside the same fixture with the replacement key, + where the same four observations report all four created; + * "this configuration carries no retired key" sits beside the same check + over the same configuration with the retired key put back; + * each ordering assertion sits beside the same broken fixture without the + retired key, where the later pre-flight is the one that speaks. + +Nothing here invokes a model: every run goes through the fake runner below. +""" +import json +import subprocess +from pathlib import Path + +import pytest + +import conftest +import harness_config +import schema_validator +import story_coordinator +from agent_runner import AgentResult +from conftest import commit_setup + +REPO_ROOT = Path(harness_config.__file__).resolve().parents[1] + +#: This module declares no origin in `conftest.STORY_ORIGINS`, so the shared +#: resolution bounds every comparison below at this story's own run commit and +#: its parent — and, while the story is still in flight, at the working tree +#: against HEAD, which is the one moment that pair is the correct baseline. +THIS_FILE = Path(__file__).resolve() + +#: The retired key and its replacement, written from the story's words rather +#: than imported from the mapping under test. A module that read both names +#: out of `RETIRED_CONFIG_KEYS` would agree with whatever that mapping happens +#: to say; these are what the story asked for, and the mapping is compared +#: against them below. +RETIRED = "clean_clone_python" +REPLACEMENT = "verification_runner" + +STORY_ID = "story-001" + +#: A runner that exists on every platform this suite runs on, so the control +#: run's clean-clone check resolves it and the suite it runs exits zero. The +#: substitution puts it in place of `echo`, the fixture command's first word. +WORKING_RUNNER = "/bin/echo" + +PASS_VERDICT = {"status": "passed", "blocking_issues": [], "unverified": [], + "retry_recommended": False} + + +# -------------------------------------------------------------------------- +# Fixture plumbing +# -------------------------------------------------------------------------- + + +class Runner: + """A fake agent runner that writes each stage's declared artifacts. + + It records every stage it was asked to run, which is how "no agent was + invoked" is observed as a fact about the coordinator rather than as the + absence of a log file nobody wrote. + """ + + def __init__(self, target_root: Path, run_dir: Path): + self.target_root = target_root + self.run_dir = run_dir + self.calls: list[str] = [] + + def __call__(self, prompt, *, stage, cwd, log_path, permission_mode, model, + allowed_tools=None): + self.calls.append(stage) + # Written exactly as the real runner writes it, so the stage log is + # observable as a file rather than only as an argument nobody used. + Path(log_path).parent.mkdir(parents=True, exist_ok=True) + with open(log_path, "a", encoding="utf-8") as handle: + handle.write(f"===== stage: {stage} =====\n") + if stage == "implementer": + (self.target_root / "src" / "app.py").write_text( + "print('hello')\n# the story's change\n", encoding="utf-8") + _write_json(self.run_dir / "changed-files.json", + {"modified": ["src/app.py"], "created": [], + "deleted": []}) + (self.run_dir / "implementation-summary.md").write_text( + "Did the work.\n", encoding="utf-8") + elif stage == "tester": + _write_json(self.run_dir / "test-results.json", { + "status": "passed", "tests_written": 1, "tests_run": 1, + "tests_passed": 1, "tests_failed": 0, "failures": [], + }) + _write_json(self.run_dir / "tester-changed-files.json", + {"modified": [], "created": ["tests/test_app.py"], + "deleted": []}) + elif stage == "verifier": + _write_json(self.run_dir / "verification-result.json", PASS_VERDICT) + elif stage == "documenter": + (self.run_dir / "documentation-report.md").write_text( + "No changes needed.\n", encoding="utf-8") + return AgentResult(ok=True, result_text=f"{stage} done") + + +def _write_json(path: Path, payload: dict) -> None: + path.parent.mkdir(parents=True, exist_ok=True) + path.write_text(json.dumps(payload, indent=2) + "\n", encoding="utf-8") + + +def configure(target_root: Path, **overrides: str) -> None: + """Rewrite the target's config keys, adding those it does not carry. + + The result is committed, because story-021's clean-tree pre-flight refuses + a run whose target tree already holds work no stage produced, and a test's + configuration is part of the repository the run starts *from*. + """ + path = target_root / ".harness" / "config.yaml" + lines = path.read_text(encoding="utf-8").splitlines() + for key, value in overrides.items(): + rendered = f"{key}: {value}" + for index, line in enumerate(lines): + if line.startswith(f"{key}:"): + lines[index] = rendered + break + else: + lines.append(rendered) + path.write_text("\n".join(lines) + "\n", encoding="utf-8") + commit_setup(target_root, "configure the target for this test") + + +def git(root: Path, *args: str) -> str: + """One git command against a repository this file built under tmp_path.""" + return subprocess.run(["git", "-C", str(root), *args], + capture_output=True, text=True, check=True).stdout + + +def branches(root: Path) -> set[str]: + return set(git(root, "branch", "--format=%(refname:short)").split()) + + +def run(target_root: Path, harness_root: Path, story_id: str = STORY_ID): + """One story executed through the real coordinator and the fake runner.""" + run_dir = target_root / ".harness" / "runs" / story_id + runner = Runner(target_root, run_dir) + code = story_coordinator.run_story(story_id, harness_root, target_root, + runner) + return code, runner, run_dir + + +@pytest.fixture +def retired_target(target_root: Path) -> Path: + """A target whose configuration still carries the retired key.""" + configure(target_root, **{RETIRED: WORKING_RUNNER}) + return target_root + + +@pytest.fixture +def replacement_target(target_root: Path) -> Path: + """The same fixture carrying the replacement key instead. + + Everything else is identical, so every difference the tests below report + between the two is a difference the key name made. + """ + configure(target_root, **{REPLACEMENT: WORKING_RUNNER}) + return target_root + + +# -------------------------------------------------------------------------- +# 1. The declaration, and the function over it +# -------------------------------------------------------------------------- + + +def test_the_mapping_retires_the_key_this_story_retired_and_names_its_successor(): + assert harness_config.RETIRED_CONFIG_KEYS == {RETIRED: REPLACEMENT} + + +def test_every_retired_key_names_a_replacement_the_harness_actually_reads(): + """A refusal pointing at a key nothing reads would send a developer to a + name that does nothing, which is worse than the drift it prevents.""" + declared = harness_config.declared_config_keys() + for retired, replacement in harness_config.RETIRED_CONFIG_KEYS.items(): + assert replacement in declared, replacement + assert retired not in declared, retired + + +def test_a_config_carrying_a_retired_key_yields_one_problem_naming_both_names(): + problems = harness_config.retired_config_problems( + {"project": "sample", RETIRED: "/somewhere/bin/python"}) + assert len(problems) == 1, problems + assert RETIRED in problems[0] + assert REPLACEMENT in problems[0] + + +def test_a_config_carrying_no_retired_key_yields_nothing(): + """Beside its control: the same config with the retired key added is + reported, so the empty list is a fact about the config rather than a + function that reports nothing whatever it is handed.""" + clean = {"project": "sample", REPLACEMENT: "/somewhere/bin/runner", + "test_command": "echo tests-ok"} + assert harness_config.retired_config_problems(clean) == [] + assert harness_config.retired_config_problems({**clean, RETIRED: "x"}) + + +def test_this_repositorys_own_configuration_carries_the_replacement_and_not_it(): + """The shipped config was updated by this story rather than left to be + refused by the harness it ships with.""" + config = harness_config.load_config(REPO_ROOT) + assert config[REPLACEMENT] + assert harness_config.retired_config_problems(config) == [] + # The control for that absence: the same check over the same configuration + # with the retired key put back reports it. + assert harness_config.retired_config_problems( + {**config, RETIRED: config[REPLACEMENT]}) + + +# -------------------------------------------------------------------------- +# 2. The refusal, and what it leaves behind +# -------------------------------------------------------------------------- + + +def test_a_run_whose_config_carries_the_retired_key_is_refused( + retired_target, harness_root, capsys, +): + code, _, _ = run(retired_target, harness_root) + assert code == 1 + + refusal = capsys.readouterr().err + assert RETIRED in refusal + assert REPLACEMENT in refusal + # It says where to make the edit, not only that something is wrong. + assert str(retired_target / ".harness" / "config.yaml") in refusal + + +def test_the_refusal_leaves_no_run_directory_no_state_no_branch_and_no_agent( + retired_target, harness_root, +): + """Read off the refused target's tree, as the story asks, rather than off + the exit status alone. Its control is the next test, which makes the same + four observations of the same fixture carrying the replacement key and + finds all four present.""" + before = branches(retired_target) + + code, runner, run_dir = run(retired_target, harness_root) + + assert code == 1 + assert not run_dir.exists() + assert not (run_dir / "state.json").exists() + assert not (retired_target / ".harness" / "logs" / + f"{STORY_ID}.log").exists() + assert branches(retired_target) == before + assert runner.calls == [] + + +def test_the_same_fixture_with_the_replacement_key_creates_all_four( + replacement_target, harness_root, +): + """The control the absences above need, and the story's own criterion that + a configuration carrying the new key runs to completion.""" + before = branches(replacement_target) + + code, runner, run_dir = run(replacement_target, harness_root) + + assert code == 0, runner.calls + assert run_dir.is_dir() + assert json.loads((run_dir / "state.json").read_text( + encoding="utf-8"))["status"] == "completed" + assert (replacement_target / ".harness" / "logs" / + f"{STORY_ID}.log").is_file() + assert branches(replacement_target) - before == {f"story/{STORY_ID}"} + assert runner.calls == ["implementer", "tester", "verifier", "documenter"] + + +def test_the_replacement_key_is_the_runner_that_completed_run_recorded( + replacement_target, harness_root, +): + """Not merely that the run completed: the check it ran resolved the value + the configuration named, which is what makes the control a control on the + key rather than on the fixture.""" + _, _, run_dir = run(replacement_target, harness_root) + record = json.loads( + (run_dir / "clean-clone-result.json").read_text(encoding="utf-8")) + assert record["runner"] == WORKING_RUNNER + assert record["ran"] is True + assert record["command"].startswith(WORKING_RUNNER) + + +# -------------------------------------------------------------------------- +# 3. The ordering: above every other pre-flight +# -------------------------------------------------------------------------- + + +def test_the_refusal_precedes_the_workflow_being_loaded_at_all( + target_root, harness_root, capsys, +): + """The strongest ordering evidence available, because the two outcomes are + different in kind rather than in wording. + + A workflow name nothing ships cannot be loaded: without the retired key + the run raises reading it. With the retired key it refuses cleanly, which + can only happen if the retired-key check ran first — and the routing and + self-route pre-flights read that workflow, so they are below it too. + """ + configure(target_root, workflow="xyzzy-no-such-workflow") + with pytest.raises(OSError): + run(target_root, harness_root) + + configure(target_root, **{RETIRED: WORKING_RUNNER}) + code, runner, _ = run(target_root, harness_root) + assert code == 1 + assert RETIRED in capsys.readouterr().err + assert runner.calls == [] + + +#: Later pre-flights, each broken in a way that produces its own refusal, and +#: a fragment of the message that refusal alone would print. +LATER_PREFLIGHTS = ( + ("a story artifact that does not exist", "story-404", "story-404.yaml"), +) + + +@pytest.mark.parametrize("case,story_id,fragment", LATER_PREFLIGHTS) +def test_the_retired_key_is_what_speaks_when_a_later_pre_flight_is_also_broken( + target_root, harness_root, capsys, case, story_id, fragment, +): + """Its own control, in the same test: the identical fixture without the + retired key produces the later refusal, so the fixture really is broken in + the second way and the retired key really is what displaced it.""" + commit_setup(target_root, "the fixture as it stands") + later_code, later_runner, _ = run(target_root, harness_root, story_id) + later = capsys.readouterr().err + assert later_code == 1, case + assert fragment in later, later + assert later_runner.calls == [] + + configure(target_root, **{RETIRED: WORKING_RUNNER}) + code, runner, _ = run(target_root, harness_root, story_id) + refusal = capsys.readouterr().err + assert code == 1 + assert RETIRED in refusal + assert fragment not in refusal, refusal + assert runner.calls == [] + + +def test_a_dirty_tree_and_a_retired_key_together_report_the_retired_key( + retired_target, harness_root, capsys, +): + """The clean-tree pre-flight is the last one a developer meets before a + run directory exists, and it is below this one too.""" + (retired_target / "dirty.txt").write_text("the developer's own\n", + encoding="utf-8") + + code, runner, _ = run(retired_target, harness_root) + + refusal = capsys.readouterr().err + assert code == 1 + assert RETIRED in refusal + assert "dirty.txt" not in refusal, refusal + assert runner.calls == [] + + +def test_the_same_dirty_tree_alone_is_what_the_clean_tree_pre_flight_reports( + replacement_target, harness_root, capsys, +): + """The control for the test above: the dirty file really is a refusal of + its own, so the retired key displaced something rather than being the only + thing wrong.""" + (replacement_target / "dirty.txt").write_text("the developer's own\n", + encoding="utf-8") + + code, runner, _ = run(replacement_target, harness_root) + + assert code == 1 + assert "dirty.txt" in capsys.readouterr().err + assert runner.calls == [] + + +# -------------------------------------------------------------------------- +# 4. The shipped configuration behaves as it did before the rename +# +# The rename is only safe if this repository's own check still exercises the +# environment it exercised yesterday. That is a differential question, so it +# is answered differentially: the configuration is read at this story's own +# baseline through the shared resolution in `tests/conftest.py`, never as +# `HEAD` and never as the working tree against the repository root — the +# coordinator commits the tree when a run completes, so those comparisons go +# vacuously green the moment this story commits. +# +# This module declares no origin, so its range is this story's own run commit +# against that commit's parent, which is exactly the pair being compared. +# -------------------------------------------------------------------------- + +CONFIG_FILE = ".harness/config.yaml" + + +def config_value(text: str, key: str) -> str | None: + """One `key: value` line's value in a config file's text, or None. + + Written here rather than through `harness_config.load_config`, which takes + a repository root and so cannot read a file's text at a revision. Comments + are stripped, because the line this story cares most about carries one. + """ + for line in text.splitlines(): + stripped = line.split("#", 1)[0].strip() + if stripped.startswith(f"{key}:"): + return stripped.partition(":")[2].strip() + return None + + +def shipped_config_before() -> str: + return conftest.repository_file_at(CONFIG_FILE, validation_file=THIS_FILE, + bound=conftest.BASELINE) + + +def shipped_config_now() -> str: + return (REPO_ROOT / CONFIG_FILE).read_text(encoding="utf-8") + + +def test_the_rename_carried_the_value_the_retired_key_held(): + """The retired key's value became the replacement's, unchanged, and the + command it modifies is untouched. + + The absence — that the shipped file no longer carries the retired key — is + controlled by the same reader finding that key in the same file at the + baseline: it is looking in the right place with the right spelling. + """ + before, now = shipped_config_before(), shipped_config_now() + + retired_value = config_value(before, RETIRED) + assert retired_value, before + assert config_value(now, RETIRED) is None + assert config_value(now, REPLACEMENT) == retired_value + assert config_value(now, "test_command") == config_value(before, + "test_command") + + +def test_the_check_builds_the_same_command_from_the_renamed_key( + target_root, tmp_path, monkeypatch, +): + """Through the real construction rather than through a restatement of it. + + Both pairs — the configuration as it was and as it is — are handed to + `run_clean_clone` against the same throwaway repository, and the command + and runner it records must be identical. + + Neither runs. The configured executable is a relative path, which + `_resolve_interpreter` looks for under the target and then on PATH, and + PATH's own lookup of a path with a separator in it is relative to the + working directory — so the check is run from a throwaway directory, where + it resolves nowhere and both calls refuse before a clone is built. That + keeps this deterministic and free of any dependency on a second + environment being installed wherever the suite runs. + """ + monkeypatch.chdir(tmp_path) + before, now = shipped_config_before(), shipped_config_now() + + was = story_coordinator.run_clean_clone( + target_root, config_value(before, "test_command"), + config_value(before, RETIRED), tmp_path / "was") + is_now = story_coordinator.run_clean_clone( + target_root, config_value(now, "test_command"), + config_value(now, REPLACEMENT), tmp_path / "is-now") + + assert (was.ran, is_now.ran) == (False, False) + assert is_now.runner == was.runner + assert is_now.command == was.command + # The pair is a comparison rather than two constants: the same call with a + # runner the configuration does not name records a different command. + other = story_coordinator.run_clean_clone( + target_root, config_value(now, "test_command"), + "/xyzzy/bin/something-else", tmp_path / "other") + assert other.command != is_now.command + + +def test_the_record_that_configuration_produces_carries_no_version_field(): + """What the rename removed, asserted as a key set rather than as one + absence: the record `run_clean_clone` builds is exactly the keys the + schema declares, so a version field surviving under any spelling fails + here.""" + record = story_coordinator.CleanCloneResult( + ran=True, command="a-runner --all", runner="a-runner", + clone_path="/somewhere", exit_code=0, output_tail="").as_record() + schema = schema_validator.load_schema("clean-clone-result") + + assert set(record) <= set(schema["properties"]) + assert set(schema["required"]) == {"ran", "command", "runner"} + assert "python" not in json.dumps(schema) + # The control for that last absence: the same search over the same schema + # with the retired spelling put back reports it. + assert "python" in json.dumps({**schema, "properties": { + **schema["properties"], "python": {"type": "string"}}}) diff --git a/tests/test_revert_check.py b/tests/test_revert_check.py index a1e201a..00c7edf 100644 --- a/tests/test_revert_check.py +++ b/tests/test_revert_check.py @@ -725,7 +725,7 @@ def test_an_unresolvable_configured_interpreter_escalates_naming_why( target, harness_root, ): """The same treatment the clean-clone check gives it.""" - configure(target, clean_clone_python="nowhere/python") + configure(target, verification_runner="nowhere/python") code, _ = run(target, harness_root, {"implementer": forced_repair}) assert code == 2 record = record_of(target) @@ -777,7 +777,7 @@ def test_a_single_file_mixing_a_repair_and_an_addition_is_permitted( record = record_of(target) assert record["permitted"] is True assert record["paths"] == ["tests/test_app.py"] - assert set(record) <= {"ran", "paths", "command", "python", "python_version", + assert set(record) <= {"ran", "paths", "command", "runner", "clone_path", "exit_code", "output_tail", "permitted", "baseline", "reason"} diff --git a/tests/test_self_routing_retry.py b/tests/test_self_routing_retry.py index ba3450a..8f51153 100644 --- a/tests/test_self_routing_retry.py +++ b/tests/test_self_routing_retry.py @@ -1496,7 +1496,7 @@ def test_a_clean_clone_that_cannot_run_still_escalates_at_a_budgeted_stage( harness, workflow = budgeted_clean_clone target_root = build_target(tmp_path / "clean-clone-unrunnable", workflow=workflow["name"]) - configure(target_root, clean_clone_python="nowhere/python") + configure(target_root, verification_runner="nowhere/python") code, runner = drive(target_root, harness, workflow=workflow) diff --git a/tests/test_stage_baseline.py b/tests/test_stage_baseline.py index 95539f3..0223304 100644 --- a/tests/test_stage_baseline.py +++ b/tests/test_stage_baseline.py @@ -58,8 +58,9 @@ import pytest -from conftest import (BASELINE as PRE_STORY_BOUND, STORY, first_retry_route, - function_source_at, load_mutant, story_diff) +from conftest import (BASELINE as PRE_STORY_BOUND, ENDPOINT, STORY, + first_retry_route, function_source_at, load_mutant, + story_diff) import harness_config import schema_validator @@ -1064,21 +1065,26 @@ def test_the_check_and_the_clone_builder_are_byte_for_byte_pre_story(): """Only the state reverted to changed. The control is the capture, in the same file at the same bound, which did change — so a comparison that had stopped resolving anything could not pass both halves. + + Both bounds are this story's own commit range. The after side read the + *working tree* until the-interpreter-is-not-assumed-to-be-python renamed + the record's interpreter field, which this story has nothing to say + about — the standing HEAD-baseline trap, repaired the standing way by + bounding the comparison at both ends rather than by relaxing it. """ - def before(name: str) -> str: + def at(name: str, bound: str) -> str: return function_source_at(COORDINATOR_REL, name, validation_file=Path(__file__), - bound=PRE_STORY_BOUND, repo=REPO_ROOT) + bound=bound, repo=REPO_ROOT) for name in ("revert_check", "_build_clone", "_revert_check_permitted", "run_clean_clone", "governed_edits"): - assert before(name) == inspect.getsource( - getattr(story_coordinator, name)), name + assert at(name, PRE_STORY_BOUND) == at(name, ENDPOINT), name - assert before("capture_stage_baseline") \ - != inspect.getsource(story_coordinator.capture_stage_baseline) - assert before("stage_baseline_dir") \ - != inspect.getsource(story_coordinator.stage_baseline_dir) + assert at("capture_stage_baseline", PRE_STORY_BOUND) \ + != at("capture_stage_baseline", ENDPOINT) + assert at("stage_baseline_dir", PRE_STORY_BOUND) \ + != at("stage_baseline_dir", ENDPOINT) def test_a_forced_edit_and_an_unforced_one_on_a_single_attempt_still_decide(