From 6f6d4931db256ee2f126ca2cd419d9bcf56c94a8 Mon Sep 17 00:00:00 2001 From: "jerod.wilkerson" <30474318+jerodw@users.noreply.github.com> Date: Sat, 15 Aug 2026 17:54:41 -0600 Subject: [PATCH 1/2] story-043: An undeclared config key is refused Implemented by the l5 harness story workflow. --- .harness/config.yaml | 1 - .harness/docs/ARCHITECTURE.md | 10 +- orchestration/harness_config.py | 48 +- orchestration/story_coordinator.py | 27 +- schemas/harness-config.schema.json | 2 +- scripts/l5-init | 2 +- templates/config.yaml | 1 - tests/conftest.py | 1 - tests/test_changed_files_records.py | 2 +- tests/test_config_keys_are_obeyed.py | 33 +- tests/test_escalation_resume.py | 1 - tests/test_escalation_summary.py | 1 - tests/test_foreign_work_refusal.py | 32 +- tests/test_harness_config.py | 1 - tests/test_l5_status_cli.py | 1 - .../test_no_target_stack_in_harness_source.py | 3 - tests/test_plan_commit.py | 1 - tests/test_required_output_freshness.py | 1 - tests/test_rerun_refusal.py | 1 - tests/test_resume_guard.py | 1 - tests/test_retired_config_keys.py | 513 ---------- tests/test_retry_routing.py | 1 - tests/test_revert_baseline.py | 1 - tests/test_revert_check.py | 1 - tests/test_run_status.py | 1 - tests/test_self_routing_retry.py | 1 - tests/test_stage_baseline.py | 1 - tests/test_undeclared_config_keys.py | 934 ++++++++++++++++++ 28 files changed, 1023 insertions(+), 600 deletions(-) delete mode 100644 tests/test_retired_config_keys.py create mode 100644 tests/test_undeclared_config_keys.py diff --git a/.harness/config.yaml b/.harness/config.yaml index e51ca28..8c3fd89 100644 --- a/.harness/config.yaml +++ b/.harness/config.yaml @@ -1,5 +1,4 @@ # Target-repository harness configuration -project: level-five workflow: story-workflow branch_prefix: story/ # The branch a story's branch is cut from, and the branch l5-plan expects to diff --git a/.harness/docs/ARCHITECTURE.md b/.harness/docs/ARCHITECTURE.md index 25c9ef6..cdac971 100644 --- a/.harness/docs/ARCHITECTURE.md +++ b/.harness/docs/ARCHITECTURE.md @@ -62,9 +62,13 @@ Since story-032 the `create` value is **a path at or beneath** one of that stage 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. +Since story-043 the declaration is also a **run-time check, and it is strict**: `harness_config.undeclared_config_problems(config, harness_root=None)` calls `declared_config_keys` and returns one problem per key a loaded config carries that the schema does not declare, in the order the config carries them. `run_story` calls it immediately after `load_config` and refuses through `_refuse_undeclared_config_keys` and the shared `refuse()` — **above** the workflow load and above every other pre-flight, so a refused run creates no run directory, no state file, no log, no branch, and invokes no agent. Only key *names* are examined; no value is validated, coerced or constrained, and `load_config`'s parsing is untouched, so a comment naming an unknown key is stripped before any key is seen and refuses nothing. `tests/test_undeclared_config_keys.py` holds the refusal's coverage — it replaced `tests/test_retired_config_keys.py`, whose subject no longer exists — and includes a sweep asserting that no fixture configuration under `tests/` carries an undeclared key, which is what keeps the rest of the suite runnable under the strict rule. -**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. +**Each problem names the offending key and lists the declared set** — `'' is not a key the harness reads; it reads: …` — the shape story-028's routing refusal takes, because a bare "unknown key" would leave the developer to find the vocabulary themselves. The declared set is composed into each problem rather than appended by the coordinator, so the function is self-contained and the refusal is actionable without opening the schema. + +**This replaced a by-name retirement mapping rather than joining it.** story-041 left `RETIRED_CONFIG_KEYS` — one entry, `clean_clone_python` → `verification_runner` — and refused that name alone; story-043 deleted the mapping and its function, so the retired key is now refused as one more undeclared key and the literal went with the mechanism. That is what took the last target-stack language name out of `orchestration/`, and it is the *reason* a sibling mapping must not come back: any retirement mapping reintroduces the tie it removed. Two things fell out of the one change. A mistyped key stops being silently ignored — `branch_prefixx: story/` used to run, quietly take the default, and be discovered from the branch name. And the cost, stated plainly: a config carrying `clean_clone_python` is told the key is not one the harness reads rather than being told what replaced it; someone upgrading finds the replacement in the schema instead. + +**`project` was removed rather than declared**, from `.harness/config.yaml`, `templates/config.yaml`, the `{project}` substitution in `scripts/l5-init`, and every fixture configuration under `tests/`. It was the one key a shipped config carried that nothing reads, and under the strict rule it would have refused this repository's own config. Declaring it was not available: the declared set *is* the set of keys the harness reads, and story-039's three coverage checks require every declared key to be read, to carry a proof, and for that proof to go red when its read is replaced by its fallback — none of which a key nothing reads can satisfy. Adding a key to `templates/config.yaml` therefore now means adding it to the schema and giving it a proof, or not adding it at all. 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. @@ -421,7 +425,7 @@ 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` (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. +**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` (8 entries since story-043, 9 before it) 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 shebang each `scripts/l5-*` entry point carries names the interpreter *this* harness runs under — and **every entry carries a one-line reason**, so the permanent half is a judgement on the record rather than a suppression list. The ninth entry was `orchestration/harness_config.py`, whose `RETIRED_CONFIG_KEYS` spelled a retired key; story-043 deleted the mapping, and the allowlist entry had to go with it or the two-way set equality would fail. Only two entries name a file under `orchestration/` now, both docstrings about this code: `story_parser.py`'s type-coercion note and `story_coordinator.py`'s `self_route_problems`. The two lists together equal exactly what the scan reports against this repository (11 distinct `(path, line text)` pairs from 12 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 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. diff --git a/orchestration/harness_config.py b/orchestration/harness_config.py index b77813d..936836c 100644 --- a/orchestration/harness_config.py +++ b/orchestration/harness_config.py @@ -13,34 +13,34 @@ import schema_validator # The declaration of which keys the harness reads, beside the artifact -# schemas. It is a declaration and not a run-time check: nothing here -# validates a target's config file against it, and no unknown key is -# refused. +# schemas. It is also what a target's config file is checked against at +# pre-flight: a key the schema does not declare refuses the run. 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 undeclared_config_problems( + config: dict, harness_root: Path | None = None +) -> list[str]: + """One problem per key a loaded config carries that the schema does not declare. -def retired_config_problems(config: dict) -> list[str]: - """One problem per retired key a loaded config still carries. + The declared set is the set of keys the harness reads, so a key outside + it is a key nothing will ever act on — a retired name left behind after + a rename, or a mistyping of a declared one. Either is refused rather + than ignored, because ignoring it lets the run fall through to a + default and quietly do something other than what the config asked for. - 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 + Each problem names the offending key and lists the declared set, the + shape the routing refusal takes: a bare "unknown key" would leave the + developer to find the vocabulary themselves. Problems come back in the + order the config carries the keys, and an empty list is the whole of "this config carries none". """ + declared = declared_config_keys(harness_root) + listed = ", ".join(declared) 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 + f"'{key}' is not a key the harness reads; it reads: {listed}" + for key in config + if key not in declared ] @@ -90,10 +90,10 @@ def declared_config_keys(harness_root: Path | None = None) -> tuple[str, ...]: this module's package exactly as the artifact schemas are, and read through schema_validator.load_schema so schemas/ keeps one reader. - The declaration is not a run-time check. Nothing calls this while a run - is executing, no target's config file is validated against it, and no - unknown key is refused; what reads it is the coverage that asserts set - equality against the keys the harness actually reads. + Two things read it. The coverage asserts set equality against the keys + the harness actually reads, and `undeclared_config_problems` checks a + loaded config against it at pre-flight, so a key the schema does not + declare refuses the run rather than being silently ignored. A missing, unparseable or wrong-shaped schema raises ValueError naming the path, rather than degrading to an empty or partial tuple, which diff --git a/orchestration/story_coordinator.py b/orchestration/story_coordinator.py index bc291e4..9e599e6 100644 --- a/orchestration/story_coordinator.py +++ b/orchestration/story_coordinator.py @@ -2329,8 +2329,8 @@ 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. +def _refuse_undeclared_config_keys(target_root: Path, problems: list[str]) -> int: + """Refuse a run whose configuration carries a key the harness does not read. 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 @@ -2338,9 +2338,9 @@ def _refuse_retired_config_keys(target_root: Path, problems: list[str]) -> int: """ return refuse( f"{target_root / '.harness' / 'config.yaml'} carries configuration keys " - f"the harness no longer reads:", + f"the harness does not read:", problems, - "Rename each key to its replacement before running a story.", + "Remove or correct each key before running a story.", ) @@ -2532,15 +2532,16 @@ def run_story( """ 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) + # Pre-flight: a key the schema does not declare is refused rather than + # ignored. Ignoring one lets the run fall back to a default and quietly + # exercise something other than what the config asked for — a retired name + # left after a rename, or a mistyping of a declared key. 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. + undeclared = harness_config.undeclared_config_problems(config, harness_root) + if undeclared: + return _refuse_undeclared_config_keys(target_root, undeclared) workflow = harness_config.load_workflow(harness_root, config.get("workflow", "story-workflow")) rules = harness_config.load_rules(harness_root) diff --git a/schemas/harness-config.schema.json b/schemas/harness-config.schema.json index a1d0b85..8a131d2 100644 --- a/schemas/harness-config.schema.json +++ b/schemas/harness-config.schema.json @@ -1,7 +1,7 @@ { "$schema": "https://json-schema.org/draft/2020-12/schema", "title": "harness-config", - "description": "The set of keys the harness reads out of a target repository's .harness/config.yaml. The declared properties are the whole of that set: a key a target's config file carries and no harness code reads is out of the set by construction, which is why 'project' is absent. This file is a declaration rather than a run-time check — nothing validates a target's config file against it during a run and no unknown key is refused, so a target carrying an extra key runs exactly as it would without this schema. It is a contract rather than an artifact shape, so no stage is asked to produce or satisfy it; what holds it is coverage, asserted as set equality against declared_config_keys() in both directions. Every declared key carries a proof that varies it to a value the harness would never pick and observes the harness obey it. Types are the types harness_config.load_config produces: every scalar is a string, because the config parser never coerces, and a list-valued key is an array of strings.", + "description": "The set of keys the harness reads out of a target repository's .harness/config.yaml. The declared properties are the whole of that set: a key a target's config file carries and no harness code reads is out of the set by construction. This file is checked against a target's config at pre-flight: a key it does not declare refuses the run, naming the offending key and this declared set, so a mistyped key stops rather than silently taking a default. It is a contract rather than an artifact shape, so no stage is asked to produce or satisfy it; what holds it is coverage, asserted as set equality against declared_config_keys() in both directions. Every declared key carries a proof that varies it to a value the harness would never pick and observes the harness obey it. Types are the types harness_config.load_config produces: every scalar is a string, because the config parser never coerces, and a list-valued key is an array of strings.", "type": "object", "required": [], "properties": { diff --git a/scripts/l5-init b/scripts/l5-init index 781b566..1eb7f8d 100755 --- a/scripts/l5-init +++ b/scripts/l5-init @@ -34,7 +34,7 @@ def main() -> int: (harness_dir / sub).mkdir(parents=True, exist_ok=True) config = (TEMPLATES / "config.yaml").read_text(encoding="utf-8") - config = config.replace("{project}", target.name).replace("{test_command}", test_command) + config = config.replace("{test_command}", test_command) (harness_dir / "config.yaml").write_text(config, encoding="utf-8") for template in sorted((TEMPLATES / "standards").glob("*.md")): diff --git a/templates/config.yaml b/templates/config.yaml index 5f6223e..e86d2eb 100644 --- a/templates/config.yaml +++ b/templates/config.yaml @@ -1,5 +1,4 @@ # Target-repository harness configuration (created by l5-init) -project: {project} workflow: story-workflow branch_prefix: story/ permission_mode: acceptEdits diff --git a/tests/conftest.py b/tests/conftest.py index 1c604f9..269c6fe 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -441,7 +441,6 @@ def _committed(repo: Path, relative: str) -> bool: """ CONFIG = """\ -project: sample-target workflow: story-workflow branch_prefix: story/ permission_mode: acceptEdits diff --git a/tests/test_changed_files_records.py b/tests/test_changed_files_records.py index 04d1eb6..bf3117e 100644 --- a/tests/test_changed_files_records.py +++ b/tests/test_changed_files_records.py @@ -125,7 +125,7 @@ def test_enforcement_follows_declaration_not_stage_name(target_root, harness_roo """Removing the tester's changed_files declaration disables its check, proving enforcement is driven by the workflow definition.""" harness_copy = tmp_path / "harness-copy" - for sub in ("prompts", "workflows", "rules"): + for sub in ("prompts", "workflows", "rules", "schemas"): shutil.copytree(harness_root / sub, harness_copy / sub) workflow_path = harness_copy / "workflows" / "story-workflow.json" workflow = json.loads(workflow_path.read_text()) diff --git a/tests/test_config_keys_are_obeyed.py b/tests/test_config_keys_are_obeyed.py index 7cd2517..ededb02 100644 --- a/tests/test_config_keys_are_obeyed.py +++ b/tests/test_config_keys_are_obeyed.py @@ -303,8 +303,7 @@ def node_id(self) -> str: def _yaml_lines(values: dict[str, object]) -> str: - lines = ["# fixture configuration for story-039's proofs", - "project: xyzzy-target"] + lines = ["# fixture configuration for story-039's proofs"] for key in sorted(values): value = values[key] if isinstance(value, list): @@ -561,11 +560,10 @@ def test_the_schema_is_in_the_inventory_and_declares_no_key_the_harness_ignores( schema = schema_validator.load_schema("harness-config") assert "additionalProperties" not in json.dumps(schema) assert schema_validator.unsupported_keywords(schema) == [] - # `project` is the live instance of a key a target's config file carries - # and no harness code reads. Its absence is the schema's top-level claim - # made concrete: the declared set is what the harness *reads*. - assert "project" in THIS_REPO_CONFIG - assert "project" not in DECLARED + # The schema's top-level claim made concrete, now that an undeclared key + # refuses the run: this repository's own config carries nothing outside + # the declared set, and the declared set is what the harness *reads*. + assert [key for key in THIS_REPO_CONFIG if key not in DECLARED] == [] #: The three shapes `declared_config_keys` must raise on rather than degrade @@ -1183,17 +1181,16 @@ def test_the_unchanged_comparison_can_tell_a_changed_path_apart(tmp_path): assert unchanged == "" -def test_no_key_gained_a_run_time_check_and_no_unknown_key_is_refused(tmp_path): - """The declaration is a declaration. A target carrying an extra key runs. +def test_a_key_the_schema_does_not_declare_refuses_the_run(tmp_path): + """The declaration is also the run-time check, since story-043. - Constructed rather than argued: a fixture target is given a key the schema - does not declare, and the run completes exactly as it does without it. + Constructed rather than argued: the same fixture target that completes + without it is given a key the schema does not declare, and the run is + refused instead. The control is `complete_run` everywhere else in this + module — the fixture is otherwise identical. """ - run = complete_run(tmp_path, xyzzy_undeclared_key="something-nobody-reads") - assert run.config["xyzzy_undeclared_key"] == "something-nobody-reads" + run = start_run(tmp_path, xyzzy_undeclared_key="something-nobody-reads") assert "xyzzy_undeclared_key" not in DECLARED - assert run.state["status"] == "completed" - # And nothing in the coordinator's path calls the reader of the schema. - assert "declared_config_keys" not in ( - REPO_ROOT / "orchestration" / "story_coordinator.py" - ).read_text(encoding="utf-8") + assert run.code == 1 + assert run.stages == [] + assert not run.run_dir.exists() diff --git a/tests/test_escalation_resume.py b/tests/test_escalation_resume.py index 20eaf3c..52027aa 100644 --- a/tests/test_escalation_resume.py +++ b/tests/test_escalation_resume.py @@ -140,7 +140,6 @@ def failing(attempt: int, *, retry: bool) -> dict: """ CONFIG = """\ -project: resume-target workflow: story-workflow branch_prefix: story/ permission_mode: acceptEdits diff --git a/tests/test_escalation_summary.py b/tests/test_escalation_summary.py index 997277f..f0799b1 100644 --- a/tests/test_escalation_summary.py +++ b/tests/test_escalation_summary.py @@ -132,7 +132,6 @@ def failing(attempt: int, *, retry: bool) -> dict: """ CONFIG = """\ -project: summary-target workflow: story-workflow branch_prefix: story/ permission_mode: acceptEdits diff --git a/tests/test_foreign_work_refusal.py b/tests/test_foreign_work_refusal.py index d2e33b4..996d0a9 100644 --- a/tests/test_foreign_work_refusal.py +++ b/tests/test_foreign_work_refusal.py @@ -133,7 +133,6 @@ def failing(attempt: int, *, retry: bool) -> dict: """ CONFIG = """\ -project: clean-tree-target workflow: story-workflow branch_prefix: story/ permission_mode: acceptEdits @@ -1141,18 +1140,39 @@ def test_attempting_the_bypass_does_not_bypass_it( """The search above says there is no key to find; this says that setting them anyway changes nothing. - The control is the same target, with every one of those variables and keys - still set, run once the tree is clean — which proceeds. So the refusal is - the dirty tree rather than the extra configuration having broken the run. + Since an undeclared configuration key refuses the run at pre-flight, a + bypass written as a key cannot even be honoured: each attempted name is + reported as a key the harness does not read, and the run is refused before + the tree is looked at. The environment attempts are then carried into the + dirty-tree refusal itself, which still speaks. + + The control is the same target, with every one of those variables still + set, run once the tree is clean — which proceeds. So the refusal is the + dirty tree rather than the extra configuration having broken the run. """ target = make_target("bypass-target") for name in ENV_ATTEMPTS: monkeypatch.setenv(name, "1") config = target / ".harness" / "config.yaml" - write(config, config.read_text(encoding="utf-8") - + "".join(f"{key}\n" for key in CONFIG_ATTEMPTS)) + declared = config.read_text(encoding="utf-8") + write(config, declared + "".join(f"{key}\n" for key in CONFIG_ATTEMPTS)) commit(target, "every escape hatch a developer would try") + # A clean tree, so nothing else could be refusing: each attempted key is + # reported as one the harness does not read, rather than being honoured. + capsys.readouterr() + code, runner = run(target, harness_root) + assert code == 1 + assert runner.calls == [] + refusal = capsys.readouterr().err + for attempt in CONFIG_ATTEMPTS: + assert f"'{attempt.split(':')[0]}' is not a key the harness reads" in refusal + + # The attempted keys taken back out, and the tree dirtied: the refusal a + # developer meets is the tree, and the environment variables set above do + # not change it. + write(config, declared) + commit(target, "the attempted keys come back out") write(target / STRAY, "no stage wrote this\n") capsys.readouterr() code, runner = run(target, harness_root) diff --git a/tests/test_harness_config.py b/tests/test_harness_config.py index 28f9f34..6a582af 100644 --- a/tests/test_harness_config.py +++ b/tests/test_harness_config.py @@ -6,7 +6,6 @@ def test_quoted_values_are_unquoted(tmp_path: Path): (tmp_path / ".harness").mkdir() (tmp_path / ".harness" / "config.yaml").write_text( - 'project: sample\n' 'test_command: "echo ok"\n' 'allowed_tools:\n' ' - "Bash(.venv/bin/python:*)"\n' diff --git a/tests/test_l5_status_cli.py b/tests/test_l5_status_cli.py index 53c4eb9..499fa7a 100644 --- a/tests/test_l5_status_cli.py +++ b/tests/test_l5_status_cli.py @@ -15,7 +15,6 @@ L5_STATUS = HARNESS_ROOT / "scripts" / "l5-status" CONFIG = """\ -project: cli-target runs_dir: .harness/runs """ diff --git a/tests/test_no_target_stack_in_harness_source.py b/tests/test_no_target_stack_in_harness_source.py index 2e1ceec..20cbaa6 100644 --- a/tests/test_no_target_stack_in_harness_source.py +++ b/tests/test_no_target_stack_in_harness_source.py @@ -142,9 +142,6 @@ ('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", } diff --git a/tests/test_plan_commit.py b/tests/test_plan_commit.py index 5fa4af8..2ec44dd 100644 --- a/tests/test_plan_commit.py +++ b/tests/test_plan_commit.py @@ -76,7 +76,6 @@ } CONFIG = """\ -project: plan-target workflow: story-workflow branch_prefix: story/ permission_mode: acceptEdits diff --git a/tests/test_required_output_freshness.py b/tests/test_required_output_freshness.py index a2a4b9b..42ba903 100644 --- a/tests/test_required_output_freshness.py +++ b/tests/test_required_output_freshness.py @@ -133,7 +133,6 @@ def failing(attempt: int) -> dict: """ CONFIG = """\ -project: freshness-target workflow: story-workflow branch_prefix: story/ permission_mode: acceptEdits diff --git a/tests/test_rerun_refusal.py b/tests/test_rerun_refusal.py index 33fe351..03b17d5 100644 --- a/tests/test_rerun_refusal.py +++ b/tests/test_rerun_refusal.py @@ -123,7 +123,6 @@ """ CONFIG = """\ -project: finished-branch-target workflow: story-workflow branch_prefix: story/ permission_mode: acceptEdits diff --git a/tests/test_resume_guard.py b/tests/test_resume_guard.py index b587f08..788a0c4 100644 --- a/tests/test_resume_guard.py +++ b/tests/test_resume_guard.py @@ -115,7 +115,6 @@ """ CONFIG = """\ -project: shared-root-target workflow: story-workflow branch_prefix: story/ permission_mode: acceptEdits diff --git a/tests/test_retired_config_keys.py b/tests/test_retired_config_keys.py deleted file mode 100644 index 711e78d..0000000 --- a/tests/test_retired_config_keys.py +++ /dev/null @@ -1,513 +0,0 @@ -"""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_retry_routing.py b/tests/test_retry_routing.py index e83ff35..583dc5a 100644 --- a/tests/test_retry_routing.py +++ b/tests/test_retry_routing.py @@ -160,7 +160,6 @@ def failing(target=OMITTED, *, retry: bool = True) -> dict: """ CONFIG = """\ -project: routing-target workflow: {workflow} branch_prefix: story/ permission_mode: acceptEdits diff --git a/tests/test_revert_baseline.py b/tests/test_revert_baseline.py index 54869cf..cb26294 100644 --- a/tests/test_revert_baseline.py +++ b/tests/test_revert_baseline.py @@ -92,7 +92,6 @@ "-p", "no:cacheprovider"]) CONFIG = f"""\ -project: suite-target workflow: story-workflow branch_prefix: story/ permission_mode: acceptEdits diff --git a/tests/test_revert_check.py b/tests/test_revert_check.py index 00c7edf..819a02c 100644 --- a/tests/test_revert_check.py +++ b/tests/test_revert_check.py @@ -76,7 +76,6 @@ "-p", "no:cacheprovider"]) CONFIG = f"""\ -project: suite-target workflow: story-workflow branch_prefix: story/ permission_mode: acceptEdits diff --git a/tests/test_run_status.py b/tests/test_run_status.py index 92087db..012460a 100644 --- a/tests/test_run_status.py +++ b/tests/test_run_status.py @@ -8,7 +8,6 @@ from story_coordinator import RunState CONFIG = """\ -project: sample-target runs_dir: .harness/runs """ diff --git a/tests/test_self_routing_retry.py b/tests/test_self_routing_retry.py index 8f51153..2324d7d 100644 --- a/tests/test_self_routing_retry.py +++ b/tests/test_self_routing_retry.py @@ -253,7 +253,6 @@ def test_the_no_model_guard_fires_when_a_model_is_invoked(tmp_path): """ CONFIG = """\ -project: self-route-target workflow: {workflow} branch_prefix: story/ permission_mode: acceptEdits diff --git a/tests/test_stage_baseline.py b/tests/test_stage_baseline.py index 0223304..d4e9612 100644 --- a/tests/test_stage_baseline.py +++ b/tests/test_stage_baseline.py @@ -103,7 +103,6 @@ "-p", "no:cacheprovider"]) CONFIG = f"""\ -project: suite-target workflow: story-workflow branch_prefix: story/ permission_mode: acceptEdits diff --git a/tests/test_undeclared_config_keys.py b/tests/test_undeclared_config_keys.py new file mode 100644 index 0000000..977d6f6 --- /dev/null +++ b/tests/test_undeclared_config_keys.py @@ -0,0 +1,934 @@ +"""Independent validation for story-043's undeclared-key refusal. + +A key a target's `.harness/config.yaml` carries that +`schemas/harness-config.schema.json` does not declare stops the run at +pre-flight. The declared set is the set of keys the harness reads, so a key +outside it is a key nothing will ever act on — a retired name left behind +after a rename, or a mistyping of a declared one. Both used to run: the +first because only `clean_clone_python` was refused by name, the second +because nothing looked at unknown keys at all, so `branch_prefixx: story/` +ran, quietly took the default, and the developer found out from the branch +name. + +Written from the story's acceptance criteria rather than from the +implementation, at four altitudes: + + * **the function.** `harness_config.undeclared_config_problems` is a pure + function over a loaded config, so it is driven directly: which keys it + reports, in which order, and what each problem says. + * **the refusal.** Throwaway targets carrying the retired key and a + mistyped key are 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 + undeclared key win — including a workflow name that cannot be loaded at + all, which raises without the undeclared key and refuses cleanly with it. + * **the configurations this repository ships.** Its own config, the + template, and what `scripts/l5-init` writes must all load without + refusal, which is the regression a strict rule is most likely to cause. + +Every absence asserted here carries a demonstration that it can fail: + + * "the refused run created no run directory, no state file, no log, no + branch and invoked no agent" sits beside the same fixture without the + offending key, where the same five observations report all five; + * "this configuration carries no undeclared key" sits beside the same + check over the same configuration with an undeclared key put back; + * "a comment naming a retired key is not refused" sits beside the same + line with its `#` removed, which is refused; + * "the retired mechanism's three names appear nowhere in the repository" + sits beside the same scan asked for a name that does exist; + * "the scan reports nothing under orchestration/ beyond the two + legitimate mentions" sits beside a throwaway root with a tie planted + under orchestration/, which the same scan reports. + +Nothing here invokes a model: every run goes through the fake runner below. +""" +import json +import re +import shutil +import subprocess +import sys +from pathlib import Path + +import pytest + +import harness_config +import harness_source +import story_coordinator +from agent_runner import AgentResult +from conftest import commit_setup + +import test_no_target_stack_in_harness_source as stack_module + +REPO_ROOT = Path(harness_config.__file__).resolve().parents[1] + +#: The key story-041 retired and story-043 stops naming. It is written here +#: from the story's words rather than imported from anything under test: the +#: point of this story is that no source file spells it any more, so a module +#: that read it out of the harness would have nowhere to read it from. +RETIRED = "clean_clone_python" + +#: A mistyping of a declared key — the new capability. `branch_prefix` is +#: declared and defaults to `story/`, so before this story a run carrying +#: this spelling completed on the default branch name and said nothing. +MISTYPED = "branch_prefixx" + +STORY_ID = "story-001" + +#: A runner that exists on every platform this suite runs on, so a control +#: run's clean-clone check resolves it and the suite it runs exits zero. +WORKING_RUNNER = "/bin/echo" + +#: The stem of the three names story-043 deletes. None of the three may appear +#: in the repository outside `.harness/runs/`, and this module is scanned along +#: with the rest of it — so the names are composed here rather than written, +#: or the scan below would report the module making the claim. +_STEM = "retired" + "_config_" + +#: The mapping, the function that read it, and the coordinator's pre-flight. +DELETED_NAMES = ( + (_STEM + "keys").upper(), + _STEM + "problems", + "_refuse_" + _STEM + "keys", +) + +#: The control for that absence: a name the change introduced, so the same +#: scan over the same files is known to be able to see a name at all. +SURVIVING_NAME = "undeclared_config_problems" + +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 deconfigure(target_root: Path, *keys: str) -> None: + """Remove `key: value` lines from the target's config, and commit.""" + path = target_root / ".harness" / "config.yaml" + kept = [line for line in path.read_text(encoding="utf-8").splitlines() + if not any(line.startswith(f"{key}:") for key in keys)] + path.write_text("\n".join(kept) + "\n", encoding="utf-8") + commit_setup(target_root, "remove config keys for this test") + + +def git(root: Path, *args: str) -> str: + """One git command against a repository 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 sound_target(target_root: Path) -> Path: + """The shared fixture with a resolvable verification runner and nothing + undeclared, so a run through it completes. + + Every refused fixture below is this one plus a key, so each difference a + test reports is a difference that key made. + """ + configure(target_root, verification_runner=WORKING_RUNNER) + return target_root + + +@pytest.fixture(params=[RETIRED, MISTYPED], ids=["retired", "mistyped"]) +def offending_key(request) -> str: + """The two kinds of undeclared key the story names, so every guarantee + below is asserted of both rather than of the retired one alone.""" + return request.param + + +# -------------------------------------------------------------------------- +# 1. The function over a loaded config +# -------------------------------------------------------------------------- + + +def test_the_retired_key_is_now_reported_as_a_key_the_harness_does_not_read(): + problems = harness_config.undeclared_config_problems( + {"test_command": "echo ok", RETIRED: "/somewhere/bin/python"}) + assert len(problems) == 1, problems + assert RETIRED in problems[0] + + +def test_a_mistyping_of_a_declared_key_is_reported_too(): + """The capability this story adds. Its own control sits in the same + assertion: the correctly spelled key beside it is not reported.""" + problems = harness_config.undeclared_config_problems( + {"branch_prefix": "story/", MISTYPED: "story/"}) + assert len(problems) == 1, problems + assert MISTYPED in problems[0] + assert "'branch_prefix'" not in problems[0], problems[0] + + +def test_every_problem_names_the_offending_key_and_lists_the_declared_set(): + declared = harness_config.declared_config_keys() + problems = harness_config.undeclared_config_problems( + {RETIRED: "x", MISTYPED: "y"}) + + assert len(problems) == 2, problems + for key, problem in zip((RETIRED, MISTYPED), problems): + assert key in problem + for name in declared: + assert name in problem, (name, problem) + + +def test_the_declared_set_is_the_thirteen_the_schema_carries_and_no_more(): + """The story's constraint that this change adds no key and removes none. + + Read out of the schema file itself rather than out of the function that + reads it, so the two are compared rather than one restating the other. + """ + schema = json.loads( + (REPO_ROOT / "schemas" / "harness-config.schema.json").read_text( + encoding="utf-8")) + assert tuple(schema["properties"]) == harness_config.declared_config_keys() + assert len(schema["properties"]) == 13, sorted(schema["properties"]) + assert RETIRED not in schema["properties"] + assert "project" not in schema["properties"] + + +def test_problems_come_back_in_the_order_the_config_carries_the_keys(): + forwards = harness_config.undeclared_config_problems( + {"aaa": "1", "test_command": "echo ok", "zzz": "2"}) + backwards = harness_config.undeclared_config_problems( + {"zzz": "2", "test_command": "echo ok", "aaa": "1"}) + + assert [p.split("'")[1] for p in forwards] == ["aaa", "zzz"] + assert [p.split("'")[1] for p in backwards] == ["zzz", "aaa"] + + +def test_a_config_carrying_only_declared_keys_yields_nothing(): + """Beside its control: the same config with one 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 = {key: "value" for key in harness_config.declared_config_keys()} + assert harness_config.undeclared_config_problems(clean) == [] + assert harness_config.undeclared_config_problems({**clean, MISTYPED: "x"}) + + +def test_the_empty_config_yields_nothing(): + """No key is required — the schema's `required` is empty — so a config + carrying nothing is a config carrying nothing undeclared.""" + assert harness_config.undeclared_config_problems({}) == [] + + +# -------------------------------------------------------------------------- +# 2. The refusal, and what it leaves behind +# -------------------------------------------------------------------------- + + +def test_a_run_whose_config_carries_an_undeclared_key_is_refused( + sound_target, harness_root, capsys, offending_key, +): + configure(sound_target, **{offending_key: "whatever"}) + + code, _, _ = run(sound_target, harness_root) + + assert code == 1 + refusal = capsys.readouterr().err + assert offending_key in refusal + # It says where to make the edit, not only that something is wrong. + assert str(sound_target / ".harness" / "config.yaml") in refusal + + +def test_the_refusal_message_lists_the_keys_the_harness_does_declare( + sound_target, harness_root, capsys, offending_key, +): + """Its control is in the same assertion: a name that is not declared and + is not the offending key must not appear, so "every declared name is in + the text" is not satisfied by a message that names everything.""" + configure(sound_target, **{offending_key: "whatever"}) + + run(sound_target, harness_root) + + refusal = capsys.readouterr().err + for name in harness_config.declared_config_keys(): + assert name in refusal, name + assert "clean_clone_interpreter" not in refusal, refusal + + +def test_the_refusal_leaves_no_run_directory_no_state_no_log_no_branch_and_no_agent( + sound_target, harness_root, offending_key, +): + """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 + five observations of the same fixture without the key and finds all five + present.""" + configure(sound_target, **{offending_key: "whatever"}) + before = branches(sound_target) + + code, runner, run_dir = run(sound_target, harness_root) + + assert code == 1 + assert not run_dir.exists() + assert not (run_dir / "state.json").exists() + assert not (sound_target / ".harness" / "logs" / f"{STORY_ID}.log").exists() + assert branches(sound_target) == before + assert runner.calls == [] + + +def test_the_same_fixture_without_the_key_creates_all_five( + sound_target, harness_root, +): + """The control the absences above need, and the story's own criterion + that a config carrying nothing undeclared reaches its stages.""" + before = branches(sound_target) + + code, runner, run_dir = run(sound_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 (sound_target / ".harness" / "logs" / f"{STORY_ID}.log").is_file() + assert branches(sound_target) - before == {f"story/{STORY_ID}"} + assert runner.calls == ["implementer", "tester", "verifier", "documenter"] + + +def test_removing_the_offending_key_from_a_refused_target_lets_it_run( + sound_target, harness_root, offending_key, +): + """The refusal's guidance is "remove or correct each key", so the same + target with the key removed and nothing else changed must run. This is + the control paired most tightly with the refusal: one line of the same + file is the whole difference.""" + configure(sound_target, **{offending_key: "whatever"}) + assert run(sound_target, harness_root)[0] == 1 + + deconfigure(sound_target, offending_key) + + code, runner, _ = run(sound_target, harness_root) + assert code == 0, runner.calls + assert runner.calls == ["implementer", "tester", "verifier", "documenter"] + + +def test_several_undeclared_keys_are_all_named_in_one_refusal( + sound_target, harness_root, capsys, +): + """A developer who mistyped two keys is told about both, rather than + fixing one and meeting the next refusal.""" + configure(sound_target, **{RETIRED: "x", MISTYPED: "y", + "runs_dirr": ".harness/runs"}) + + code, runner, _ = run(sound_target, harness_root) + + refusal = capsys.readouterr().err + assert code == 1 + for key in (RETIRED, MISTYPED, "runs_dirr"): + assert key in refusal, key + assert runner.calls == [] + + +def test_a_comment_naming_a_retired_or_unknown_key_is_not_refused( + sound_target, harness_root, +): + """load_config strips comments before any key is recorded, so a config + documenting the key it used to carry still runs. + + Its control is the next test: the same line with its `#` removed is + refused, so the comment really did carry the name and the pass is the + stripping rather than the scan looking at the wrong file. + """ + path = sound_target / ".harness" / "config.yaml" + path.write_text( + path.read_text(encoding="utf-8") + + f"# {RETIRED}: {WORKING_RUNNER}\n" + + f"# {MISTYPED}: story/\n", + encoding="utf-8") + commit_setup(sound_target, "document the retired keys in a comment") + + code, runner, _ = run(sound_target, harness_root) + + assert code == 0, runner.calls + assert runner.calls == ["implementer", "tester", "verifier", "documenter"] + + +def test_the_same_line_without_its_comment_marker_is_refused( + sound_target, harness_root, capsys, +): + """The control for the test above.""" + path = sound_target / ".harness" / "config.yaml" + path.write_text( + path.read_text(encoding="utf-8") + + f"{RETIRED}: {WORKING_RUNNER}\n", + encoding="utf-8") + commit_setup(sound_target, "the same line, uncommented") + + code, runner, _ = run(sound_target, harness_root) + + assert code == 1 + assert RETIRED in capsys.readouterr().err + assert runner.calls == [] + + +def test_a_trailing_comment_on_a_declared_key_is_not_read_as_a_key( + sound_target, harness_root, +): + """The other half of the stripping: a comment on the end of a good line + does not become part of the key or a key of its own.""" + path = sound_target / ".harness" / "config.yaml" + path.write_text( + path.read_text(encoding="utf-8").replace( + "branch_prefix: story/", + f"branch_prefix: story/ # not {MISTYPED}"), + encoding="utf-8") + commit_setup(sound_target, "a trailing comment naming an unknown key") + + code, runner, _ = run(sound_target, harness_root) + + assert code == 0, runner.calls + + +# -------------------------------------------------------------------------- +# 3. The ordering: above every other pre-flight +# -------------------------------------------------------------------------- + + +def test_the_refusal_precedes_the_workflow_being_loaded_at_all( + sound_target, harness_root, capsys, offending_key, +): + """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 undeclared + key the run raises reading it. With the undeclared key it refuses + cleanly, which can only happen if the undeclared-key check ran first — + and the routing and self-route pre-flights read that workflow, so they + are below it too. + """ + configure(sound_target, workflow="xyzzy-no-such-workflow") + with pytest.raises(OSError): + run(sound_target, harness_root) + + configure(sound_target, **{offending_key: "whatever"}) + code, runner, _ = run(sound_target, harness_root) + + assert code == 1 + assert offending_key in capsys.readouterr().err + assert runner.calls == [] + + +def test_the_undeclared_key_is_what_speaks_when_the_story_artifact_is_missing( + sound_target, harness_root, capsys, offending_key, +): + """Its own control, in the same test: the identical fixture without the + undeclared key produces the later refusal, so the fixture really is + broken in the second way and the undeclared key really displaced it.""" + missing = "story-404" + later_code, later_runner, _ = run(sound_target, harness_root, missing) + later = capsys.readouterr().err + assert later_code == 1 + assert f"{missing}.yaml" in later, later + assert later_runner.calls == [] + + configure(sound_target, **{offending_key: "whatever"}) + code, runner, _ = run(sound_target, harness_root, missing) + + refusal = capsys.readouterr().err + assert code == 1 + assert offending_key in refusal + assert f"{missing}.yaml" not in refusal, refusal + assert runner.calls == [] + + +def test_a_dirty_tree_and_an_undeclared_key_together_report_the_key( + sound_target, harness_root, capsys, offending_key, +): + """The clean-tree pre-flight is the last one a developer meets before a + run directory exists, and it is below this one too.""" + configure(sound_target, **{offending_key: "whatever"}) + (sound_target / "dirty.txt").write_text("the developer's own\n", + encoding="utf-8") + + code, runner, _ = run(sound_target, harness_root) + + refusal = capsys.readouterr().err + assert code == 1 + assert offending_key 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( + sound_target, harness_root, capsys, +): + """The control for the test above: the dirty file really is a refusal of + its own, so the undeclared key displaced something rather than being the + only thing wrong.""" + (sound_target / "dirty.txt").write_text("the developer's own\n", + encoding="utf-8") + + code, runner, _ = run(sound_target, harness_root) + + assert code == 1 + assert "dirty.txt" in capsys.readouterr().err + assert runner.calls == [] + + +# -------------------------------------------------------------------------- +# 4. The configurations this repository ships +# +# A strict rule refuses every config carrying a key nothing reads, and this +# repository shipped three such configs before the story: its own, the +# template, and whatever l5-init wrote from that template. Each is loaded +# through the real reader and put through the real pre-flight predicate. +# -------------------------------------------------------------------------- + + +def test_this_repositorys_own_configuration_carries_no_undeclared_key(): + config = harness_config.load_config(REPO_ROOT) + assert harness_config.undeclared_config_problems(config, REPO_ROOT) == [] + # The control for that absence: the same check over the same config with + # a key put back reports it, so the empty list is about this file rather + # than about a predicate that reports nothing. + assert harness_config.undeclared_config_problems( + {**config, "project": "level-five"}, REPO_ROOT) + + +def test_the_template_carries_no_undeclared_key(tmp_path): + """Read through the real loader against a throwaway target, because the + template becomes a target's config verbatim but for one substitution.""" + target = tmp_path / "from-template" + (target / ".harness").mkdir(parents=True) + text = (REPO_ROOT / "templates" / "config.yaml").read_text(encoding="utf-8") + (target / ".harness" / "config.yaml").write_text( + text.replace("{test_command}", "echo tests-ok"), encoding="utf-8") + + config = harness_config.load_config(target) + assert harness_config.undeclared_config_problems(config, REPO_ROOT) == [] + assert harness_config.undeclared_config_problems( + {**config, "project": "sample"}, REPO_ROOT) + + +def test_the_template_carries_no_substitution_placeholder_but_the_command( + tmp_path, +): + """`{project}` left the template, so l5-init has nothing to substitute + for it. Its control is `{test_command}`, the placeholder that remains: + the same search over the same text finds that one.""" + text = (REPO_ROOT / "templates" / "config.yaml").read_text(encoding="utf-8") + assert "{project}" not in text + assert "{test_command}" in text + + +def test_l5_init_writes_a_config_a_run_would_not_refuse(tmp_path): + """The freshly initialised target, built by running the real script. + + Its control is the same check over the same produced file with a key + appended, which is reported — so "no undeclared key" is a fact about + what l5-init wrote. + """ + target = tmp_path / "fresh" + target.mkdir() + result = subprocess.run( + [sys.executable, str(REPO_ROOT / "scripts" / "l5-init"), + "--test-command", "echo tests-ok"], + cwd=target, capture_output=True, text=True) + assert result.returncode == 0, result.stderr + + written = (target / ".harness" / "config.yaml").read_text(encoding="utf-8") + assert "{project}" not in written, written + assert "echo tests-ok" in written + + config = harness_config.load_config(target) + assert harness_config.undeclared_config_problems(config, REPO_ROOT) == [] + assert harness_config.undeclared_config_problems( + {**config, "project": "fresh"}, REPO_ROOT) + + +def test_a_freshly_initialised_target_can_run_a_story( + tmp_path, target_root, harness_root, +): + """Not merely that l5-init's config loads: a story run against it reaches + its stages, which is the guarantee the story states. + + The config l5-init writes is copied over the shared fixture's, so the + story artifact, standards and git repository the fixture builds are + reused and the only thing under test is the configuration. + """ + fresh = tmp_path / "fresh-init" + fresh.mkdir() + result = subprocess.run( + [sys.executable, str(REPO_ROOT / "scripts" / "l5-init"), + "--test-command", "echo tests-ok"], + cwd=fresh, capture_output=True, text=True) + assert result.returncode == 0, result.stderr + + shutil.copyfile(fresh / ".harness" / "config.yaml", + target_root / ".harness" / "config.yaml") + configure(target_root, verification_runner=WORKING_RUNNER) + + code, runner, _ = run(target_root, harness_root) + assert code == 0, (runner.calls, result.stdout) + assert runner.calls == ["implementer", "tester", "verifier", "documenter"] + + +#: The declared set, read once, for deciding which runs of lines in a test +#: module's source are a configuration rather than some other `key: value` +#: text — a story artifact, a dict literal, a rendered YAML block. +DECLARED = harness_config.declared_config_keys(REPO_ROOT) + +_CONFIG_KEY = re.compile(r"^([a-z_][a-z_0-9]*):(\s.*)?$") + + +def _config_subset_line(raw: str) -> str | None: + """One source line reduced to the configuration line it carries, or None. + + Fixture configurations reach a target's `config.yaml` two ways: as a + triple-quoted block written verbatim, and as quoted list items joined + before being written. Both are the same subset once the surrounding + quoting, the trailing comma and a trailing escaped newline come off. + """ + line = raw.strip().rstrip(",") + if len(line) >= 2 and line[0] == line[-1] and line[0] in "\"'": + line = line[1:-1] + if line.endswith("\\n"): + line = line[:-2] + if line.lstrip().startswith("- ") or _CONFIG_KEY.match(line): + return line + return None + + +def _fixture_configurations(text: str) -> list[list[str]]: + """Every run of lines in a module's source that is a configuration. + + A run qualifies when at least two of its keys are declared ones and they + are the majority, which is what separates a fixture config from the + story artifacts and rendered YAML that share its shape. The majority + rule is deliberately not "all declared": a run carrying an undeclared + key is exactly what this must still return, or the sweep would skip the + thing it is looking for. + """ + found, block = [], [] + for raw in [*text.splitlines(), ""]: + line = _config_subset_line(raw) + if line is not None: + block.append(line) + continue + keys = [match.group(1) for match in + (_CONFIG_KEY.match(item) for item in block) if match] + declared = [key for key in keys if key in DECLARED] + if len(declared) >= 2 and len(declared) * 2 >= len(keys): + found.append(block) + block = [] + return found + + +def test_no_fixture_configuration_under_tests_carries_an_undeclared_key( + tmp_path, +): + """The sweep the story asked for, asserted rather than trusted, and for + any undeclared key rather than for `project` alone. + + Every `.harness/config.yaml` a module under tests/ writes verbatim is a + run of lines in that module's source, so the runs are found, each is + written to a throwaway target, read through the real loader and put + through the real predicate. A key nothing declares is reported whatever + it is called — the `project` the sweep removed, or a mistyping nobody + has made yet. + """ + def problems_for(lines: list[str]) -> list[str]: + target = tmp_path / "probe" + (target / ".harness").mkdir(parents=True, exist_ok=True) + (target / ".harness" / "config.yaml").write_text( + "\n".join(lines) + "\n", encoding="utf-8") + return harness_config.undeclared_config_problems( + harness_config.load_config(target), REPO_ROOT) + + # Three controls. The predicate reports a planted `project`, reports a + # planted key that is not `project` — the widening this test exists for — + # and the reader finds a configuration planted the way the fixtures carry + # theirs, so a sweep that reports nothing is a sweep that looked. + good = ["workflow: story-workflow", "branch_prefix: story/", + "test_command: echo tests-ok"] + assert problems_for(good) == [] + assert problems_for([*good, "project: sample"]) + assert problems_for([*good, f"{MISTYPED}: story/"]) + + planted = _fixture_configurations( + 'CONFIG = """\\\n' + "\n".join([*good, "project: sample"]) + '\n"""\n') + assert len(planted) == 1, planted + assert problems_for(planted[0]) + + scanned, offenders = [], [] + for path in sorted((REPO_ROOT / "tests").glob("*.py")): + for block in _fixture_configurations(path.read_text(encoding="utf-8")): + scanned.append(path.name) + offenders += [f"{path.name}: {problem}" + for problem in problems_for(block)] + + assert offenders == [], offenders + # And the sweep really reached the fixtures. Completeness is checked + # against a signal the reader had no part in: every module carrying a + # `workflow:` line, which every fixture configuration opens with, must be + # a module the reader returned a configuration for. A reader that found + # none of them would report no offender just as happily. + carriers = {path.name for path in (REPO_ROOT / "tests").glob("*.py") + if "\nworkflow: " in path.read_text(encoding="utf-8")} + assert carriers, "the completeness signal itself found nothing" + assert carriers <= set(scanned), sorted(carriers - set(scanned)) + assert "conftest.py" in scanned, scanned + + +def test_the_dict_built_fixture_configuration_carries_no_undeclared_key(): + """The one fixture configuration the sweep above cannot see. + + story-039's proofs build their config from a dict and render it, so no + run of `key: value` lines exists in that module's source to be found. + The dict is asked directly instead. Its control is the same predicate + over the same fixture with a key added, which `fixture_config` accepts + because it takes arbitrary overrides. + """ + import test_config_keys_are_obeyed as obeyed + + assert harness_config.undeclared_config_problems( + obeyed.fixture_config(), REPO_ROOT) == [] + assert harness_config.undeclared_config_problems( + obeyed.fixture_config(project="level-five"), REPO_ROOT) + + +# -------------------------------------------------------------------------- +# 5. The mechanism is gone, not joined by a sibling +# -------------------------------------------------------------------------- + + +def _scanned_sources() -> list[Path]: + """Every file that is harness source rather than harness prose. + + `.harness/` is excluded whole. The story excludes `.harness/runs/` + explicitly, and the same reason covers the rest of that directory: it is + the harness's record of its own work, not the harness. `.harness/stories/` + holds the story artifact that *asked* for the deletion, which necessarily + names what it deletes, and `.harness/docs/ARCHITECTURE.md` is a history + that may keep describing a mechanism in the past tense. Neither is a place + the mechanism could survive: what "deleted rather than joined by a sibling" + means is that no code, schema, prompt, workflow, rule or test spells it, + which is what the directories below hold. + + The tracked set is the working tree's `tests/*.py` united with what `git + ls-files` reports, and not `git ls-files` alone. A test file written by + this stage is untracked until the coordinator's `git add -A` at commit, so + a scan of the tracked set alone cannot see the module the claim is made + in: it would pass while this file spelled all three names, and start + failing the moment the story committed. The union puts the claimant inside + its own claim, which is what `test_the_scan_reaches_this_module` checks. + """ + listed = subprocess.run(["git", "-C", str(REPO_ROOT), "ls-files"], + capture_output=True, text=True, + check=True).stdout.split() + paths = {REPO_ROOT / name for name in listed + if not name.startswith(".harness/")} + paths.update((REPO_ROOT / "tests").glob("*.py")) + return sorted(path for path in paths if path.is_file()) + + +def _files_mentioning(name: str) -> list[str]: + found = [] + for path in _scanned_sources(): + try: + text = path.read_text(encoding="utf-8") + except (OSError, UnicodeDecodeError): + continue + if name in text: + found.append(str(path.relative_to(REPO_ROOT))) + return found + + +@pytest.mark.parametrize("name", DELETED_NAMES) +def test_the_retired_mechanisms_names_appear_nowhere_in_the_repository(name): + """Its control is the next test: the same scan over the same files finds + a name that does exist, so an empty result here means the name is gone + rather than that the scan is reading nothing.""" + assert _files_mentioning(name) == [] + + +def test_the_scan_that_found_nothing_can_find_something(): + """The negative control for the three assertions above.""" + found = _files_mentioning(SURVIVING_NAME) + assert "orchestration/harness_config.py" in found, found + assert "orchestration/story_coordinator.py" in found, found + + +def test_the_scan_reaches_this_module(): + """The other half of that control, and the one the absence needs most. + + The three names are gone from the harness; the place they are likeliest to + survive is a test module *about* their deletion. So this asserts that the + scanned set contains this file — had it not, the absence above would be + green while this module spelled all three, and would go red on its own + the moment the run committed and `git ls-files` began reporting it. + + A name written into this module is therefore found by the same scan, which + is what the second assertion shows: `SURVIVING_NAME` is spelled here, and + the scan reports this file for it. + """ + scanned = {str(path.relative_to(REPO_ROOT)) for path in _scanned_sources()} + here = str(Path(__file__).resolve().relative_to(REPO_ROOT)) + assert here in scanned, sorted(name for name in scanned + if name.startswith("tests/")) + assert here in _files_mentioning(SURVIVING_NAME) + + +def test_no_retirement_mapping_survives_on_harness_config(): + """The story's constraint that the refusal replaces the retired-key + refusal rather than joining it: no attribute of the module maps a name to + a replacement any more.""" + mappings = {name: value + for name, value in vars(harness_config).items() + if isinstance(value, dict) and not name.startswith("__")} + assert mappings == {}, mappings + # Named through DELETED_NAMES rather than written out, for the reason + # given there. Their control is the assertion beneath, which shows the + # same hasattr over the same two modules seeing what does exist. + _mapping, retired_problems, retired_refusal = DELETED_NAMES + assert not hasattr(harness_config, retired_problems) + assert not hasattr(story_coordinator, retired_refusal) + assert hasattr(harness_config, SURVIVING_NAME) + assert hasattr(story_coordinator, "_refuse_undeclared_config_keys") + + +# -------------------------------------------------------------------------- +# 6. The last language name has left orchestration/ +# -------------------------------------------------------------------------- + + +def test_the_scan_reports_only_the_two_legitimate_mentions_under_orchestration(): + """The point of deleting the literal. Its control is the next test.""" + under_orchestration = { + (finding.path, finding.line.strip()) + for finding in harness_source.scan(REPO_ROOT) + if finding.path.startswith("orchestration/") + } + expected = {(path, line) for (path, line) in stack_module.PERMANENT_MENTIONS + if path.startswith("orchestration/")} + + assert {path for path, _ in under_orchestration} == { + "orchestration/story_parser.py", + "orchestration/story_coordinator.py", + } + assert under_orchestration == {(path, line.strip()) + for path, line in expected} + + +def test_the_same_scan_reports_a_tie_planted_under_orchestration(tmp_path): + """The negative control for the absence above, built against a throwaway + root rather than by editing this one.""" + root = tmp_path / "throwaway" + (root / "orchestration").mkdir(parents=True) + (root / "orchestration" / "planted.py").write_text( + "COMMAND = 'pytest tests/'\n", encoding="utf-8") + + findings = harness_source.scan(root) + + assert [f.path for f in findings] == ["orchestration/planted.py"] + + +def test_permanent_mentions_holds_eight_and_none_is_harness_config(): + """Read off the list the other module owns, because this story's edit to + it is one of its acceptance criteria.""" + mentions = stack_module.PERMANENT_MENTIONS + assert len(mentions) == 8, sorted(mentions) + assert not [path for path, _ in mentions + if path == "orchestration/harness_config.py"] + # The control for that absence: the same comprehension over the same list + # finds the orchestration path that is still there. + assert [path for path, _ in mentions + if path == "orchestration/story_parser.py"] + + +def test_harness_config_no_longer_spells_any_stack_token(): + """The story's stated outcome, asserted of the file rather than of the + scan's allowlist. Its control is the same matcher over the same file's + text with the retired key put back.""" + source = (REPO_ROOT / "orchestration" / "harness_config.py").read_text( + encoding="utf-8") + assert not harness_source.STACK_PATTERN.search(source), source + assert harness_source.STACK_PATTERN.search( + source + f"\nRETIRED = {{'{RETIRED}': 'verification_runner'}}\n") From a21fae2ad48928ed83be3b544286a6f0ae6c66be Mon Sep 17 00:00:00 2001 From: "jerod.wilkerson" <30474318+jerodw@users.noreply.github.com> Date: Sat, 15 Aug 2026 18:26:01 -0600 Subject: [PATCH 2/2] Do not name a deleted module in the architecture document The documenter wrote 'it replaced tests/test_retired_config_keys.py', which this story deletes. story-038's check requires every tests/ path the document names to exist, and it does not, so the suite went red on main's CI. The sentence was true and the rule is right: a reader following a path in the document should find something. Reworded to keep the history without the dangling path. Nothing in the run could have caught this. The documenter runs after the verifier and after the clean-clone check, so its output is the only stage output no check ever sees. Filed as .harness/requests/the-clean-clone-check-runs-after-the-documenter.md. Co-Authored-By: Claude Opus 5 --- .harness/docs/ARCHITECTURE.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.harness/docs/ARCHITECTURE.md b/.harness/docs/ARCHITECTURE.md index cdac971..739023c 100644 --- a/.harness/docs/ARCHITECTURE.md +++ b/.harness/docs/ARCHITECTURE.md @@ -62,7 +62,7 @@ Since story-032 the `create` value is **a path at or beneath** one of that stage 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. -Since story-043 the declaration is also a **run-time check, and it is strict**: `harness_config.undeclared_config_problems(config, harness_root=None)` calls `declared_config_keys` and returns one problem per key a loaded config carries that the schema does not declare, in the order the config carries them. `run_story` calls it immediately after `load_config` and refuses through `_refuse_undeclared_config_keys` and the shared `refuse()` — **above** the workflow load and above every other pre-flight, so a refused run creates no run directory, no state file, no log, no branch, and invokes no agent. Only key *names* are examined; no value is validated, coerced or constrained, and `load_config`'s parsing is untouched, so a comment naming an unknown key is stripped before any key is seen and refuses nothing. `tests/test_undeclared_config_keys.py` holds the refusal's coverage — it replaced `tests/test_retired_config_keys.py`, whose subject no longer exists — and includes a sweep asserting that no fixture configuration under `tests/` carries an undeclared key, which is what keeps the rest of the suite runnable under the strict rule. +Since story-043 the declaration is also a **run-time check, and it is strict**: `harness_config.undeclared_config_problems(config, harness_root=None)` calls `declared_config_keys` and returns one problem per key a loaded config carries that the schema does not declare, in the order the config carries them. `run_story` calls it immediately after `load_config` and refuses through `_refuse_undeclared_config_keys` and the shared `refuse()` — **above** the workflow load and above every other pre-flight, so a refused run creates no run directory, no state file, no log, no branch, and invokes no agent. Only key *names* are examined; no value is validated, coerced or constrained, and `load_config`'s parsing is untouched, so a comment naming an unknown key is stripped before any key is seen and refuses nothing. `tests/test_undeclared_config_keys.py` holds the refusal's coverage — it replaced the retired-key module story-041 added and this story deleted, whose subject no longer exists — and includes a sweep asserting that no fixture configuration under `tests/` carries an undeclared key, which is what keeps the rest of the suite runnable under the strict rule. **Each problem names the offending key and lists the declared set** — `'' is not a key the harness reads; it reads: …` — the shape story-028's routing refusal takes, because a bare "unknown key" would leave the developer to find the vocabulary themselves. The declared set is composed into each problem rather than appended by the coordinator, so the function is self-contained and the refusal is actionable without opening the schema.